Editor styles: if scope is needed, don't increase specificity #56649
Conversation
ellatrix
added
[Type] Bug
ellatrix
requested review from
oandregal,
jasmussen,
michalczaplinski,
andrewserong and
aaronrobertshaw
|
Size Change: +18.2 kB (+1%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
The code change looks good although I have one quick question.
Should we also be wrapping the .editor-styles-wrapper class in :where for the Additional CSS control in the Advanced panel?
Also, following the test instructions and registering a v2 block via the console didn't seem to result in a non-iframed editor for me. I ended up testing by enabling custom fields through the post editor's preferences.
Once I got a non-iframed editor, the .editor-styles-wrapper class was wrapped in a :where.
It might pay to get a second set of eyes on this in case I've missed testing something.
When this lands, I take it the next steps will be to cull the uses of .editor-styles-wrapper from places like block editor.scss stylesheets? Do we plan to do that on a block-by-block basis?
There was a problem hiding this comment.
This seems to be working well for me, too!
One potential concern — while this reduces the specificity of when .editor-styles-wrapper is injected, what happens for rules that were originally against body, but now have the reduced specificity of :where(.editor-styles-wrapper)?
For example, in the iframed editor, the following rules are output for TT4 via body (0,0,1) specificity:
When not iframed, these rules now have specificity of (0,0,0):
In practice this doesn't appear to cause issues with the core themes I've tested with (TT4, TT3), but could it potentially cause issues for themes out in the wild that have rules that were expected to be overwritten by rules attached to body, but might no longer be if the scoped styles are reduced in specificity?
Or to put it differently, the idea of this PR sounds good to me, but how do we determine whether it could be a breaking change for folks? 🤔
| @@ -88,7 +88,7 @@ export default function EditorStyles( { styles, scope } ) { | |||
| return [ | |||
| transformStyles( | |||
| _styles.filter( ( style ) => style?.css ), | |||
| scope | |||
| scope ? `:where(${ scope })` : undefined | |||
There was a problem hiding this comment.
Just a non-blocking question: is it better to do this internally within EditorStyles or would there be any benefit of having the consumer pass in :where(.editor-styles-wrapper) explicitly for the scope?
There was a problem hiding this comment.
Given that the intention behind scope is to make sure there's no bleed of content styles into the editor chrome, I think it makes sense for the scoping selector to always have 0 specificity. This also helps ensure the specificity will always be the same between editor and front end.
|
Personally I can't imagine what theme would've been built expecting However, there are also far more specific ones than simply replacing |
|
Related: #57176 |
|
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. |
tellthemachines
force-pushed
the
try/editor-style-scope-without-specificity
branch
from
caa0c53 to
ee81eff
Compare
There was a problem hiding this comment.
This PR solves https://core.trac.wordpress.org/ticket/60614 and seems like a good idea in principle, so giving it a round of testing! I rebased the branch because the node version was updated since this PR was made, and also to test with other recent changes.
I've found a couple of issues with block layout styles in the non-iframed post editor.
- The gallery block has an overly specific fallback display style which isn't playing well with this specificity reduction (I think ideally we'd reduce the specificity for that selector too)
- The
.wp-container-layout styles aren't being output at all. Not sure why this change would cause that to happen, but testing with a Row or Stack block, any non-default styles aren't being applied.
non-iframed post editor on trunk:
non-iframed post editor on this branch:
(the two images are a Gallery block)
|
Oh, I see what the problem is: layout styles are still manually adding Unfortunately, even though the reset styles that selector was originally added to override now have minimal specificity, it's still enough to override the default layout styles if 
so I think we might still need this selector to deliver a small specificity bump. My suggestion would be to do something like |
|
Ok I've updated the PR with the solution outlined above so we can test it more easily. |
|
Thanks for updating @tellthemachines! While testing, I thought at first that this PR lowered the specificity too much and could potentially cause TT1 to not display the layout rules correctly, but after testing on 
Overall, I like the idea of passing in the desired scope for the editor styles in It looks like some of the layout logic's unit tests are failing, I think the expected styling just needs updating from this line onwards, for |
|
Thanks for reviewing and testing, @andrewserong!
Hmm, are you on an older version of WP? I remember that being an issue on TT1 a while ago but it has since been fixed (I don't see the rule containing I'll leave test updating until we're reasonably sure this is the way forward to avoid unnecessary work 😅 |
Ah, yes, of course! My bad. I run a few different local environments and I was on 6.4.3 with TT1 2.0. In my 6.5-RC3 + TT1 2.1 environment, this works fine 👍 |
|
OK @ellatrix told me she's fine with my hijacking of her PR 😄 and there don't seem to be any further objections, so I'll go ahead with updating the tests now! |
There was a problem hiding this comment.
Thanks for pushing this one forward @tellthemachines 👍
It's looking pretty close to me.
✅ Button outline issue is resolved on this branch
✅ Gallery block displays correctly
✅ div:where(.editor-styles-wrapper) selector is used in generated styles (with one exception)
✅ Unit tests are passing
✅ Could not see any differences for blocks between iframed and non-iframed editors
As noted earlier, there's one more place I think that needs the switch from .editor-styles-wrapper to div:where(.editor-styles-wrapper).
The custom CSS feature wraps raw CSS, e.g. background: blue, in .editor-styles-wrapper. If I understand how this all fits together we just need to tweak the on change and blur handlers.
Hmm, adding custom CSS both in the top-level "Additional CSS" field in global styles and in the block-specific ones generates correctly scoped selectors in the non-iframed editor: 
Am I missing something here? |
Try without a selector in the top-level Additional CSS e.g. styles for whatever the root selector should be. You can use exactly the CSS quoted, |
I definitely must be missing something 😅 Adding just styles without a selector in the top level Additional CSS produces no output for me. I'm trying on trunk too, with the same result. Testing with TT4, both with a style variation and the default, neither of which have top-level CSS declared in theme.json. |
|
My testing resulted in the same as @tellthemachines (root Additional CSS in global styles appears to require selectors, block-level in global styles does not). Output and scoping of root Additional CSS using
From a little digging, I think what's happening with these blur and change handlers is a little deceptive. These handlers aren't adding any scoping to the output of the CSS rules, but rather the So, I think the confusion here is that the From manual testing, I can see that root Additional CSS rules are output on the site frontend with no additional scoping (so the rules don't apply anywhere), so it looks like there isn't (yet) a feature for automatically wrapping root CSS rules: 
In PHP, the output in get_base_custom_css doesn't add any wrapping selectors, but the processing in get_block_custom_css_nodes does for block-level-in-global-styles custom css. Apologies that was all a bit densely worded! In case that doesn't make sense reading it in long-form, the TL;DR:
There is every chance I might have missed some details in my reading there (or be way off in my understanding), so please double-check my findings 🙂 I'm at the end of my week now, but happy to give this PR another test next week. For now, it sounds like it might not need any further changes? |
Yes, I was just having a closer look at that code and your assessment matches mine! I don't think further action is needed here. Thanks for looking into it! |
|
Thanks for digging deeper and clarifying what is happening there 👍 I've had another quick look to see where some of the 
|
| @@ -38,7 +38,7 @@ export function ExperimentalBlockCanvas( { | |||
| > | |||
| <EditorStyles | |||
| styles={ styles } | |||
| scope=".editor-styles-wrapper" | |||
| scope="div:where(.editor-styles-wrapper)" | |||
There was a problem hiding this comment.
| scope="div:where(.editor-styles-wrapper)" | |
| scope=":where(.editor-styles-wrapper)" |
This selector doesn't match when the iframe preview is active (class is applied to body, not a div). We can just remove the tag from the selector and it will work just as well.
There was a problem hiding this comment.
The selector doesn't need to match the iframed state, because this instance of EditorStyles only renders when there's no iframe.
This comment explains why the increased specificity provided by the div selector is necessary.
There was a problem hiding this comment.
Could we add a comment inline?
There was a problem hiding this comment.
When testing in 6.5, this selector was still being applied even with the iframe preview. It seemed like the intent of this PR was to ensure it's always available no matter if the iframe preview is in use or not. In it's current form, this will break if it applies to an iframe, since the iframe markup has this class on the body and not a div.
If the intended behavior is for this to only show without an iframe preview, then it appears there's a bug somewhere. I replicated this yesterday with this branch checkout and the latest 6.5 RC (4, I believe).
There was a problem hiding this comment.
Comment added!
this selector was still being applied even with the iframe preview
Do you mean .editor-styles-wrapper or div:where(.editor-styles-wrapper)? If the latter, could you please provide detailed reproduction steps? The change in this PR is exclusively targeting the non-iframed editor. However, .editor-styles-wrapper may still be in use in the iframed editor.
Thanks for tracking that down! This is a similar case to the The downside is that after these changes I'm no longer 100% sure this patch will be suitable for a WP minor release. Looking into it now. |
Ok I tested both layout and element styles applied to block instances, which is where the |
|
Does this PR still make sense to pursue after the recent specificity changes? The issue it was proposed as a fix for has been otherwise resolved. |
|
I think this should still be done, but with |
|
Just found this issue. There's another bug that's arisen recently - #63925 that might fall under this umbrella. One of my concerns (not with this PR, but generally) is the way the prefixer works. Usually it prefixes, but for root selectors It replaces the root part of the selector. It can create inconsistency since some styles end up with specificity bumps while others don't. An example is that what's proposed in this PR would mean a style like I was recently looking at an alternative prefixing library in this PR - CSS transformation: Fix mangled selectors by switching to a different postcss plugin for prefixing. It has a callback that runs for each selector, and would allow us to change how prefixing works per selector. We could try switching to that library or propose something like that in The idea could be to transform selectors like this:
I think that could maintain specificity and ensure individual selectors are relative to one another. |
|
I've put together a proof of concept of the above in #64534 |
|
I'll close this as #64534 has now been merged. |
What?
Previously:
Currently, when the editor is iframed (most cases), we don't scope editor styles. Now the problem is that when the styles do need to be scoped in a non iframed editor, all the editor styles' specificity is bumped as well.
Why?
There's cases in block editor.css stylesheets where the extra
.editor-styles-wrapperis needed to override other styles.It's also better to keep the specificity of the editor styles css rules the same between the iframed and non iframed editor.
How?
We can use
:where()to avoid adding specificity.Testing Instructions
To trigger a non iframed editor, run this in the console:
Alternatively you could enable a plugin that adds meta boxes.
Testing Instructions for Keyboard
Screenshots or screencast