Try reducing specificity of layout style selectors #60228
Conversation
tellthemachines
added
[Type] Enhancement
tellthemachines
requested review from
MaggieCabrera,
ramonjd,
carolinan,
andrewserong and
aaronrobertshaw
|
Size Change: -1.49 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Ok folks, I think I've addressed all the issues that had popped up in #57841, so this is ready for some testing! |
|
FWIW I've begun testing this and it's looking good so far ✨ Unfortunately, I've run out of time and only gotten to reviewing the code and testing mostly standard blocks in a block theme. If nobody else picks this up in the meantime, I'll finish off reviewing it when I'm back online next week. So far: ✅ Code changes here almost match 1:1 with the original PR except for a few intentional deviations The blocks I've primarily tested with include:
Those I haven't yet looked at are: Query, Post Content/Template, and pagination blocks.
I think you should be right to update the tests. I'm happy to share the load and update the tests further down the line if it turns out we need to. |
|
Thanks for testing @aaronrobertshaw!
I don't think it's a valuable use of time to update those tests before the changes are final. The only thing they flag is changes to the styles output, which we already know are happening. They're useless in indicating any actual breakage as a result of the changes, because breakage is purely visual. The other problem with updating early is that those test strings change often, so the likelihood of merge conflicts increases quite a bit after editing those files. |
Your call, my thinking was these changes already had a fair bit of testing via #57841 and we could have some confidence there that this could land quickly. |
I hope so! It would be good to get a bit more testing on it though, and many of us are heading into a long weekend, so I'd rather be cautious with the expectation 😅 |
There was a problem hiding this comment.
I've had a chance to give this some further testing. This time across both classic and block themes.
Block themes included: 2024, 2023, 2022, and emptytheme
Classic themes included: 2021, 2020, 2019, and emptyhybrid
I primarily focused on the same set of blocks from my previous testing that adopt layout or blockGap support but did also quickly smoke test other blocks at random.
The main blocks tested with included; Cover, Column/s, Social Links, Details, Navigation, Gallery, Quote, Group/Row/Stack, and Buttons.
For the vast majority of cases, this all tested well as per the test instructions.
There did seem to be a couple of theme-specific issues though.
1. TwentyTwentyOne
The Row and Stack blocks display differently between the editor and frontend on this branch. It looks like TT1 adds a .wp-block-group { display: flow-root } style. With the current style load order and specificity it is taking precedence over .is-layout-flex.
2. TwentyTwenty
TwentyTwenty adds some .editor-styles-wrapper scoped styles which reset margins for the Gallery and Quote blocks. On trunk, the higher specificity allows the layout styles to take precedence, however, they lose out on this branch.
3. TwentyTwentyOne & EmptyHybrid
The navigation block's layout controls for justification don't appear to work on trunk or this PR. So just something I thought worth noting.
Do we have an easy means to get a ballpark assessment of how many themes could have styles that may conflict with the reduced specificity here? Or a way to reach out to impacted theme authors like we sometimes do with plugin authors?
All in all, if we can manage the theme-specific issues, I feel like this is pretty close.
That said, some extra eyes and additional reviews would be very beneficial as this one seems like it could be easy to have missed something 🙏
|
Tested lots of patterns across some core themes. The general trend seems to be that really old classic themes are good, new block themes are good, the in between themes (post block editor but pre block themes) have some issues. I'll also try to test some third party themes (open to recommendations). Here are the issues I spotted:
|
|
Thanks for testing @aaronrobertshaw and @talldan! I couldn't find all the patterns mentioned (e.g. Project Description) in Twenty Twenty or Twenty Twenty One but I tested with the ones I found and made some adjustments. The base layout rule (the one that sets the
I think that's actually as it should be! #60489 removes the |
|
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. |
I can only reproduce this if the Navigation block contains a Page List with only one or two short blocks in it. Page List is getting a It could be fixed by adding an override in the Page List editor styles but it's not ideal to so when the issue is specific to classic themes. It could also be fixed by updating the Navigation block to follow other layout blocks in adding layout classes to the inner wrapper instead of the outer. That's a bigger piece of work though. Edit: actually, it happens with the Search block too. I think moving the layout classes is the best fix for this, if we can do it without upsetting all the layers of custom Nav block behaviour 😅 |
tellthemachines
force-pushed
the
try/reduce-layout-specificity
branch
from
bbf8ffd to
0776387
Compare
|
OK folks I think this is ready for another round! @aaronrobertshaw and @talldan, if you could confirm the issues you found are fixed perhaps I'll move on to the test string updates so we can get this merged soonish. |
|
I've been testing themes 2013-2024, just with Group and Column blocks so far. Switching between trunk and this branch for each theme I can't (yet) see any regressions. Haven't looked at other blocks. Here's my test HTML<!-- wp:columns {"style":{"spacing":{"margin":{"top":"var:preset|spacing|60","bottom":"var:preset|spacing|60"}}}} -->
<div class="wp-block-columns" style="margin-top:var(--wp--preset--spacing--60);margin-bottom:var(--wp--preset--spacing--60)"><!-- wp:column {"style":{"spacing":{"blockGap":"var:preset|spacing|50"}}} -->
<div class="wp-block-column"><!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"},"blockGap":"var:preset|spacing|80","margin":{"top":"var:preset|spacing|60"}}},"layout":{"type":"flex","orientation":"vertical","justifyContent":"stretch"}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;margin-top:var(--wp--preset--spacing--60);padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:group {"style":{"color":{"background":"#f0e9e9"}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background" style="background-color:#f0e9e9"><!-- wp:paragraph -->
<p>Inner Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#f0e9e9"}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background" style="background-color:#f0e9e9"><!-- wp:paragraph -->
<p>Inner Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#f0e9e9"}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background" style="background-color:#f0e9e9"><!-- wp:paragraph -->
<p>Inner Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"}}},"layout":{"type":"flex","flexWrap":"nowrap"}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:paragraph {"style":{"color":{"background":"#f27c84"}}} -->
<p class="has-background" style="background-color:#f27c84">Row</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"color":{"background":"#f27c84"}}} -->
<p class="has-background" style="background-color:#f27c84">Row</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"color":{"background":"#f27c84"}}} -->
<p class="has-background" style="background-color:#f27c84">Row</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"},"blockGap":"var:preset|spacing|50"}},"layout":{"type":"flex","flexWrap":"nowrap","justifyContent":"center","orientation":"vertical"}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:paragraph {"style":{"color":{"background":"#ffdf50"}}} -->
<p class="has-background" style="background-color:#ffdf50">Stack</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"color":{"background":"#ffdf50"}}} -->
<p class="has-background" style="background-color:#ffdf50">Stack</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"color":{"background":"#ffdf50"}}} -->
<p class="has-background" style="background-color:#ffdf50">Stack</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"},"blockGap":"var:preset|spacing|60"}},"layout":{"type":"grid","columnCount":2,"minimumColumnWidth":null}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:paragraph {"style":{"color":{"background":"#a0ff9e"}}} -->
<p class="has-background" style="background-color:#a0ff9e">Grid</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"color":{"background":"#a0ff9e"}}} -->
<p class="has-background" style="background-color:#a0ff9e">Grid</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"color":{"background":"#a0ff9e"}}} -->
<p class="has-background" style="background-color:#a0ff9e">Grid</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"style":{"color":{"background":"#a0ff9e"}}} -->
<p class="has-background" style="background-color:#a0ff9e">Grid</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:column -->
<!-- wp:column {"width":"316px","style":{"color":{"background":"#dfc8c8"}},"layout":{"type":"constrained","contentSize":"158px","wideSize":"278px","justifyContent":"right"}} -->
<div class="wp-block-column has-background" style="background-color:#dfc8c8;flex-basis:316px"><!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:paragraph -->
<p>Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:paragraph -->
<p>Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:paragraph -->
<p>Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
<!-- wp:group {"style":{"color":{"background":"#daa0f0"},"spacing":{"padding":{"top":"var:preset|spacing|10","bottom":"var:preset|spacing|10","left":"var:preset|spacing|10","right":"var:preset|spacing|10"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background" style="background-color:#daa0f0;padding-top:var(--wp--preset--spacing--10);padding-right:var(--wp--preset--spacing--10);padding-bottom:var(--wp--preset--spacing--10);padding-left:var(--wp--preset--spacing--10)"><!-- wp:paragraph -->
<p>Group</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:column --></div>
<!-- /wp:columns --> |
I've retested with the layout blocks across 2020 - 2024 and the issues noticed previously with 2021 have been resolved. As for the other issues I noted, the nav block issue was out of scope for this PR, but I'd like a plan to address/fix the 2020 issue flagged.
I'm not sure I agree 100% on this as before #60154 broke the styles, this PR was breaking them too. Or, am I misunderstanding this comment? If the considered decision is to dismiss the concern, so be it.
True, but the reality is they are. Do we need to wait on the results of that ticket or are you happy if some themes break in the meantime? |
I don't believe so, and reverting #60154 on this branch shows Gallery and Quote looking as expected in the editor. I think it's best to fix that separately; I'm hoping we can re-add specificity to the selector without breaking anything else.
No, we'd probably better patch it in the meantime. But our docs explicitly state that "...you should not target any of the editor class names directly" so it's not great that default themes are being inconsistent with that recommendation 😬 |
That's fair enough 👍 My concern was in part, whether the earlier issue (introduced in #60154) might result in other issues in this PR being hidden when comparing back and forth between trunk and this branch.
Agreed, "do as we say, not as we do" doesn't help anyone much!
Sounds good. So long as we have a plan to address it, we hopefully won't be breaking any trust with theme authors 🤞 |
|
Ok I just put up a fix for the Twenty Twenty issue in #60802. |
There was a problem hiding this comment.
Thanks for the patience here and the quick fix @tellthemachines 👍
I've given this another run after rebasing on trunk and it's looking good.
Block Themes (tested with 2024 & 2023)
- Didn't spot any new issues across both editors or frontend
- Global styles worked as expected
- Block instance styles related to spacing supports apply correctly
Classic Themes (2020, 2019, Neve, OceanWp, emptytheme )
- Neve: There were some margin differences between the editor and frontend but these were also present on trunk and before Fix horizontal flex layout in classic themes. #60154.
- OceanWP: Similar top level margin differences between editor and frontend as Neve. Also on trunk.
- 2019: Didn't centre content but was the same between trunk and this PR
Hybrid Theme (emptyHybrid)
- Didn't spot any issues
Once the tests are updated, this should be good to go 🚀
tellthemachines
force-pushed
the
try/reduce-layout-specificity
branch
from
0776387 to
4b5ea07
Compare
I haven't done the PHP ones yet! Just the JS. I should finish updating them all tomorrow. |
Apologies, I only read the last commit message and thought I could quickly sneak in an approval before finishing up. |
|
Ok all the test strings are updated now! |
What?
Final part of #57841.
Reduces specificity of the layout selectors:
display: flex):where().wp-container-core-group-is-layout-10, as well as the selectors that apply margin overrides such as:where(.wp-block-group-is-layout-constrained) > :first-child)I'll leave updating the test strings to after there's been at least one round of testing and we're reasonably sure of the changes because it's such a pain to do.
Testing Instructions
There are likely edge cases I'm missing here. If you can think of any, please test them 🙏
Testing Instructions for Keyboard
Screenshots or screencast