useBlockPreview: Try outputting EditorStyles to ensure local style overrides are rendered #55288
Conversation
|
Size Change: +4.03 kB (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
| }; | ||
| }, [] ); | ||
|
|
||
| return <EditorStyles styles={ styles } />; |
There was a problem hiding this comment.
How are these styles being added right now, or previously before the bug?
Why does EditorStyles not include these already?
There was a problem hiding this comment.
Prior to the bug being introduced the styles were output via a portal, so even in a child preview with its own block editor provider, the generated styles were being output by the parent. However, since moving over to the block editor store-based state approach for style overrides, this preview no-longer propagates (or renders) any styles generated by the blocks within its preview, because the local-to-the-preview state isn't being used anywhere. Or to put it differently setStyleOverride will only update the preview's block editor store, and not the parent environment's block editor store, which is where EditorStyles is currently used.
The proposed fix in this PR is to ensure that the preview also outputs EditorStyles, but only those styles generated from within the preview.
I've updated the PR description with the following:
Prior to this PR, for things that use useBlockPreview, the styles that are generated within that preview were not output anywhere. This is because ExperimentalBlockEditorProvider uses its own registry, so is designed to have values passed down to it, but not to propagate values outside of itself. So any calls to setStyleOverride would update state within the local block editor provider, and not be captured by the parent's EditorStyles.
andrewserong
changed the title
andrewserong
added
Backport to WP 6.7 Beta/RC
andrewserong
requested review from
youknowriad,
apeatling,
ajlende,
ramonjd and
tellthemachines
|
Update: I think I've gotten this PR working pretty well — here's a screengrab from testing it using Safari: 2023-10-13.12.27.50.mp4I've done the following:
Overall, I think this PR improves the behaviour quite a bit, but it is still a fairly hacky approach so will likely need to be thoroughly tested manually to ensure it doesn't break anything. I've updated the PR description and testing instructions, but please let me know if there's anything I've missed 🙂 |
There was a problem hiding this comment.
Thanks for persisting with this! The fix is working well for me across all platforms and the code change looks very clean.
My only question is if the Safari duotone fix should strictly be a part of this PR and the backport, given that it's an unrelated bug that was already around in the previous release.
Good question! From re-testing 6.3.1 without the Gutenberg plugin active, it looks like the previews were working in Safari, so it seems we need to include the Safari fixes in order for 6.4 to be consistent with the working behaviour in 6.3, as far as I can tell. But let me know if you've encountered that bug in 6.3. Here's a quick test video of 6.3 with Gutenberg deactivated: 2023-10-13.15.31.45.mp4One thing that is slightly unfortunate about the current state of the APIs is that it seems 6.3 was likely a little more performant than this fix. But short of reverting the |
|
For what it's worth, tested with:
And it's working as described. |
It's hard to tell if it's exactly the same bug because the whole preview behaviour is so badly broken on trunk 😬 Instead I was testing switching between duotone filters in Image blocks. On both trunk and 6.3.2, if you apply a filter and then switch to different colors two or three times, it freezes on the first color (on save, the front end updates as expected). That behaviour is fixed on this branch, so I assume that what that Safari fix addressed is an existing bug. Having said that, it seems to be a pretty safe change, so not a dealbreaker either way! I thought I'd mention it because we're getting pretty close to RC1 so it's best to be conservative about changes. |
Ah, gotcha 👍
Thank you for mentioning it! Good to be cautious this close to a release and make sure whatever we land is what we're all comfortable with. Let's see what kind of feedback we get for this PR over the weekend 🙂 |
… not needed since EditorStyles handles its own style overrides retrieval
|
I've done one more tiny update based on feedback from Riad in f076055 I think this should be ready for a final review now 🤞 |
|
Flaky tests detected in f076055. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6526723656
|
|
Thanks for the re-review @ramonjd and for all the feedback across both explorations, everyone! Since this fix has wound up being fairly small code-wise, and that it's testing well, and the current approach is consistent with all the feedback so far, I think this is in a good place to land. I'll merge it in now, but it should be a straightforward revert if we encounter any issues. Thanks again! 🙇 |
andrewserong
deleted the
try/alternative-fix-for-post-template-previews-styles
branch
…errides are rendered (#55288) * useBlockPreview: Try alternative fix for displaying local style overrides * Avoid duplicate styles, fix rendering issues in Safari * Add more explanatory comments * Remove additional check for styles within the block preview, as it is not needed since EditorStyles handles its own style overrides retrieval
|
I just cherry-picked this PR to the 6.4-rc1-2 branch to get it included in the next release: fe74c65 |
SiobhyB
removed
the
Backport to WP 6.7 Beta/RC
| // eslint-disable-next-line no-unused-expressions | ||
| blockElement.offsetHeight; | ||
| blockElement.style.display = display; | ||
| } |
There was a problem hiding this comment.
This is very annoying. How is this related to the original issue? Has this always been an issue in safari? Would be great to find a fix that doesn't rely on blockElement.
There was a problem hiding this comment.
Safari has a history of not correctly updating HTML elements with SVG filters applied. The approach used here of accessing el.offsetHeight was previously used when Safari wouldn't update the element correctly even on the first pass of rendering the page. That particular bug has been fixed now in Safari, but I guess it's broken now for updating CSS after the first pass.
It would be worthwhile to report this issue to https://bugs.webkit.org and add it to the list in #47302.
There was a problem hiding this comment.
Just checked the list of open webkit issues, and it looks like it's already tracked (from way back in 2012!) in this one: https://bugs.webkit.org/show_bug.cgi?id=99996 so I've added it to the list in #47302
This is very annoying. How is this related to the original issue? Has this always been an issue in safari? Would be great to find a fix that doesn't rely on blockElement.
It is annoying that we have to include browser-specific workarounds like this, but for now I think keeping this within Duotone and only when the styles are updated is probably the right place for it (at least for WP 6.4). As far as I can tell it's always been an issue in Safari, but the changes from outputting style tags via a portal to using the state-based styles in WP 6.4 appears to have made the issue more prominent. At least we've got the workaround in place for now... thanks again for the troubleshooting tips @ajlende!
* Add selector as id to layout style overrides. (#55291) * Fix flickering when focusing on global style variations (#55267) * ProgressBar: use text color to ensure enough contrast against background (#55285) * Use text color at different opacities for track and indicator * Add high contrast mode styles * CHANGELOG # Conflicts: # packages/components/CHANGELOG.md * Remove empty attrs. (#54496) * Remove empty attrs. * Fix linter errors --------- Co-authored-by: Sarah Norris <[email protected]> * Add IS_GUTENBERG_PLUGIN flag to LastRevision (#55253) * useBlockPreview: Try outputting EditorStyles to ensure local style overrides are rendered (#55288) * useBlockPreview: Try alternative fix for displaying local style overrides * Avoid duplicate styles, fix rendering issues in Safari * Add more explanatory comments * Remove additional check for styles within the block preview, as it is not needed since EditorStyles handles its own style overrides retrieval * Bug: PHP notice when an image with lightbox is deleted (#55370) * Fix PHP notice when an image with lightbox is deleted * Fix PHP notice when an image with lightbox is deleted --------- Co-authored-by: tellthemachines <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Marco Ciampini <[email protected]> Co-authored-by: Jonny Harris <[email protected]> Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: Kishan Jasani <[email protected]>
What?
Fixes: #54956
Explores an alternative to #55241 that instead of syncing style overrides, tries to output
EditorStylesinstead. This PR fixes an issue where Duotone styles set within a Post Template block are not rendered in non-selected instances of a Query Loop within the post or site editors. It also fixes an issue where Duotone styles in Safari were not updating for these previews, either.Why?
Prior to this PR, for things that use
useBlockPreview, the styles that are generated within that preview were not output anywhere. This is becauseExperimentalBlockEditorProvideruses its own registry, so is designed to have values passed down to it, but not to propagate values outside of itself. So any calls tosetStyleOverridewould update state within the local block editor provider, and not be captured by the parent'sEditorStyles.The fix in this PR is to output an
EditorStylescomponent within the preview, and ensure that the local-to-the-preview block editor state only contains local styles, so we avoid and duplicates between the preview and the parent block editor environment.There is a small drawback that the
styleandsvgtags in this PR are rendered as a direct child of the post template block'slielement, however based on feedback on this PR and #55241, it seems that this drawback is not as bad as the difficulty of accurately syncing betweenoverridesStylesstates, which was explored in #55241.This PR is currently my preferred approach to try to fix this bug.
How?
EditorStylesto output styles for theuseBlockPreview's local styles instead of syncing as in useBlockPreview: Try syncing local style overrides with parent block editor store #55241.useBlockPreview's settings that get passed down toExperimentalBlockEditorProvider, strip it ofstylesso that any styles applied in the parent environment are not duplicated in the child preview's editor styles. This ensures that only styles generated within that preview are output viaEditorStyles.Testing Instructions
Test markup
Testing Instructions for Keyboard
Screenshots or screencast
Before
Duotone is only applied to the selected item (as the others use a preview whose styles are not being propagated)
2023-10-11.15.17.32.mp4
After
Duotone is applied to all items, and the non selected items are updated to reflect the change. Adjusting Duotone for an image outside of the Post Template loop is unaffected:
2023-10-13.12.27.50.mp4