Try reducing specificity of global styles block selector. #57841
Conversation
tellthemachines
added
[Type] Enhancement
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/layout.php ❔ lib/class-wp-theme-json-gutenberg.php ❔ phpunit/block-supports/layout-test.php ❔ phpunit/class-wp-theme-json-test.php |
| @@ -991,7 +991,7 @@ protected static function get_blocks_metadata() { | |||
| } | |||
|
|
|||
| foreach ( $blocks as $block_name => $block_type ) { | |||
| $root_selector = wp_get_block_css_selector( $block_type ); | |||
| $root_selector = ':where(' . wp_get_block_css_selector( $block_type ) . ')'; | |||
There was a problem hiding this comment.
This approach would need to be extended to where all the feature selectors are generated e.g. an image block's border styles.
|
Size Change: -1.7 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Update: reducing the specificity of global styles is contingent on changing the order the stylesheets are loaded in. I have a PR in core for that: WordPress/wordpress-develop#5892 which fixes that problem but it needs a bit of work as it's currently breaking editor chrome styles 😬 |
|
Flaky tests detected in 2fd08c2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7969647568
|
tellthemachines
requested review from
spacedmonkey,
ellatrix and
ajitbohra
as code owners
|
OK marking this ready for review as I think the changes are pretty stable right now. Any and all feedback and testing is immensely appreciated! Especially from @WordPress/theme-team but everyone else too ❤️ |
There was a problem hiding this comment.
Great work on this @tellthemachines 👍
I'll give this a proper test on Monday. In the meantime, I tried cherry picking these updates into an alternate approach on the extended block style variations and noticed root layout styles were overriding layout styles on individual block instances. I can replicate the same behaviour on this branch.
This seems to affect the frontend worse as in the editor the .editor-styles-wrapper on the block instance styles bumps its specificity enough to take precedence and look correct.
| Editor | Frontend |
|---|---|
 |
 |
The above issue is related to this change. I didn't get a chance to dig into where the root body .is-layout-flex selector is added to see if that can be lowered as well but I'll take a look next week and I can help out more.
Also, I think the feature level selectors defined by the block selectors API still need a reduction as noted earlier. I guess we could wrap them when generating feature level declarations or they could be done on bulk in get_block_selectors.
FWIW this overall reduction really does seem promising and unlocks #57908. After a quick test it allows nested applications of block style variations which would be a big win!
|
Ok I've bumped the specificity of all base layout rules down to zero. This may need a little fine tuning if we think we should ensure that properties such as |
|
PInging @ddryo, @cbirdsong and @eric-michel, based on previous discussions I wanted to alert you of this change. |
Nice. I can confirm this solves the layout issue I noted in my last review 👍 I'm still working through testing all the edge cases and seeing how this plays with the extension of block style variations but have noticed at least one issue with the zero specificity rules. Using TT3 I'm seeing a different order of styles between the editor and frontend. In the editor, the correct general link color is being applied, as defined for the post content block. On the frontend though it's being overridden. Editor: Frontend: I'll try and dig into this tomorrow. On the surface it looks as though the element style node is being processed after block type elements. If it is related how the theme.json stylesheet is generated, I wonder if it would be beneficial to sort of collect the different nodes into categories and then generate style declarations in a more deliberate and clear order. Maybe even following the order of specificity defined in some form of documentation as proposed in this comment. |
|
Theme: Negai. On theme activation: CSS on the front: CSS in the editor. When changing the font size of the site title in the Site Editor > Styles sidebar > Blocks > Site Title Front: Editor: When changing the size on the individual site title block, using the option in the block sidebar. |
|
Thanks for testing this folks!
No, this is the issue that prompted reordering the styles in core. There's nothing wrong with the style generation itself; the problem is that element styles are part of base global styles (because they're not block-specific), and the order of style enqueuing in core is currently wrong because base global styles are being added to the document after block-specific global styles, when it should be the other way around.
These issues are also a result of the incorrect stylesheet order in core. Note that a global block style declared in theme.json and one added through the global styles sidebar have the same specificity, so essentially it's the same issue. What is happening is that, because element styles are loaded after block-specific global styles, they're taking precedence now that both have the same (zero) specificity. If these are the only issues with this PR 🤞 then things are looking pretty good! Because they will be fixed by the core PR linked above. |
There was a problem hiding this comment.
Very cool exploration!
What will this mean for the CSS that's included with the blocks themselves? I haven't looked through in any exhaustive way, but I see the Cover block has a default padding of 1em which gets overridden by global styles on trunk, but with this PR applied, the global styles' padding for the Cover block now has a lower specificity that the CSS rules for that block:
Trunk
The global styles padding for the Cover block overrides the block's CSS that ships alongside the block.
This PR
The block's own CSS wins.
Will blocks' CSS rules need to be lowered? And for 3rd party blocks that might use existing block supports like padding, margin, etc, would they also need to know to reduce specificity in any of their own stylesheets in order to support global styles 🤔
One thing on the back of mind while reading through is thinking through the min-height: unset rule I'm exploring adding as part of the aspect ratio support PR over in #56897. For that PR to work in conjunction with this one, I think I'd need to reduce the specificity of the Cover block's min-height rule here:
I think that'd probably be fine (and would be a simple code change as part of #56897), but just wondered if it's the kind of thing that could affect more blocks or potential future block supports?
I can confirm this one also.
So if these changes require an update in core that would mean they need to be reflected in Gutenberg as well or is this blocked on BC grounds until those core updates are in the minimum version of WP supported by Gutenberg? I might be missing something but it appears like the core patch could be brought into Gutenberg.
|
|
Thanks for the feedback folks!
Yes and probably also yes 😄 . The block-library styles have been progressively losing specificity, and I think that trend should continue as they are essentially core block global styles declared in a legacy manner. Third party block developers may need to reduce specificity in their styles if they're counting on global styles overriding their block ones (not sure how common that scenario is? But yeah this will have to be advertised as a potentially breaking change)
I was planning to do a similar PR for Gutenberg, but I can try adding the changes to this branch instead if it helps for testing purposes! In terms of priority, I believe the work should be done core-first because it affects the order styles are loaded in core so its effects might not be fully reproducible in Gutenberg. Another consideration is that this PR shouldn't land in core before that core PR is committed, or we'll see breakage along the lines of those issues already noted above. Given that we're close to Beta and package updates are starting to happen, I'd like to be 100% sure the change in core is viable before going ahead with this one. |
That'd be great thanks. It should also help with testing the extended block style variations and getting more eyes on this PR. It could still be a separate PR if you'd prefer to just make this one based on it.
No arguments here. I agree also with wanting to make sure the approach is viable in core before landing this.
This shouldn't land at all until the changes to the order styles are loaded are also reflected in Gutenberg. Perhaps that's another reason to bring the changes into this PR to make sure of that? |
aaronrobertshaw
force-pushed
the
try/reduce-global-styles-specificity
branch
from
122fd65
to
026da63
Compare
|
Thanks for fixing the tests @aaronrobertshaw ! I've updated with the style loading changes from the core PR so this should be a bit easier to test now. |
|
Thanks for testing @carolinan!
This PR is not aiming for 6.5, but for 6.6. The goal is to merge early in the cycle, so we have plenty of time to detect and address any resulting breakage before the next release.
This is an interesting one. It happens because the core layout styles in this PR are no longer specific enough to override the max-width defined for the shortcode in core. The shortcode block just renders whatever is passed into it, with no wrapper, so there's no way of forcing its contents to adjust to content width. The only solution I can see to this is bumping the specificity of that layout rule up again. I think that could be done for the single rule without having to adjust specificity for any other rules, so it's worth a try. I did wonder how prevalent use of the core audio shortcode might be in the block editor when there's also an Audio block available, but the Audio block has no styling options (due to using the HTML 
(core shortcode on top, Audio block below) so I imagine there are still legitimate reasons to use the shortcode 😂 |
|
It would have been helpful for contributors to know it was no longer planned for 6.5, as we have been asked to continue testing it, for 6.5, and it affects what we spend time on. |
I'm sorry about the miscommunication. When the PR was opened, the hope was to get it into 6.5, but as the feature cutoff approached it became clear that it wouldn't make it. I should have made it clear here that this is no longer being considered for 6.5. |
|
How does this affect https://core.trac.wordpress.org/ticket/60280 |
This PR depends on that ticket. Because reordering the style output was a pretty small and self-contained change, it could safely be included in 6.5. It probably won't have much of an effect without this PR, but it's good to have it done already. |
| @@ -541,7 +541,7 @@ class WP_Theme_JSON_Gutenberg { | |||
| * @var string[] | |||
| */ | |||
| const ELEMENTS = array( | |||
| 'link' => 'a:where(:not(.wp-element-button))', // The `where` is needed to lower the specificity. | |||
| 'link' => 'a:not(.wp-element-button)', | |||
There was a problem hiding this comment.
There are a few other places the elements selectors might need updating as well as here if we are actually wanting to increase this selectors specificity from 0-0-1 to 0-1-1 e.g. the elements block support and the global styles output constant.
Given the spirit of this PR is to reduce specificity can we omit this change?
There was a problem hiding this comment.
I might be a bit turned around jumping between PRs, I take it this change was because at least in the theme.json class element selectors are getting wrapped in :where?
As these selectors may be referenced elsewhere, is there a benefit to keeping the a:where(:not(.wp-element-button)) selector for links so it is consistent with the elements block support?
There was a problem hiding this comment.
I'm not sure! Theoretically it would be nice if the element selectors didn't make any specificity assumptions, and we just manipulated the specificity as needed when using them in different contexts.
Relatedly, I think I found a bug in the link selector on this branch: the blocks-specific link selector no longer has the :not bit attached. Looking into it now!
There was a problem hiding this comment.
Oh turns out that's only recently been fixed on trunk, so rebasing should fix it. Might not be worth it if I'm going to split this into multiple PRs though.
There was a problem hiding this comment.
For posterity, the PR fixing it on trunk was #59114.
Agreed, it's probably not work worrying about. On the splitting up front, I have a few other PRs based on this branch so my only question is do you see the different split parts as being pretty stable at this stage?
There was a problem hiding this comment.
I hope so, but this PR isn't getting a huge amount of feedback or testing so far, so part of my reason to split it up is hope that smaller PRs might be shippable faster. Stability is hard to predict without feedback! It works fine according to my testing, but I'm likely overlooking something 😅
Refactored out the common base styles for tests that don't need to target them explicitly to make maintaining these tests easier.
|
I've pushed a few fixes for the theme.json and global styles tests which now include a small refactor to make it easier to keep them up-to-date with our changes here. |
| @@ -2,6 +2,6 @@ | |||
| @include caption-style-theme(); | |||
| } | |||
|
|
|||
| .wp-block-audio { | |||
| :where(.wp-block-audio) { | |||
There was a problem hiding this comment.
Can you explain why these particular changes are necessary (block styles classnames)? I admit it's a bit hard to follow the reasoning of the whole changes.
There was a problem hiding this comment.
Yes! So this PR reduces the specificity of block global styles by wrapping their selectors in :where(). With that change, the margins set in these block-library files now have higher specificity than the global styles, so it would appear that the global styles margins have no effect.
That made it necessary to reduce the specificity of these styles, as they shouldn't be allowed to override anything theme- or user-defined.
There was a problem hiding this comment.
Won't this impact third-party blocks using this kind of styles as well?
There was a problem hiding this comment.
I'm not sure I understand. Would third party blocks use core block-library styles at all?
There was a problem hiding this comment.
I mean some third-party blocks might be using .my-block { margin: something } and with the changes being done to theme.json style generation, the styles of block will be more specific and taking over (even if the users changes the margin in global styles for instance)
There was a problem hiding this comment.
It is possible to do this @import url('example.css') layer(<layer-name>); to layer an entire stylesheet. So worth exploring whether we can update the "link" of the stylesheets that we want to auto-later with that. There's a proposal to add layer directly to <link but it's not finalized yet.
There was a problem hiding this comment.
As I mentioned in my comment above, I think an initial implementation of layers could include only core CSS (such as the block library styles) and generated CSS that's meant to be low specificity (such as global styles and block supports).
User-defined styles on specific blocks should be excluded from this as we always want them to override anything else, and theme/plugin enqueued stylesheets should almost always override core styles. I guess an exception here might be third party block stylesheets: should they be treated like core block stylesheets?
I'm not sure we should be using @import for stylesheets at all because it forces stylesheets to be loaded sequentially, which can have quite an impact on performance. There are some workarounds but I'm not sure any of them are 100% safe 😅
There was a problem hiding this comment.
It does seem like regardless of the outcome of the current PR, we should be exploring CSS layers. That said, it's clear that it's a big undertaking, so if you all think we should move forward with the current PR, I'm fine with that.
Let's make sure we keep an eye on the feedback and the impact though.
There was a problem hiding this comment.
I also agree that we should definitly investigate CSS Layers. The thing I want to call out though that has always prevented me from persuing it further is that fudamentally there is no great fallback for browsers that don't support layers.
We do have pretty decent browser supoport these days. But I think there is a big difference between features that fall back easily such as for example text-wrap: balance. If the brower doesn't support that, it just falls back to use another value.
If the browser doesn't support layers or container queries etc it cannot fallback to anything really and so the outcome is a very bad one.
There was a problem hiding this comment.
I do think we should move forward with the current PR regardless of any investigation into layers (which is fine to do in principle - we should try new stuff out - but I suspect the fact that at some point we'll have to rely on @importing third party stylesheets will prove a blocker to actually shipping it)
The potential breaking changes will be similar whether we use layers or not, so I think it would be good to come to an agreement on style hierarchy regardless of the approach. Most changes in this PR have so far been uncontroversial; the one unresolved issue is how to manage third party block styles vs global styles.
As I said above, I'm not sure this will be a major issue because the only conflict we've found so far is with blocks that:
- provide their own styles and simultaneously offer global styles support AND
- provide default styles in the block stylesheet for the same properties that can be changed in global styles
In my testing, I couldn't find any blocks that did both these things.
In any case, this will have to be clearly announced as a breaking change so block devs have the chance to make any necessary tweaks to their plugins in advance.
|
Thanks for all the feedback so far folks! This PR has become a bit of a monster so I'm starting to think it might be better to split it into multiple, more easy to review PRs. I'm thinking something like:
|
|
It might be good to add this PR's efforts to the discussion over on #55829. |
|
Just tested this on a large custom build project and it doesn't break as much as I had imagined 🎉 One thing I noticed is this: The Global styles generated by the Group block flex layout controls no longer override the base flex styles.
(The class |
|
Also pinging the @WordPress/outreach group here to see if anyone else is able to provide some more real world testing :) |
Oh well spotted @fabiankaegy! I'm thinking that could be fixed by removing |
What?
Tries to address specificity issues in #56540 (comment) by wrapping the block global styles root selector, global element selectors, global feature selectors and global layout selectors in
:where().For this to work it was also necessary to lower the specificity of some block-library styles, especially ones from the optionally enqueued "theme" files.
This should get some pretty thorough testing with themes, all sorts of blocks with global styles and variations thereof.
Testing Instructions
wp-block-styles(only early block themes such as TT2 have this) and check global styles override block-library styles correctly.Testing Instructions for Keyboard
Screenshots or screencast