Styles: try wrapping with :root to fix reset styles #61638
Conversation
ellatrix
added
Global Styles
|
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/class-wp-theme-json-gutenberg.php ❔ lib/theme.json ❔ phpunit/class-wp-theme-json-test.php |
There was a problem hiding this comment.
Thanks for the PR!
In terms of specificity, :root is exactly the same as a classname, so this change effectively undoes the reduction in #60106. I think that if that's what we want to do, reverting #60106 is a better solution as it will keep the simpler, more semantic classname selector.
The other thing to bear in mind is that, if we revert the global styles specificity, we'll need to revert #60228 too, because the reduction in the layout selectors was dependent on the global styles one.
I don't have enough context on why this change is needed, so a fuller description on this PR, or link to an issue, would be much appreciated!
|
Size Change: -1.22 kB (-0.07%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
We don't want to undo any reduction in specificity, we just want to put it at a level that is the same or higher as the reset styles. We don't want to revert global styles specificity reduction.
I'm not sure how much fuller I can make it, but I'll try. When there are reset styles at the element specify level, they will overrule any zero specificity global styles? |
|
My understanding of the proposal is that:
So if that's correct, I don't think a revert is required but there might be some additional tweaks required for the block library styles for core blocks. This does still provide a general reduction in specificity, unfortunately, not to zero as we would in an ideal world starting from scratch. |
|
Flaky tests detected in f1b413f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9221646544
|
ellatrix
requested review from
artemiomorales,
michalczaplinski,
geriux and
ajitbohra
as code owners
|
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. |
| @@ -1,9 +1,7 @@ | |||
| // Same as the group block styles. | |||
| :where(.wp-block-template-part) { | |||
| &.has-background { | |||
There was a problem hiding this comment.
It seems we previously did have some specificity left here. We should make sure it tests well now.
| // Adding `figure` to the selector increases the specificity above the global | ||
| // styles specificity, which is levelled at 0-1-0. We should figure out why | ||
| // `figure` is needed. | ||
| :root :where(figure.wp-block-gallery) { |
There was a problem hiding this comment.
I just added :root as a precaution here to make sure this overrides any reset styles.
aaronrobertshaw
force-pushed
the
try/prefix-styles-with-root
branch
from
0ba0494
to
c5b2fa5
Compare
|
I've rebased this one to address the CSS linting error regarding On the choice to use |
|
The restoration of cc/ @tellthemachines and @andrewserong for your expertise on what other layout styles may also need updating Also, I have the following list of recent PRs around reducing specificity. Are there others I've missed?
|
aaronrobertshaw
force-pushed
the
try/prefix-styles-with-root
branch
from
c5b2fa5
to
885a70b
Compare
|
I've pushed a commit that I think might be the minimum set of changes required to make layout styles work and maintain some more consistent specificity. To be honest, I had a really hard time working out what should and should not be updated for the layout style selectors. Especially, when running into bugs that turned out to also be on trunk (#61849, #61846 etc) Moving forward I think we definitely need some test coverage to ensure the correct layout styles are being applied when and where they should. These would also serve as internal documentation for anyone needing to change some aspect of our layout supports. It's also worth noting here that this PR is being discussed in regards to what CSS specificity we should settle on for WP 6.6. |
|
After some discussion, and taking into account the limited time before the 6.6 beta, the option to use a This PR should be ready to be refined, reviewed, etc with a view to landing as soon as possible. |
| @@ -2,6 +2,6 @@ | |||
| @include caption-style-theme(); | |||
| } | |||
|
|
|||
| :where(.wp-block-embed) { | |||
| .wp-block-embed { | |||
There was a problem hiding this comment.
Should the .wp-block-embed figcaption rule at the top of this file have lowered specificity?
Rather than adding :root to each individual rule or removing :where, I thought it might be better to do something like this:
:root {
:where(.wp-block-embed) {
// ...
}
}This way everything is consistent.
I realize it's the same specificity as just a classname, but it makes it easier for anyone modifying the css to maintain the same specificity.
In the future if we ever need to implement layers, we can replace the :root with @layer name, or if we want to reduce specificity to zero we remove the :root {} wrapper.
There was a problem hiding this comment.
Should the .wp-block-embed figcaption rule at the top of this file have lowered specificity?
I'd say so. A rough rule could be "anything that is configurable via global styles should be".
At a glance that mixin touches color and font-size rules and captions are now a configurable element in Global Styles.
Rather than adding :root to each individual rule or removing :where, I thought it might be better to do something like this:
Sounds good to me. I'm still working through the impacts this PR will have on #61032 and haven't gone through all the block styles yet.
My understanding is this PR has only tweaked what was changed in the original zero-specificity PRs which has proven to be somewhat incomplete. There was some discussion that we needed a complete audit of all the block styles once everything settled.
There was a problem hiding this comment.
One possible issue with the suggested approach to nesting :root and :where like that is it might encourage people to think they can still just nest the next selector within the :where when they really have to define the entire selector or make the nested selector chain another :where() e.g.
:root {
:where(.wp-block-button) {
// ...
&:where(> .wp-block-button__link) {
// ...
}
}
:where(.wp-block-button > .wp-block-button__link) {
// ...
}
}There was a problem hiding this comment.
Yeah, it gets a bit much doesn't it. 😬
There was a problem hiding this comment.
:root {
:where(.wp-block-embed) {
// ...
}
}
I had a look at this suggestion, but it becomes convoluted with some of the more complex selectors, so probably best to keep it simple right now.
|
I've updated the button block styles in 5a6a6f6. It's worth noting that the styles relating to fallback border display have been removed. Since #61556 landed, we have access to the merged global styles data in the editor settings. A follow-up is planned to update the border panel in the editor to leverage this data and apply fallbacks only when required. An alternative, looking to move the fallback border styles into the |
|
It appears the Separator block's global styles aren't working with themes that define more than just a single background color for the block in theme.json, including TT4. This is the current behaviour on trunk so I'm not 100% sure if there has been a regression or it was intended this way for some reason. Improving this can be added to the list of follow-ups. To progress this PR the Separator block's styles were updated in 2f70fdd, just note the odd behaviour when testing. |
|
I've made further tweaks for the Social Links, Table, and Tag Cloud blocks. These block style variation style updates need another good test but that will have to wait until tomorrow. |
aaronrobertshaw
force-pushed
the
try/prefix-styles-with-root
branch
from
f1b413f
to
06f8857
Compare
|
I'm wondering if I might have jumped the gun in 66cb8f3 & 362d2ae. The changes in those commits allow global styles padding for the blocks to take precedence over the default Imagine the following scenario:
Searching WP Directory I only found one theme that touched on It appears some themes have gotten around the "core/paragraph": {
"css": ".has-background {padding: 20px 30px;}"
},Given the spirit of this PR is to reduce risk of broken styles for WP6.6, I'm happy to revert these .has-background tweaks. They could be an easy tweak and follow-up post-6.6. What do you think? |
There was a problem hiding this comment.
Overall this appears to be testing well for me, and thanks for the detailed notes in #61638 (comment) @aaronrobertshaw. It seems to me that the overall reduction of global styles is a very complex problem, and I like the pragmatic direction of this PR in that it preserves many of the recent improvements, while adding an additional levelling of styles to find a pragmatic path forward for 6.6. In my testing so far, the layout styles appear to be working well for me, and I haven't encountered any conflicts so far in the flow, constrained, flex, and grid layout types.
Also, some of the changes here appear to resolve some subtle issues for older classic themes that depended on the wp-block-styles rules such as the margin reset on the image block.
I'm wondering if I might have jumped the gun in 66cb8f3 & 362d2ae.
Is the main concern that by making that change, global styles rules might unintentionally override individual paragraph blocks that have set a background color? I'm a little on the fence about this one, as I'd imagine if someone has set the list or paragraph blocks to have a particular value in global styles they might actually want it to override that default .has-background padding — my assumption is that that rule was a bit of a pleasing visual reset rather than a desired end state.
They could be an easy tweak and follow-up post-6.6. What do you think?
That sounds very reasonable to me, and if we find that it is an issue for folks during the beta period, there could be an argument for attempting to get the follow-up in sooner.
|
Greatly appreciate the thoughtful review @andrewserong 🙇
Agreed, the consensus coming from recent discussions definitely seems to have settled on a workable approach for WP6.6.
💯 These subtle issues have proven tricky to surface or predict generally. Another reason why minimising our specificity changes for this release is the safer bet.
This was my thinking too. I viewed the tweaks as more "fixes" than "changes". That said, it's a minor edge case that probably doesn't warrant changing now if we don't have to. Giving it a long run in Gutenberg post-6.6 along with some more advanced warning might smooth that transition. Another possibility to consider is a user tinkering in Global Styles with the paragraph block padding values. If they left styles there that weren't taking effect due to I've reverted those |
|
Separate follow-up for the |
There was a problem hiding this comment.
Just gave this a re-test, and it's still looking really good to me (tested in block themes and in Twenty Seventeen as a classic theme that uses wp-block-styles), with layout styles working where I'd expect them to. Thanks for the detailed follow-ups and considerations here.
I found a couple of small issues in editor styles for a couple of blocks (I've left two comments). Since it only affects the editor, do you think it'd be worth fixing them up in follow-ups? I'm aware this is already a fairly big PR, so I wondered if it'd be easier to address separately (but still for 6.6) since the impact is only within the editor.
Overall the changes here appear to be logical and pragmatic to me, the issues I ran into appear to be to do with the challenges of making changes to fairly brittle styling in some of the core blocks. Typically, I'd lean toward approaching the styling changes for each of these blocks in their own PRs, but given time constraints for the 6.6 feature freeze, in this case, I'm supportive of the idea of going with this PR, and then fixing up issues in subsequent PRs if that works for everyone.
There was a problem hiding this comment.
Thanks for the updates to the latest posts and separator blocks @aaronrobertshaw, that's resolved those issues for me!
This PR's looking in pretty good shape to me. Given the complexity involved and that in manual testing we believe that things are pretty much working nicely, I think this is in a good place to land, and we can continue to refine things in follow-ups if need be. At this stage, I think merging this larger PR is the pragmatic way to go to ensure the changes can make it in for 6.6, and we still have the Beta period to thorough test the changes and address any points of friction.
LGTM! 🚀
|
Appreciate your time and attention to detail on this one @andrewserong 🙇 I'll look to get this merged then address any follow-ups as required. A draft backport PR is available in WordPress/wordpress-develop#6633. It might be easiest to close the currently pending backport for the old zero-specificity changes (WordPress/wordpress-develop#6522) then proceed with the current draft. |
|
The failed Compressed Size / Check job is only due to the recent stale types issue and unrelated to this PR. I'll go ahead and merge this to save the rebase and re-waiting on e2s, unblocking follow-up work on section styles. |
|
(belatedly) gave this a test and all seems to be working as expected!
That sounds sensible, I went ahead and closed my PR. |
|
Backport is ready for review over in WordPress/wordpress-develop#6633 |
aaronrobertshaw
added
the
Needs Dev Note
What?
In order to apply global styles after any theme reset styles, e.g.
ol, ul { ... }, increase the specificity slightly, while keeping CSS overall specificity levelling and specificity decreases.To do: figure out where else exactly we need
:root, I'm not super familiar with this code.Note: Block style variations and their selectors will be handled via a separate follow-up as they haven't yet had their specificity reduction merged in Gutenberg (see #61032).
Why?
To make sure reset styles not wrapped in
:wheredo not break anything.Why
:rootand nothtmlorbody? Because:roothas the same specificity of attributes and would catch all kinds of reset styles including those with attributes (example).How?
Wrapping all our specificity normalised/levelled rules in
:root, so that they run after these reset styles.Testing Instructions
The only unit tests we have are covering the generation of styles rather than their application, so we'll have to rely on some manual testing or adding new tests. Any help here is appreciated! :)
Manual Testing
Testing Instructions for Keyboard
Screenshots or screencast