Global styles revisions: ensure that user-defined variation styles CSS is generated #62768
Conversation
ramonjd
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| @@ -56,7 +56,9 @@ function Revisions( { userConfig, blocks } ) { | |||
| [ originalSettings ] | |||
| ); | |||
|
|
|||
| const [ globalStyles ] = useGlobalStylesOutputWithConfig( mergedConfig ); | |||
| const [ globalStyles ] = useGlobalStylesOutputWithConfig( mergedConfig, { | |||
There was a problem hiding this comment.
I'm guessing the Style Book should also render these variations styles:
I tried it quickly, but I'm not yet sure it's enough - the preview doesn't render the style variation CSS, or update it when editing the global block styles.
Could be missing something. I'll look next week. Might be a different PR.
|
Size Change: +430 B (+0.02%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
| @@ -1369,7 +1376,10 @@ export function useGlobalStylesOutputWithConfig( | |||
| hasBlockGapSupport, | |||
| hasFallbackGapSupport, | |||
| disableLayoutStyles, | |||
| disableRootPadding | |||
| options?.disableRootPadding, | |||
| { | |||
There was a problem hiding this comment.
Note to self: I think toStyles is also ripe for a small refactor. Moving the following arguments
hasBlockGapSupport,
hasFallbackGapSupport,
disableLayoutStyles = false,
disableRootPadding = false,
into styleOptions
There was a problem hiding this comment.
Thanks for getting the ball rolling on this one @ramonjd 💪
While this does appear to solve the issue for simple applications of block style variations, I can confirm there is still an issue when it comes to nested variations that define inner block type or element styles.
Here's a rough demo video showing incorrect styles when variations define styles for inner group blocks.
Screen.Recording.2024-06-23.at.2.48.07.PM.mp4
With the current approach in this PR the styles get generated once only which causes the last defined variation style to target the inner block to take precedence incorrectly.
This nesting use case was the driving force behind applying styles in a per-application or instance way. To solve it here we'll probably need something similar to the block-style-variation.js hook.
|
In case it helps, here's some sample block markup and a few very simple section styles that can be used to replicate the issue. They aren't exactly what was in the video in my last comment but close enough. Example block markup<!-- wp:group {"className":"is-style-section-c","style":{"spacing":{"padding":{"right":"var:preset|spacing|30","left":"var:preset|spacing|30","top":"var:preset|spacing|30","bottom":"var:preset|spacing|30"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group is-style-section-c" style="padding-top:var(--wp--preset--spacing--30);padding-right:var(--wp--preset--spacing--30);padding-bottom:var(--wp--preset--spacing--30);padding-left:var(--wp--preset--spacing--30)"><!-- wp:paragraph -->
<p>Parent</p>
<!-- /wp:paragraph -->
<!-- wp:group {"className":"is-style-section-b","style":{"spacing":{"padding":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|30","left":"var:preset|spacing|30","right":"var:preset|spacing|30"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group is-style-section-b" style="padding-top:var(--wp--preset--spacing--30);padding-right:var(--wp--preset--spacing--30);padding-bottom:var(--wp--preset--spacing--30);padding-left:var(--wp--preset--spacing--30)"><!-- wp:paragraph -->
<p>Group</p>
<!-- /wp:paragraph -->
<!-- wp:group {"className":"is-style-section-a","style":{"spacing":{"padding":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|30","left":"var:preset|spacing|30","right":"var:preset|spacing|30"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group is-style-section-a" style="padding-top:var(--wp--preset--spacing--30);padding-right:var(--wp--preset--spacing--30);padding-bottom:var(--wp--preset--spacing--30);padding-left:var(--wp--preset--spacing--30)"><!-- wp:paragraph -->
<p>Child</p>
<!-- /wp:paragraph -->
<!-- wp:group {"className":"is-style-default","style":{"spacing":{"padding":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|30","left":"var:preset|spacing|30","right":"var:preset|spacing|30"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group is-style-default" style="padding-top:var(--wp--preset--spacing--30);padding-right:var(--wp--preset--spacing--30);padding-bottom:var(--wp--preset--spacing--30);padding-left:var(--wp--preset--spacing--30)"><!-- wp:paragraph -->
<p>Grandchild</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->Section style partial theme.json files{
"$schema": "https://schemas.wp.org/trunk/theme.json",
"version": 3,
"title": "Section A",
"slug": "section-a",
"blockTypes": [ "core/group" ],
"styles": {
"color": {
"background": "slategrey",
"text": "snow"
},
"blocks": {
"core/group": {
"color": {
"background": "darkslategrey",
"text": "whitesmoke"
}
}
}
}
}{
"$schema": "https://schemas.wp.org/trunk/theme.json",
"version": 3,
"title": "Section B",
"slug": "section-b",
"blockTypes": [ "core/group" ],
"styles": {
"color": {
"background": "teal",
"text": "seashell"
},
"blocks": {
"core/group": {
"color": {
"background": "mediumturquoise",
"text": "white"
}
}
}
}
}{
"$schema": "https://schemas.wp.org/trunk/theme.json",
"version": 3,
"title": "Section C",
"slug": "section-c",
"blockTypes": [ "core/group" ],
"styles": {
"color": {
"background": "darkslateblue",
"text": "aliceblue"
},
"blocks": {
"core/group": {
"color": {
"background": "slateblue",
"text": "linen"
}
}
}
}
} |
|
Thanks for explaining all that. I haven't delved too deeply yet, but it looks tricky.
Interesting. So I see the editor is generating styles under "variation classname + clientId" selectors to target nested elements. And it's happening in the useBlocks hook. Revisions prints a block list, but template level blocks, e.g., post-content block. I still have a superficial overview, so I don't completely understand what needs to be done yet, but how right am I in assuming that we'd have to:
And all that every time a user switches between revisions. For point one, I'll check if there's a hook. I don't like the sound of iterating through a tree of blocks and checking for variations. |
That aligns with my initial impressions once this issue had been uncovered.
There was a hook specific to this smaller task however there was a recent refactor that improved the
Something like this will be needed.
There might be a shortcut we can take to determine the blocks that have variations applied. The styles in the editor are generated and applied as style overrides. To achieve nested section styling we needed to ensure these overrides were stored with the blocks My understanding (which is very limited) of the revisions UI is that the existing blocks on the page you are viewing are passed to a new iframed editor. Will those blocks' clientIds change? If not, we could leverage the style overrides to know which blocks need the section styles generated rather than iterating the block tree. |
No, the block list is static once open 👍🏻
Ah, I didn't realize the style overrides were stored and we could access them. That opens another door, thanks! |
I think understand the question better now: yes the client ids change because the block list is rendered again in the revisions view. Between applying style revisions, the client ids don't change again. So this, I think, throws doubts on using the |
That's a shame 🤔 If we set the variation class on the blocks that have variations applied before passing them to revisions perhaps that gives us something to rely on. Without having had a chance to dig into the code yet, it seems to me like we need a few things:
|
ramonjd
force-pushed
the
try/style-revisions-build-style-variations
branch
from
7f7128e to
ad91fbe
Compare
| const overrides = useSelect( | ||
| ( select ) => unlock( select( blockEditorStore ) ).getStyleOverrides(), | ||
| [] | ||
| function EditorStyles( { styles, scope, overrides } ) { |
There was a problem hiding this comment.
Converted to draft while I refactor and question life choices.
This is the main thrust of things: passing overrides to EditorStyles, which can override the overrides. 🙃
I'm overriding them all. Maybe better to merge them?
There was a problem hiding this comment.
I'm only passing EditorStyles the overrides that need to override the overrides 😄
And now, merging them with the store overrides.
| @@ -59,6 +59,72 @@ function getVariationNameFromClass( className, registeredStyles = [] ) { | |||
| return null; | |||
| } | |||
|
|
|||
| export function useGetBlockVariationStylesWithConfig( | |||
There was a problem hiding this comment.
This is a first draft, but the basic logic is to grab the current overrides and update their CSS values with any incoming config. In our case, the user config from a styles revisions.
Ignore the naming. Maybe useUpdateBlockVariationOverridesWithConfig
I'm also looking for opportunities to reuse some of the logic within the block style variation hook, but that's a later thing.
|
|
||
| const mergedOverrides = useGetBlockVariationStylesWithConfig( | ||
| config, | ||
| cloneObject( variationOverrides ) |
There was a problem hiding this comment.
Otherwise we're mutating the overrides object it seems. If we don't clone, we can just run the hook and not pass the result to Editor Styles.
This seems like a bug in the store, because mutating shouldn't be possible? That is, the result from useSelect should always be a unique reference.
I don't know yet. Too hungry. I'll check.
aaronrobertshaw
added
the
Backport to WP 6.7 Beta/RC
| */ | ||
| export function useUpdateBlockVariationOverridesWithConfig( | ||
| config, | ||
| variationOverrides = [] |
There was a problem hiding this comment.
There are probably opportunities to abstract this and share some love with useBlockStyleVariation. Perhaps return an override object instead. I'll look into it later - first on the agenda is to see if this PR flies at all.
| * @param {Object} config A global styles object, containing settings and styles. | ||
| * @return {Array} An array of new block variation overrides. | ||
| */ | ||
| export function useUpdateBlockVariationOverridesWithConfig( config ) { |
There was a problem hiding this comment.
Actually, what I'm thinking would be better than all this is for useBlockStyleVariation() to have access to a mergedConfig that contains the current revision.
gutenberg/packages/block-editor/src/hooks/block-style-variation.js
Lines 142 to 153 in 8ba1ae2
Maybe it could be return value from useContext( GlobalStylesContext );, e.g.,
const { merged: mergedConfig, currentRevisionConfig } = useContext( GlobalStylesContext );I'm just riffing here. Not certain if it'd work.
However, I think this scenario should be taken into account for:
One of the suggestions is to have the site editor recognise global styles as a valid entity, as it does for templates, pages and so on. Global styles could not only be the current mergedConfig, but a revision config.
If I get time I'll start looking at that.
There was a problem hiding this comment.
Both options sound worth exploring post 6.6>
| export function useUpdateBlockVariationOverridesWithConfig( config ) { | |
| export function useUpdateBlockStyleVariationOverridesWithConfig( config ) { |
I know the name is getting pretty long but I think it helps to be clear this is to do with block style variations, not block variations.
|
I'm having a hard time understanding what the problem is. Is it the order of rendering the style overrides? This could also be fixed by passing the clientId, and then rendering the styles in the right block order? |
Good question. Sorry, the PR is young and dynamic and I haven't updated the description properly. No, it's the content of the style overrides. Initially, the overrides are set using the global styles config in the However, the revision history contains multiple versions of the global styles config that need to be swapped in and out dynamically.
Could you elaborate? Passing the clientId to where? I'm interested to see if this helps. |
There was a problem hiding this comment.
Thanks again for taking the time to review this PR, folks.
As mentioned, the fact that confusion exists surrounding this change tells me:
- I need to explain the problems we're trying to solve more clearly (admittedly it took me a while to get my head around style variations in the last few days!), and
- I need to look at my other idea of setting a "currentRevision" config or something that can be fetched in useBlockStyleVariation. This would, if it works like I think it would, mean far less code, but potentially require a "currentRevision" record somewhere in the core data store.
Are there e2e tests for this?
I'll work on that tomorrow.
| } | ||
|
|
||
| function EditorStyles( { styles, scope, overrides } ) { | ||
| const styleOverrides = useSelect( |
There was a problem hiding this comment.
useUpdateBlockStyleVariationOverridesWithConfig fetches the overrides only as a reference to check if a block has a block style variation applied.
If so, and a matching path in the currently-selected revision user config exists, then it will return a new array containing a set of overrides specific to the current revision.
The merge here in EditorStyles is a merge of CSS only, basically, "hey, I have these other overrides I'd like to output."
| * @param {Object} config A global styles object, containing settings and styles. | ||
| * @return {Array} An array of new block variation overrides. | ||
| */ | ||
| export function useUpdateBlockStyleVariationOverridesWithConfig( config ) { |
There was a problem hiding this comment.
The style overrides in the store are generated using the current user global config.
We have to be able to recreate/create style variation overrides using any config, such as a global styles revision CPT.
With this PR I found the following constraints:
- The revision preview recreates the block list, which creates new clientIds
useStyleOverride()runs for these ideas, but it uses the current user global styles config- The quickest way to tell if a style variation override existed for the blocks rendered in the revision preview was to check the overrides store.
- I wanted to call
useStyleOverride()again to overwrite the overrides in the store using the revision config, but I had to do so by looping over a collection
It was a all bit of a rush job.
That it's causing some confusion informs me that I should pursue the other idea of setting a "currentRevision" config or something that can be fetched in useBlockStyleVariation
ramonjd
force-pushed
the
try/style-revisions-build-style-variations
branch
from
d7078b5 to
323a990
Compare
|
Flaky tests detected in 55cc60c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9691349709
|
| }, | ||
| } ); | ||
|
|
||
| test.describe( 'Block Style Variations', () => { |
There was a problem hiding this comment.
This is a bit of a monster. It checks:
- Applying block style variations in the site editor to blocks and nested blocks
- Checking that those styles are applied correctly
- Updating block style variations in global styles, and checking that those styles are applied correctly.
- Checking that the revisions preview matches the editor canvas
There was a problem hiding this comment.
Nice work @ramonjd 👍
I like that we're not touching the public EditorStyles component now. Bonus points for the e2e test as well!
Another round of testing the latest approach shows everything still working as expected.
✅ Existing section styles functionality is working
✅ Revisions containing block style variations display correctly
✅ Applying revisions still works and correct variation styles are in effect
✅ Tweaks to custom block style variations work as expected
✅ Revisions containing style variation changes are reflected in post editor and frontend after applying
I left a few minor questions about the e2e as inline comments but all in all this is looking pretty good to me. ✨
| // Check nested grandchild group block inherited styles. | ||
| await expect( thirdGroup ).toHaveCSS( 'border-style', 'groove' ); | ||
| await expect( thirdGroup ).toHaveCSS( 'border-width', '22px' ); |
There was a problem hiding this comment.
I know this test is a beast already but I was wondering if we should also apply the first variation to the grandchild here at the end?
Something to ensure that earlier defined style variations don't have their styles overridden by a later definition's inner block type styles.
There was a problem hiding this comment.
Just checking I understand - so the test is to apply a block style variation (A) to the nested grandchild block to ensure that A's styles overwrite any inherited styles from parent blocks.
I'll add it and you can tell me if I got it right 😄
There was a problem hiding this comment.
Yep, I believe we're on the same page.
If only a simple stylesheet was being generated and there was a single set of CSS for each variation, including inner block type styles, for example:
:root :where(.is-style-variation-a .wp-block-group) {...}
:root :where(.is-style-variation-b .wp-block-group) {...}Then checking that variation B gets applied when selecting it on the child covers whether that variation then overcomes the inner block type styles for variation A applied to the parent. So far, so good 👍
Given the same specificity in the example above, the variation B inner block type styles would override the selection of variation A on the grandchild due to being loaded later.
Adding this extra test for the application of variation A again on the grandchild will cover that we're not relying on the load order of the CSS. In other words, it ensures we're creating the per-instance styles appropriately so that the most immediate variation's styles are in effect.
I hope that makes it clearer 🤞
…stage. Getting closer. But the overrides in state need to be merged with incoming revision styles.
…g config from the revision.
Destructure in hook so the consumer doesn't have to clone Only send the override overrides to EditorStyles that need to be overridden.
add overrides to dep array in Editor Styles rename hook
Register revision overrides with useStyleOverride
Add rudimentary E2E test covering block style partials, applying them, updating them and viewing styles revisions.
ramonjd
force-pushed
the
try/style-revisions-build-style-variations
branch
from
52133da to
55cc60c
Compare
There was a problem hiding this comment.
Thanks for the hard work and patience here @ramonjd 👍
I've given this PR another test and it continues to work nicely.
I'm happy to give this an approval again but if you do feel it warrants a second opinion after my false start, that's fine too 😁
|
There was a conflict while trying to cherry-pick the commit to the wp/6.6 branch. Please resolve the conflict manually and create a PR to the wp/6.6 branch. PRs to wp/6.6 are similar to PRs to trunk, but you should base your PR on the wp/6.6 branch instead of trunk. |
…S is generated (#62768) * Render styles after the variation style overrides have been saved to stage. Getting closer. But the overrides in state need to be merged with incoming revision styles. * For every current override, update the variation CSS with the incoming config from the revision. * Rename hook Destructure in hook so the consumer doesn't have to clone Only send the override overrides to EditorStyles that need to be overridden. * Fetching overrides in the hook * Feedback suggestions from review: add overrides to dep array in Editor Styles rename hook * Return getBlockStyles from the useSelect callback * Refactor so we don't have to change the EditorStyles props Register revision overrides with useStyleOverride * Adding some explanatory comments Add rudimentary E2E test covering block style partials, applying them, updating them and viewing styles revisions. * Removed unused style fixture Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: ellatrix <[email protected]>
|
Cherry pick PR to |
* Render styles after the variation style overrides have been saved to stage. Getting closer. But the overrides in state need to be merged with incoming revision styles. * For every current override, update the variation CSS with the incoming config from the revision. * Rename hook Destructure in hook so the consumer doesn't have to clone Only send the override overrides to EditorStyles that need to be overridden. * Fetching overrides in the hook * Feedback suggestions from review: add overrides to dep array in Editor Styles rename hook * Return getBlockStyles from the useSelect callback * Refactor so we don't have to change the EditorStyles props Register revision overrides with useStyleOverride * Adding some explanatory comments Add rudimentary E2E test covering block style partials, applying them, updating them and viewing styles revisions. * Removed unused style fixture Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: ellatrix <[email protected]>
|
This was manually cherry-picked in #62905. |
ellatrix
added
Backported to WP Core
What?
Hi.
Global styles revisions should output block style variations CSS if the revision contains variations styles. This PR does that.
TODO
How?
By rendering the editor styles after the block list has rendered, at which the variation overrides are in the state and, therefore, accessible.
Then, using the current revision history config , grabbing the blocks' current style overrides, regenerating them using the current revision config, and re-registering them via
useStyleOverride.Why?
A bit of code quality as it makes room for future, additional properties, but alsoto fix a bug where the CSS for user-edited style variations in the global styles CPT/revisions aren't generated in the global styles revisions preview.To generate style variation overrides, the style overrides CSS is generated using the global styles config in the
useBlockPropshook.However, the revision history contains multiple versions of the global styles config that need to be swapped in and out dynamically.
Testing Instructions
/stylese.g.,/styles/white-fang.json{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 3, "title": "White fang", "blockTypes": [ "core/group", "core/image", "core/columns" ], "styles": { "color": { "background": "yellow" }, "border": { "radius": "1111px", "width": "10px", "color": "red", "style": "solid" }, "blocks": { "core/paragraph": { "border": { "width": "0" }, "color": { "background": "aliceblue", "text": "purple" } } } } }White fangblock style variation.White fangblock style variation, saving each time.Thank you!
Screenshots or screencast
Kapture.2024-06-24.at.15.39.10.mp4