Layout: Add layout aware margin styles output for blocks in global styles #47399
Conversation
andrewserong
added
[Type] Bug
|
For visibility, since I know a few folks have taken an interest in fixing this issue, just thought I'd ping a few people: @tellthemachines @aaronrobertshaw @t-hamano This appears to be working pretty well for me so far, but I've only just finished hacking it together so I might have very well missed edge cases, etc. I'll do any tidying up that might be needed and aim to switch this over to ready for review by the end of the week (I'm AFK tomorrow). |
|
Size Change: +20 kB (+2%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Thank you for your great work! I will try this PR within a couple of days, but this PR will also fundamentally solve #47059 👍 |
There was a problem hiding this comment.
This is working as expected with both block and classic themes ✅
Mostly questions below, and a further bonus question 😅 : what should happen if Group block has a gap value set in Global Styles? Currently it's overriding the Global Styles-set margin of a child Heading block:
| if ( ! block_has_support( $block_type, array( '__experimentalLayout' ), false ) ) { | ||
| if ( | ||
| ! block_has_support( $block_type, array( '__experimentalLayout' ), false ) && | ||
| ! block_has_support( $block_type, array( 'spacing', 'margin' ), false ) |
There was a problem hiding this comment.
Trying to wrap my head around what this condition is doing - if layout is not supported, when does it matter whether margin is supported or not?
There was a problem hiding this comment.
Ah, I see the confusion here, I should add a comment to clarify.
This check is about what the current block supports rather than what the theme supports, so to support layout aware margins for blocks without layout support (e.g. heading, paragraph, image) we need to allow it to pass at this point. The check is primarily an early return so that for blocks that we know don't need any handling, we don't need to do any further lookups to theme settings, etc. Previously, that was just blocks that didn't support layout, but we now need the check to be layout + margin.
Further down in the function, we check $has_block_gap_support (that is, the theme has block gap support) before outputting the layout aware margin rules.
There was a problem hiding this comment.
Got it, thanks for clarifying!
lib/theme.json
Outdated
| @@ -246,7 +246,8 @@ | |||
| "margin-block-end": "0" | |||
| } | |||
| } | |||
| ] | |||
| ], | |||
| "marginSelector": " > " | |||
There was a problem hiding this comment.
I guess the benefit of declaring the selector here is having a single source of truth between editor and front end code. If this becomes unnecessary though (say we find a better solution with cascade layers) how easily can we remove it?
There was a problem hiding this comment.
Yeah, the benefit was to try to keep the selector in one place. It should be easy enough to remove from theme.json since it's an undocumented part of the layout definitions object, which isn't intended to be overridden by themes. I think we'd just need to land the changes to the PHP / JS code that uses a different approach first, and then as a final step, remove this property in a follow-up.
That said, if it seems like something we'd likely want to change in the shorter-term, I suppose we could hard-code it for now. But in general I kind of like the idea of trying to keep the definitions in one place where possible, for clarity... but that may or may not be all that valuable 🤔
|
Thanks for taking this for a spin @tellthemachines!
Oh, that's a great question. I think I might need to ping a designer for ideal feedback on this one, as it's a bit more of a UX / user expectations concern and I could see the decision going either way. @jasmussen / @WordPress/gutenberg-design — if anyone has input here on the expected behaviour here, we'd love some feedback. To recap:
|
|
Update: I've pushed a small change (72d0afe) to ensure that children of the root I've switched this over to ready for review now — I think it's mostly working aside from figuring out which should win between block-level block spacing in global styles and block-level margin in global styles. I'll see if I have any fresh ideas after the weekend and continue on with this PR on Monday 🤔 — very happy for feedback on whether folks think it's a blocker, or if there's a clever further tweak we can make to the output rules. |
|
Flaky tests detected in 1fa9c3b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4249382021
|
Very tricky to parse the nuance here, thanks for trying to summarize, but forgive me if I'm still missing a beat. My initial instinct was that the ancestor wins, and child loses, mostly on CSS principle. But for this to work the rules of the ancestor should be intentionally added by a user. I.e.:
Is that a correct use case and would it work this way? Pros/cons to this approach? |
|
What I have noticed is that when margin is set at the block level, the top margin of the first block in the container and the bottom margin of the last block in the container take precedence. My feeling is that these two margin styles should not apply.
For example, how about applying only the margin-top to the second and subsequent blocks in a container, as shown below? /* From */
.is-layout-constrained > .wp-block-heading {
margin-top: 50px;
margin-bottom: 50px;
}
/* To */
.is-layout-constrained > * + .wp-block-heading {
margin-top: 50px;
} |
|
Thanks for the feedback, folks!
@jasmussen I think you're getting it there — the trick is that by wrapping the heading block in a group block, the group block's block spacing value (if set in global styles) will unfortunately override the heading's global styles margin value. I'm not sure if we have (yet) a good way of figuring out how to apply that, so I'm still trying to think about how we might fix it 🤔 Also, apologies, it is a difficult issue to describe, thank you for taking the time to look at this!
@t-hamano thanks for the feedback, there — I was a little on the fence about whether to only apply |
Oh it does this by default and even without any margin/spacing customization? Yes tricky indeed. |
Not entirely by default; a value has to be set for the Group block spacing in Global Styles. So it's one Global Styles customisation overriding another. I'm ok with landing this as-is, because the benefit of having margins working for all blocks inside layout containers is already huge, and we can fine-tune some of the detail later. @andrewserong is there anything else you feel needs improving here? Otherwise I'm happy to give it a final check. |
I agree, thanks @tellthemachines — from my perspective, the change in this PR is probably desirable in that it isn't making too many assumptions about how the layout rules should be applied, it's mostly dealing with the priority issue. Since it's potentially contentious as to whether or not the styling rules here are the right way to go (@t-hamano has some very valid feedback about potentially different rules), I'd be keen to make sure that there's a bit more visibility on the approach here, so I'll add a few more reviewers / pings in case there are other folks who might have an opinion on it. CC: @WordPress/gutenberg-core |
andrewserong
requested review from
scruffian,
MaggieCabrera and
aaronrobertshaw
|
Is this intended to only work in block themes? |
Yes, since it's only for sites that opt in to blockGap and set margin rules at the block level in global styles. |
|
Thanks for all the hard work on solving the layout vs margin styles issue @andrewserong 🙇 I've taken this PR for a quick spin but I'm seeing differences between the post editor and the frontend. Steps to replicate:
This is what I'm seeing: Screen.Recording.2023-02-01.at.10.41.23.am.mp4 |
|
Please know that in 6.2, classic themes are already able to opt-in to appearance tools, including block gap. I have already suggested that we may need to revert it and update the theme support without including block gap, but if we need to do that, an early decision is better. |
|
Thanks for digging in @aaronrobertshaw and @t-hamano, much appreciated!
I'll have a play around as to why it's looking different, I wonder if the margin collapse of the subtle difference in styling in the block editor has anything to do with it... this is potentially related to:
Ah, our good friend margin collapse! 😅 |
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
andrewserong
force-pushed
the
add/layout-aware-margin-styles-for-block-level-global-styles
branch
from
c3b19ed
to
3604c0f
Compare
|
Quick update: I haven't forgotten about this one, and I'll continue chipping away at it next week. I'm still noticing some odd differences between the site editor and the site frontend that I suspect could be partly to do with how the global styles get processed and output, rather than only being to do with the rule we're generating. Mostly I'm trying to diagnose that a cover block as first child of a group block, with top margins set in global styles, results in the cover block's margin winning in the site editor, but not on the site frontend.
Inspecting the generated rules, the selector generated is
So, it looks like the output in the site editor will need to be tweaked slightly, I think... I'm not too sure which direction it needs to be tweaked just yet, but I'll continue to play around with this next week. Also, for anyone reading this, don't worry if my comments here don't make any sense! As it's the end of my week, I mostly wanted to jot down my thoughts while they were fresh in my mind, so I know where to pick up again next week 🙂 |
|
I'm not entirely sure if the following is useful to the conversation. But I wanted to nevertheless surface this codepen. Essentially what it does is use |
Not quite since we're not really looking at the block-level margin output in that kind of way (thank you for sharing, though, always great to see some practical examples in working code!). But it has spurred a thought that we could potentially expand the margin styles output here to also include rules for the first and last items, to parallel the block spacing rules. I'll have a play with that tomorrow to see if that helps the reliability of the output. 🤔 |
|
Update: I've had a go at another approach that adds some extra complexity, but appears to help introduce better parity between the site editor and the site frontend. I'm not sure if I'm sold on the approach, but I think the exploration was worth it, at least! I'll recap the ideas explored in b536418:
The styles that get output now look like the following (assuming a Group block set to margin top of .is-layout-flow > .wp-block-group { // First block sets top margin to 0, pass in real value for bottom margin.
margin-block-start: 0;
margin-bottom: 50px;
}
.is-layout-flow > * + .wp-block-group { // In-between blocks get to output real values for both top and bottom.
margin-top: 25px;
margin-bottom: 50px;
}
.is-layout-flow > *:last-child.wp-block-group { // Last block sets bottom margin to 0, pass in real value for top margin.
margin-top: 25px;
margin-block-end: 0;
}
.wp-site-blocks > * + .wp-block-group { // Root blocks rules (might also need the above logic)
margin-top:25px;
margin-bottom:50px;
}From playing with this locally, it looks like we can now get close to parity between the site editor and site frontend:
Notes / caveats:
Very happy for any feedback on this approach, and whether or not this approach is overly complicating things. I'm cautiously optimistic that it's something that gets it working, but what I'm not sure about is whether it's worth the extra complexity. If so, I'll look into seeing if we can simplify the code a bit / consolidate things where possible. |
|
Thank you for your great initiative, @andrewserong! 🎉 I used the following data and confirmed that it is working exactly as expected: theme.json{
"$schema": "https://schemas.wp.org/trunk/theme.json",
"version": 2,
"settings": {
"appearanceTools": true,
"layout": {
"contentSize": "840px",
"wideSize": "1100px"
}
},
"styles": {
"blocks": {
"core/paragraph": {
"spacing": {
"margin": {
"top": "1.8em"
}
}
},
"core/heading": {
"spacing": {
"margin": {
"top": "30px",
"bottom": "29px"
}
}
}
},
"elements": {
"h2": {
"spacing": {
"margin": {
"top": "40px",
"bottom": "39px"
}
}
}
}
}
}Simgle page markup<!-- wp:paragraph -->
<p>Root Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Root Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Root Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Root Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Root Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Root Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Root Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Group > Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Group > Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Group > Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>Group > Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Group > Group > Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Group > Group > Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Group > Group > Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:cover {"customOverlayColor":"#d9d4d4","isDark":false} -->
<div class="wp-block-cover is-light"><span aria-hidden="true" class="wp-block-cover__background has-background-dim-100 has-background-dim" style="background-color:#d9d4d4"></span><div class="wp-block-cover__inner-container"><!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Group > Cover > Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Cover > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Group > Cover > Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Cover > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Group > Cover > Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Group > Cover > Paragraph</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->
<!-- wp:group {"layout":{"type":"flex","flexWrap":"nowrap"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>Row > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Row > Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Row > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Row > Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Row > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Row > Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Row > Paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"layout":{"type":"flex","orientation":"vertical"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>Stack > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Stack > Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Stack > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Stack > Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Stack > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Stack > Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Stack > Paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:cover {"customOverlayColor":"#d9d4d4","isDark":false} -->
<div class="wp-block-cover is-light"><span aria-hidden="true" class="wp-block-cover__background has-background-dim-100 has-background-dim" style="background-color:#d9d4d4"></span><div class="wp-block-cover__inner-container"><!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Cover > Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Cover > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Cover > Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Cover > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Cover > Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Cover > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:paragraph -->
<p>Cover > Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":1} -->
<h1 class="wp-block-heading">Cover > Group > Heading H1</h1>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Cover > Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading -->
<h2 class="wp-block-heading">Cover > Group > Heading H2</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Cover > Group > Paragraph</p>
<!-- /wp:paragraph -->
<!-- wp:heading {"level":3} -->
<h3 class="wp-block-heading">Cover > Group > Heading H3</h3>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>Cover > Group > Paragraph</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div></div>
<!-- /wp:cover -->Screenshot
These margin rules don't apply to blocks within a cover block, but I think they are not covered by this PR and are expected behavior for now. One thing I noticed is that the element-level margins are not applied: {
"$schema": "https://schemas.wp.org/trunk/theme.json",
"version": 2,
"styles": {
"elements": {
"h2": {
"spacing": {
"margin": {
"top": "40px",
"bottom": "39px"
}
}
}
}
}
}What do you think about element-level margins? |
There was a problem hiding this comment.
Thanks for persisting with this! It's a tough problem to get right.
Block margins inside blocks with layout are now working well with default gap values ✅ but there's still a difference between editor and front end when Group has a block spacing value set in global styles:
Editor:
Front end:
This seems to be due to the order in which core block styles are output, because rule specificity is the same in both cases.
Did we reach a decision on which style should take precedence in this case? (I feel it should be the child block's style as that would be more consistent with the behaviour we're trying for in this PR.) In any case, I wonder if it would be possible to fiddle with specificity a bit further so we're not dependent on order of output?
One thing I don't love is the amount of extra CSS this change adds, especially because it gets added to all blocks that set margins, regardless of if they will ever be inside a layout-enabled block or not. For instance, I'm seeing these styles output for the Comments Title block, that can only ever exist inside a Comments block, which doesn't have layout.
There's sufficient repetition in styles that it shouldn't be too much of a performance hit when gzipped but it's still not optimal. I'm afraid don't have any better ideas at the moment 😅 but it may be worth thinking on this a bit further.
|
Thanks for reviewing and testing @t-hamano and @tellthemachines!
The excessive CSS rules are my main concern here, too, but I haven't been able to think of a stable alternative. Very happy to pursue any ideas that folks have, though! Since it's proving to be a tricky issue to fix, my main objective right now is to follow the question, "What code changes (however inefficient and verbose) get us to a working state?". The hope is that if we can get it technically working, the path toward optimising the output might then become a littler clearer 🤞 With that in mind and based on the helpful feedback here, I'll next explore the following:
|
Sounds sensible! If nothing else, this work is allowing us to clarify what the desired state of things actually is. |
… to children of site blocks
|
Just dusting this one off a little to continue playing with consolidating some of the PHP logic. I imagine we might wind up going with something like #47858 in the end, but thought it still worthwhile to see where the ideas in this PR might lead. I'll continue on with this tomorrow. |
What?
The idea in this PR is to add additional output for margin styles in themes that opt-in to blockGap, to introduce layout aware margin rules that are particular to the layout types. Since the existing flow and constrained layouts borrow their margin/spacing logic from the idea of the Stack layout as described in Every Layout book, the approach in this PR is inspired by the idea of exceptions, by outputting additional margin rules. Where the approach in this PR differs is that it uses direct values as output, rather than CSS variables.
So, in a block theme that opts-in to block gap, if a user or theme sets margin styles on the Group block, then this PR will output additional rules. Example output:
Why?
As described in #43404, the existing layout rules currently override margin rules set at the block level in global styles. A previous attempt looked at trying to reduce the specificity of layout styles (#45927), however this approach did not result in the desired outcome. The specificity of layout styles was reduced, however it then resulted in the layout and margin rules having the same level of specificity (010), which meant that if the layout styles were output after the margin styles, the layout styles still won out.
So, the argument in this PR is that it would be better to be explicit about having block-level margin rules that intentionally have a higher specificity than layout rules, however lower specificity than layout rules set at the individual block level. The result is:
.is-layout-flow * + *(010) — base layout styles.is-layout-flow > .wp-block-group(020) — layout aware margin styles outputHow?
marginSelectorproperty to the layout definitions intheme.json.marginSelectorto join the layout classname with the block's classname.Testing Instructions
nullintheme.jsonor if running a Classic theme)A question — do we also need to output these extra rules for the
flexlayout? I don't think so, since it usesgaprather than vertical margins, but it might be worth double checking.Testing Instructions for Keyboard
Screenshots or screencast