Theme JSON: use block global style for block gap where no gap value exists in layout.php #39870
Conversation
| // Some styles such as blockGap are only meant to be available at the top level (ROOT_BLOCK_SELECTOR), | ||
| // hence we only output styles at the top level. | ||
| if ( 'top' === _wp_array_get( self::VALID_STYLES, array( $value_path[0], $value_path[1] ), null ) && static::ROOT_BLOCK_SELECTOR !== $selector ) { | ||
| continue; |
There was a problem hiding this comment.
I'm suspect of this. It seems fragile, only in the sense that I'm not aware of any side-effects.
Needs lots of testing.
ramonjd
added
the
Global Styles
|
Size Change: +44 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I know the tests are failing. This PR is just to test out an idea from #39789 |
There was a problem hiding this comment.
This is awesome, thanks for picking this one up @ramonjd 🙇 — I haven't test it yet, but just added a quick drive-by comment about where we do the checking for the block-level value from global styles.
Looks like a good direction to go in to me, and happy to give it a more detailed test / look on Monday if no-one beats me to it!
ramonjd
force-pushed
the
try/global-styles-skip-block-gap-style-generation
branch
from
7f7d330
to
f0344ff
Compare
ramonjd
changed the title
ramonjd
added
CSS Styling
ramonjd
requested review from
oandregal,
TimothyBJacobs and
spacedmonkey
as code owners
| // If there is no block-level value for blockGap, | ||
| // but a global styles value available for blockGap, | ||
| // use the latter. | ||
| if ( empty( $gap_value ) ) { |
There was a problem hiding this comment.
Why is this needed? isn't the fallback supposed to happen using the CSS variable?
There was a problem hiding this comment.
(And we can only set the CSS variable at the global level theme.json)
There was a problem hiding this comment.
Why is this needed?
Good question.
Logically it's not.
I only put it there so we could skip over trying to fetch the global value via gutenberg_get_global_styles if a gap value set in the block's local styles attribute is found, which would take precedence anyway.
isn't the fallback supposed to happen using the CSS variable?
It will either way, unless I'm missing something, which could be the case 😇
My thinking went along of these lines: if the block's local styles attribute does not contain a gap value, we'll try the block's global styles.
Either could still be null by the time we call gutenberg_get_layout_style() in which case the fallback would be --wp--style--block-gap, which is set at the root.
Does that sound right?
There was a problem hiding this comment.
It does feel like duplication to me, since if the first one is null, the fallback will be the CSS variable (which corresponds to the root value).
Or in other words, if we do the fallback in php, why do we have the CSS variable in the first place.
There was a problem hiding this comment.
It does feel like duplication to me, since if the first one is null, the fallback will be the CSS variable (which corresponds to the root value).
Thanks for clarifying.
Just so I understand your concerns, are you seeing somewhere where the root block gap value, as opposed to the block-level global style value, is returned from
$spacing_global_styles = gutenberg_get_global_styles( array( 'blocks', $block['blockName'], 'spacing' ) );on this line?
If so, yes, that would be a mistake.
The intention of this PR is to get gap working for block-level global styles by:
- filtering out block-level global styles for blockGap, but NOT the root, in the style sheet. See this line
- using the block-level global styles value, which still exists in the theme json tree, as a fallback ONLY IF the block attributes do not contain a
style.spacing.blockGapvalue. If the former doesn't exist, we'll use the root value.
The effect of point 1 is that a user sets the value of, say, the Group block's gap in global styles, .wp-block-group { --wp--style--block-gap: '{someCustomValue}px} won't be rendered:
<style id='global-styles-inline-css'>
body {
/* this will still be here! */
--wp--style--block-gap: 1.5rem;
}
/* block-level global styles rules will not be printed */
</style>The consequence of point 2 – at least what I'm attempting - is that if a user has set the value of Group block's gap in global styles it will be used when an individual block's attributes do not contain a style.spacing.blockGap value.
So the priority should still be:
- try to use block attributes first
- then the block's global styles second
- finally the root value
If things aren't working that way in this PR it means I've done something wrong! 😬
There was a problem hiding this comment.
You raise good points, thanks for clarifying.
This basically changes how the "style engine" of layout works, instead of being styleObject => CSS, it's now ( styleObject, theme.json ) => CSS. It changes the mental model and will have an impact on the style engine work.
Yeah, I see what you mean. We might run into trouble if we mix block support style attributes with global styles.
we're planning to add "theme.json" like support to each block (containers)
I wasn't aware of Style Partials (theme.json) so I agree that a change like the one in this PR might come back to bite us. 👍
I was having a discussion with @tellthemachines about something similar over at Try using variable for root padding value #39926
I was wondering about an alternative: a const VALID_TOP_LEVEL_STYLES (or whatever) in WP_Theme_JSON_Gutenberg that explicitly defines unique, root-level-only CSS vars.
body {
--wp--root--style--block-gap: 2px;
--wp--root--style--padding: 10px;
/* etc ... */
}
I haven't really thought this through, but I was venturing towards checking VALID_TOP_LEVEL_STYLES when we assemble the styles in compute_style_properties and appending --wp--root-style-- for ROOT_BLOCK_SELECTOR styles.
Or there might be a tricky way to assemble them in sanitize() via $schema['styles'], $schema['styles']['elements'] and $schema['styles']['blocks'].
Uniquely identifying global CSS vars has a benefit, aside from explicitly stating their purpose, in that their values can be used for top-level layout calculation.
For example, in #39926, full-width blocks can use a root padding CSS var set a negative margins so that they stretch to the "full-width" of the screen.
Ultimately, we need to know how to treat blockGap when it is a root or a block/element style value and output something different accordingly.
I can chat about it with @andrewserong and see what we can cook up.
There was a problem hiding this comment.
Great questions.
This basically changes how the "style engine" of layout works, instead of being styleObject => CSS, it's now ( styleObject, theme.json ) => CSS. It changes the mental model and will have an impact on the style engine work.
Totally, I think it reveals some pragmatic challenges surrounding how we output styles, particularly for the layout support, which as @ramonjd mentions further down, is a little different to the other block supports.
In the case of this PR (and the current implementation of the layout support), everything happens at the individual block level when it's rendered, so out of necessity, to factor in global styles, we've kinda got to "look up" to the parents in order to deal with those values (or use CSS variables to stand-in for it).
In an ideal circumstance, with the style engine, how differently might we be doing things? The model for the style engine is a lot clearer for supports like padding, margin, typography, etc, where we'd like to consolidate the approach at the individual block level and global styles, and using a block's classname is an appropriate hook for it. In some of the explorations (like #39374) it looks like we'll be able to move more to global styles, but the unique block gap values was still a bit of mystery to me as to how best to handle it, and neatly declare values that get passed down / or cascade down correctly.
With block gap, because the value is used directly in different circumstances (rendering flow vs flex gap), it does create this additional challenge. I think because the use case is so different to each of the other block supports, I'm wondering if the approach in this PR would be an acceptable way to unblock getting the global styles support in, and if we could treat it as an implementation detail we could potentially swap out if and when partials in #39281 is explored.
From my perspective, it could be worthwhile proceeding with this change, so long as:
- We're happy that folks will set
blockGapvalues at the block level withintheme.jsonand via the global styles interface in the site editor. - So long as we're happy with the shape of the data as it's stored, then theoretically, we could change the implementation after the fact, once we have a clearer idea of how we'd like to handle outputting the layout styles in the style engine.
From some of the explorations we've looked at recently, given that a fair bit of the logic winds up being moved to the Theme JSON class anyway, I don't think that the current change here would preclude further work, but very happy to be proved wrong, as I definitely don't want to nudge us into a decision that's difficult to reverse!
There was a problem hiding this comment.
With block gap, because the value is used directly in different circumstances (rendering flow vs flex gap), it does create this additional challenge.
One alternative could be to keep the current logic (selectors specific styles but output something like):
.selector.is-layout-flex {
gap: something;
}
.selector.is-layout-default > * {
margin-top: something;
}
To be honest, I didn't think much about this, it's just a random idea for now, but it's an alternative that allows us to avoid that dependency to parent global style.
From some of the explorations we've looked at recently, given that a fair bit of the logic winds up being moved to the Theme JSON class anyway, I don't think that the current change here would preclude further work, but very happy to be proved wrong
I don't know 🤷 to be honest, personally I don't have a full confidence here but I trust your judgement.
There was a problem hiding this comment.
One alternative could be to keep the current logic (selectors specific styles but output something like):
I really like that approach — I'd like it if we can output something like that in the longer term. I might be overthinking it, but I think that approach slightly opens up the can-of-worms surrounding rendering out semantic classnames, which we're in more of the proposal / exploratory phase of (since we'd need to adjust the logic for how the theme JSON class outputs styles, and then the layout support to inject the classnames).
I don't know 🤷 to be honest, personally I don't have a full confidence here but I trust your judgement.
Thank you, much appreciated! Since I was one of the folks to propose the idea of looking up global styles, I think I'd feel more comfortable with getting a couple more opinions if you're not totally sold on the idea, to confidence check before proceeding. I think my main interest is to see how viable we think a short-term fix is (so that we can fix this bug in WP 6.0) and then aim to have the class-based approach like in your code snippet in time for WP 6.1.
There was a problem hiding this comment.
cc @jorgefilipecosta @oandregal @mcsf if you have any opinions here.
|
I've been toying around with alternatives, hoping to cover the requirements of root padding CSS vars as well, but I can't yet find something that satisfies all conditions, namely:
Stepping back, if the approach in this PR doesn't end up being the way to go, should we rethink how Ignoring how hard it would be to do, given that
I'm not sure, just thinking aloud right now 🔊 |
I quite like the approach in this PR, but yes, it probably is worth giving some thought to how
Not sure how viable that'd be, but just an idea, so we don't wind up with two gap features that are very similar 🤔
I actually quite like "spacing" as the grouping for it, because controlling the gap between things is so closely related to padding and margin, where other controls like orientation, justification, etc, aren't unit-based, so more naturally fit into |
Great stuff. I like this idea, at least as a soft alternative to keep the spirit of this PR alive. 🥇 |
ramonjd
force-pushed
the
try/global-styles-skip-block-gap-style-generation
branch
from
db592a2
to
58aa996
Compare
I was thinking about this today. To make the gap CSS property useful, the block would have to employ flex anyway. It would be nice to separate the two still (layout gap and regular gap).
I think it's gap that starkly highlights the edge case because of the way it works at the root level. I can't yet come up with any other use case for why we'd want to look up global styles at the block supports level, which means Gutenberg's inheritance model is working fairly well I guess 😄 As @andrewserong mentions above, the other block supports play much more nicely. So in my mind, either we accept that block gap is a special case and carry on, or we split/change the way block gap works (no idea about that one 🤣 ) or we run with this PR, and look up the Gutenberg style inheritance tree to determine the fallback. The latter would fix the bug until something better comes along. To be honest I'm not sure either. Given the the inheritance model of global styles > block global styles > block styles, and somewhere in between elements and, soonish, style "partials" I wonder how long we can get away with the Style Engine only knowing about a block's style object to the exclusion of all other styles that may effect that particular block. Maybe things will become clearer once we work out how the Style Engine will gather and output block styles/classnames: we'll be able to see inheritance/cascade on the frontend. The answer might lie in the way we structure the CSS as opposed to PHP logic. 🤷 |
Or grid! In that case, we're really saying that the block that's opted-in has provided its own
I think I'm leaning in this direction, personally. Given that in alternate ways of doing it (like rendering out the class name from the theme JSON class) we're still pulling the value from the same place in the data structure, it seems like the proposed fix here isn't one that would be too difficult to revert if we needed to, since it doesn't alter any saved data.
That's a good question 🤔, it's very much from the perspective of everything cascading down correctly, which I really like. 🤞 between being able to add more specific selectors, multiple attributes per classname, etc, and consolidating rendering a style object between global styles + at the individual block level, I think we should be able to cover most use cases. Riad's very cool suggestion for the classname-based approach above is a good example of one of the things that should be fairly straightforward if and when we settle on one of the layout-as-presets approaches. It's highly likely we'll still encounter edge cases that don't quite fit into things, but at least from the style engine perspective — if we've simplified and consolidated rendering a fair bit, and removed a lot of the duplication and one-off code in block supports, and if folks find it easy enough to contribute to, then I think we'll have succeeded.
I think so too 🙂 |
Thanks for your thoughts @andrewserong 🙇 One thing that only occurred to me just now is that
$block_gap = gutenberg_get_global_settings( array( 'spacing', 'blockGap' ) );
$default_layout = gutenberg_get_global_settings( array( 'layout' ) );Maybe part of the paradigm should be to consider it a case of "layout implementation exceptionalism". 😄 Seeing it this way, it makes me sleep better |
ramonjd
force-pushed
the
try/global-styles-skip-block-gap-style-generation
branch
from
58aa996
to
3ea2641
Compare
|
@ramonjd is this still something we are trying to land in 6.0. If not, I will remove from the project board. Thanks! |
Thanks for checking @ndiego I'll remove it. I'm not convinced we'll come up with a satisfactory compromise in time. |
|
This work seems to be connected to the idea of block partials, which is blocked by conditional loading of the styles for each blocks. I had a go at introducing that change here: #40513 I'd appreciate some input from you on this change as I think it will help to drive forward the changes here... |
ramonjd
force-pushed
the
try/global-styles-skip-block-gap-style-generation
branch
from
3ea2641
to
3b3d96e
Compare
|
Noting that there is a WIP alternative to this PR in the works: So I might put this one on ice for a bit. |
|
Thanks Ramon! Yes, I'm currently hacking away at #40875 to explore Riad's suggestion from this comment (#39870 (comment)) to see if it could be a viable alternative. The PR's a little broken right now but I'll continue playing around with it 🙂 |
…mpiling the styles. In layout.php, if there is no block-level value for blockGap, but a global styles value available for blockGap, use the latter.
In layout.php, move check for global style into gutenberg_render_layout_support_flag
- Before we were unsetting blockGap for block and elements from the merged $theme_json structure in sanitize(). - After we keep them in the merged $theme_json, but skip them when printing out the global stylesheet.
ramonjd
force-pushed
the
try/global-styles-skip-block-gap-style-generation
branch
from
3b3d96e
to
aac2cef
Compare
|
Closing in favour of #40875 |
What?
Use block global style value (not the root) for block gap where no gap value exists.
Resolves #39789
Why?
In #37360 we removed
blockGapfrom the block and elements arrays in$theme_json.The side-effect was that block gap values in global styles for blocks such as the Group block were neither saved in the editor or appear in global styles the frontend.
Because we were removing block gap global styles from the theme json tree it meant that we could never compile the styles and output them to the frontend.
As a temporary measure we've removed the block gap control UI from dimensions panel in #39845.
This PR reinstates it.
How?
See:
Testing Instruction
In the Site Editor, edit the global block gap value of a block that supports it, e.g., Group.
Check the editor and frontend to ensure the changes are visible.
Ensure that no
top levelstyles are overwritten, that is, any global block gap styles (--wp--style--block-gap) set at the root level in the body tag should persist.s
