Avoid using CSS variables for block gap styles #37360
Conversation
youknowriad
added
Customization
Is this an acceptable tradeoff for now? I can't see how we could allow it while solving the conflicting issue. |
|
Size Change: +67 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
What's the best way to test it? As a quick superficial test, basics look okay to me:
I did notice a hiccup on the frontend: And an error in the nav block:
Can you expand on the consequences of this? Does that mean I won't be able to do this? |
|
This is a tough one. I really like the simplicity of the CSS variables, but I cannot think of an alternative solution to this PR other than even more CSS variables like #35454. But introduces other questions/potential issues. I will give this PR a thorough test today! |
I think it is acceptable, there is still the ability to add top margins if needed. |
|
@oandregal @jorgefilipecosta I'd appreciate some help on the two remaining todo list items here when you have time :) |
|
Thanks for kicking this off! 🙇 I took a look at the following blocks:
Social links and Buttons work pretty well out of the box!
I might be wrong, but I guess we'd have to modify the CSS for Columns and Navigation, which rely on the block style Columns is not yet a layout based block, so it might have to opt-in to Happy to help out here as this PR evolves. |
|
Indeed @ramonjd blocks that don't use the "layout support" won't be able to use block gap anymore. I think that's fine to be honest and we don't need to support per block block gap for these blocks right now but we can iterate on them to add layout support. It's also fine to leave the CSS variables in their styles if needed as the root CSS variables will still be generated from theme.json |
There was a problem hiding this comment.
Thanks for digging into this one @youknowriad!
I'm wondering if it's possible to reduce the scope of the change in this PR slightly, so that we can preserve the Columns block blockGap control in the shorter-term so that we can incrementally move to the layout support without dropping the per-block individual control.
Is the root of the problem in #36521 that the rendered Layout style sets for the block's children a value that references the CSS variable? E.g. $selector > * + * { margin-top: var( --wp--style--block-gap )? If so, I'm wondering if we can update gutenberg_get_layout_style so that it always uses a real value instead of referencing the CSS variable. I was thinking if around line 171 of layout.php, if we ensure that $gap_value is either the block's style.spacing.blockGap value, or the theme's root level style.spacing.blockGap value, then gutenberg_get_layout_style might never need to reference a CSS variable in its generated styles?
If we can get it to do that, then we might be able to safely retain the inline CSS variable rendering used by the Columns, Buttons and Navigation blocks.
In addition to the Columns styling, the Buttons block also still uses the CSS variable for button width calculation, so it might not be easy to remove the block-level CSS variable entirely. However, with the main margin styles that are applied to children no longer using the CSS variable, in theory it sounds like the conflict might then be resolved?
It's also fine to leave the CSS variables in their styles if needed as the root CSS variables will still be generated from theme.json
So long as blocks include CSS variables in their styles, I think this means we'll still need to render the inline CSS variable for the blocks to be styled correctly? Apologies if I'm missing some details here, though!
Unfortunately I don't think we can. Basically, we have to keep the root CSS variable as a fallback, otherwise there's no solution for theme.json to define a consistent gap across blocks and layouts (flex, flow...) The way columns work is that they have custom styles relying on the CSS variable and when the user changes the value of the gap, we update the CSS variable. The problem is that setting that CSS variable causes the parent (often root) container margins to break because of the There are some approaches to solve that for the columns block:
Both these solutions basically mean having custom logic for the block instead of relying on the block support. I think the work needed is probably bigger than refactoring the block to use the layout abstraction. |
The blocks shouldn't include CSS variables in their styles ideally. The blocks should be relying on the layout block support to generate the right styles for them: if they have a custom block gap value, it will generate styles with that value (no variable), if they don't have a value, it will generate a fallback to the root CSS variable. For blocks that don't use the layout abstraction yet, they can't have block gap support, they can use the CSS variable in their styles if they want to still consume the "root theme.json block gap" but custom block gap support is not possible. |
|
Thanks for the explanations @youknowriad 🙇
Without thinking too deeply about it, the adhoc CSS variable approach is very tempting as it might be neater than whatever we'd have to do to set and utilize the block gap value across various Columns Block elements. I suppose the fallback value would be the value of the root theme.json block gap (if it exists), e.g., from columns/style.scss: // Add space between the multiple columns. Themes can customize this if they wish to work differently.
// Only apply this beyond the mobile breakpoint, as there's only a single column on mobile.
&:nth-child(even) {
margin-left: var(--wp--style--columns-block-gap, var(--wp--style--block-gap, 2em));
}
Just checking, would that include the Columns block? It hasn't opted into |
|
I the our time is better spent moving the columns block to the layout abstraction instead of implementing a "custom block gap" support specific to a given block. Ideally we want consistency across all blocks. At least I believe we should give it a try and see how complex it would be, maybe it's not that hard? The CSS variables approach suffers from another problem (that I just noticed today in trunk for the general block gap), When you apply it, if you have nested columns blocks, the inner one will get the block gap applied to the outer one. |
Thanks for the explanations @youknowriad! You make very good points about consolidating on the block support providing the necessary styling instead of the individual block having its own ad hoc approach. This sounds like a good way for the block support to work to me, and a good direction forward for the API. My main concern is to ensure that we don't break or lose behaviour for the blocks we've already opted-in to the block gap support (as folks had been wanting gap support for columns and buttons for quite a while). But I'm very happy for us take your lead here and investigate how complex it'll be to switch over the Columns block to using the Layout support. Let's see how it goes! One other thing for us to keep in the back of our minds as we work on the Layout support is the outstanding issue in #35376 that the styles are not rendered over the REST API so won't render correctly in the post content block within the editor. We've gotten close a couple of times at coming up with a solution for it, but unfortunately recent attempts for a fix were blocked (for very valid reasons). I don't believe we're close to a fix for it (my most recent attempt is pretty messy and I'm not sure will be viable, either), so just wanted to flag it in case it winds up being a blocker. |
|
This PR breaks blocks that define a blockGap in their markup like so:
In my experience so far we've needed to do this mostly on row blocks where we want to reset the gap to zero, what would be the best way to do that? |
|
@MaggieCabrera conceptually, it shouldn't break that use case, maybe it's because the group block doesn't support blockGap officially yet which should happen after #37459 get merged. You can maybe a try a branch with both PRs to see if it still works. |
you are right and the markup does work on blocks that support it, thank you! |
Co-authored-by: André <583546+oandregal@users.noreply.github.com>
|
Just noting here that we might need to update a WordPress core test when/if this changeset makes it into the next version of WordPress. @fullofcaffeine found that equality checks on rendered blocks and fixtures in The short of it is that the fixture, which has static classnames, doesn't match the rendered block. Sniffing around, I see that some Gutenberg tests handle this via a string replace on the I'm not sure whether we should prepare core for this now, or (something I suspect is better) waiting until the next core update and dealing with the failing tests then. Gutenberg changes a lot so maybe we should fix it when the next update happens... assuming the code is still there 😄 |
|
It is a shame that since this change blocks can't set a gap in theme.json. I opened https://github.com/WordPress/wordpress-develop/pull/2307/files to see if we could find a way round that... cc @youknowriad |
|
@scruffian unfortunately, it's done on purpose, if you allow tweaks (like your PR does), it's going to recreate the issues fixed by this PR. Basically, The wrong "margin-top" can be applied to a social-links block (or a buttons block) if you tweak its gap using theme.json. Instead of using the margin top of the default gap, it will use the one specific to that block. |
This relates to a change in the Columns block that added layout support. See WordPress/gutenberg#37360 (comment). Blocks that opt into layout support are gifted generated classnames of wp_unique_id( 'wp-container-' ). One possible approach is to update test_do_block_output in Tests_Blocks_Render to swap the id for something we expect. It's how Gutenberg approaches it when testing generated classnames in elements support.
This relates to a change in the Columns block that added layout support. See WordPress/gutenberg#37360 (comment). Blocks that opt into layout support are gifted generated classnames of wp_unique_id( 'wp-container-' ). One possible approach is to update test_do_block_output in Tests_Blocks_Render to swap the id for something we expect. It's how Gutenberg approaches it when testing generated classnames in elements support.
Closes #36521
This PR removes the block gap style CSS variable because of the conflicts it creates. We can still keep the variable but only for the "root level" theme.json style config.
Todo