Try reducing specificity of layout margin rules #47858
Conversation
|
Size Change: +735 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in c7d8c25. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4592857915
|
Update: I bumped the specificity of the block rule ever so slightly by prefacing it with |
There was a problem hiding this comment.
This is a very compelling exploration! Really cool that it appears to largely resolve the block-level-in-global styles margin case 👍
One challenge here, though, is the introduction of !important rules. From a quick test, it looks like the layout's !important rules for the first and last child blocks results in those rules overriding block-level-in-block-attributes margin rules:
In the above screenshot, this particular cover block has a margin-top value set to one of the spacing presets, and that style is output in the block's inline styles. However, the layout support's rule wins out:
|
Just noticed another couple of challenges with reducing the layout specificity / removing the bottom margin overrides and existing block styles:
So, in terms of evaluating whether we can reliably reduce the layout specificity, a couple of questions might be:
In any case, I think the ideas in this PR are super valuable to explore, particularly given that the complexity of the change (and style output) is much simpler than what's being explored over in #47399. Thanks again for digging in here! |
|
Thanks for thoroughly poking holes in this experiment @andrewserong 😄 I removed the
These are great questions! Regarding 1, I can go through the core blocks in this PR and try to lower specificity where needed. I'm pretty sure we can manage that. As for 2, it's hard to know what might be expected, but over time we've consistently had pointed complaints about the over-specificity of our styles, and I suspect we won't get much pushback on lowering them. It is a breaking change, and should be announced as such (dev notes etc.) but ultimately any change in specificity is a breaking change, and even just adding rules can break something somewhere. I'm optimistic that we can get away with it 😅 |
There was a problem hiding this comment.
Thanks for continuing down this particular path @tellthemachines, I still think it's a really compelling direction 👍
From giving this a re-test, I think there's another wrinkle with the update to the non-first-or-last rules, in that it looks like we still need to reset the margin-block-end for each of these rules to 0 so that we can override the user agent styles for elements like paragraph tags. Otherwise, it looks like the 1em rules creep back into play. Here's a screenshot:
I'd be curious to hear what other folks think about this as a potential solution, so I'll just copy in @t-hamano for visibility, too.
|
I re-added the bottom margin override and reduced the specificity of the Columns block margin rule. I think everything's working as it should now. Regarding a possible clash with #47952, I'm pretty sure we can make things work with the compound class added in that PR, because the rule as it is in this PR - I'm going to go ahead and mark this ready for review, but I'll hold off on updating the test markup for now because it's likely that #47952 will be merged soon, and I don't love solving merge conflicts that much 😂 |
tellthemachines
requested review from
MaggieCabrera,
apeatling,
carolinan and
ndiego
|
Thanks for the update @tellthemachines! From a quick search of blocks' CSS, here are some other blocks where the specificity may need to be reduced (or at least, we should test to see whether or not it'll need adjusting)
|
|
Maybe this is another example of this being the case? |
|
/cc @WordPress/block-themers for more eyes on this |
|
Thanks for the list of blocks with margins @andrewserong ! I lowered the specificity on File, Post Excerpt and Pullquote. Left and right-aligned images only seem to have inline margins, unless I'm missing something 🤔 and in my testing Post Template seems to be getting the right styles whichever position it's in inside Query. Regarding #45107 and similar uses of |
I don't think so. It would probably involve adding a bit of logic — currently I think |
I did a little bit of digging: it looks like there are top/bottom margin rules for a deprecated form of the image block that target |
tellthemachines
force-pushed
the
try/minimal-margin
branch
from
300cf63 to
6a4947b
Compare
|
Now that #47952 is merged, I've updated the rules in this PR to take those changes into account. I also updated the global styles output to reflect the changed rules and fixed the test markup. This is now ready for another round of review/testing! |
There was a problem hiding this comment.
This is testing really well for me so far @tellthemachines, and I think it's looking like the most promising direction for addressing the styling conflicts between margin and layout rules 👍
Just adding a few more pings for visibility now that this is ready for another round of review, as some of these folks have been involved in the discussions so far: @t-hamano, @aaronrobertshaw, @youknowriad, @WordPress/gutenberg-design
Will do some more detailed testing when I'm back on Tuesday!
|
I'd love a @youknowriad take on this. In principle, I'm a fan of using I haven't had a chance to deeply test this either, but from a glance it looks like it's changing the owl selector and the "stack" behavior. Is the net result the same? That is: it's so nice to not have to worry about the top margin of the first child, and the bottom margin of the last child, and have the margin behave like "gap", essentially. Ideally we can keep that behavior. Adjacent, I looked at using |
| @@ -2488,8 +2488,9 @@ public function get_root_layout_rules( $selector, $block_metadata ) { | |||
| $has_block_gap_support = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'blockGap' ) ) !== null; | |||
| if ( $has_block_gap_support ) { | |||
| $block_gap_value = static::get_property_value( $this->theme_json, array( 'styles', 'spacing', 'blockGap' ) ); | |||
| $css .= '.wp-site-blocks > * { margin-block-start: 0; margin-block-end: 0; }'; | |||
| $css .= ".wp-site-blocks > * + * { margin-block-start: $block_gap_value; }"; | |||
| $css .= ":where(.wp-site-blocks) > * { margin-block-start: $block_gap_value; }"; | |||
There was a problem hiding this comment.
It looks like this and the JS version are missing the margin-block-end: 0; rule, so the user agent stylesheet rules for paragraph are poking through:
There was a problem hiding this comment.
Oh whoops, I copied the old rule and forgot to add the end margin 😳
There was a problem hiding this comment.
Nice one, thanks for all the updates @tellthemachines and for persevering with this! All the issues I've raised are now resolved, and this is testing well:
✅ Re-tested with a wide range of blocks, and couldn't find any issues
✅ Spacing between post title and post content within the post editor now respects the margin rules for the post title block
✅ Root-level site block spacing works as expected, with rules correctly overridden by block margin rules where appropriate
✅ Block-level margin rules in global styles apply as expected in positions within the layout container
✅ Global styles rules are overridden correctly by block-level styles
I think this is in a great place to land now, and would be worth trying out in the plugin.
LGTM! ✨
ndiego
added
[Type] Bug
tellthemachines
added
Needs Dev Note
Dev noteChanges in CSS specificity for some layout typesThe CSS output for the This fixes the issue of global margin styles for specific blocks being overridden by layout styles (see #43404). |
What?
Hypothetical alternative to #47399: trying to reduce the specificity of the layout rules to the point that they don't override block global margins.
This assumes that in a layout container:
Why?
#43404, #44736, #33795, #40813, #35267 and #46902... the issues keep piling up. Essentially, layout spacing styles shouldn't override global block margins.
Testing Instructions
See #47399.
Testing Instructions for Keyboard
Screenshots or screencast