Retain the same specificity for non iframed selectors #64534
Conversation
…e root part of the selector
talldan
added
[Type] Enhancement
|
Size Change: +2.62 kB (+0.15%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work wrangling this one @talldan 👍
It's looking like a promising direction to me!
I still need to do some broader testing, mainly around layout and global styles, but it fixes the issue in #63925.
✅ Heading block global styles remain higher specificity than prefixed element styles
✅ Prefixed selectors match those promised in PR description
✅ Couldn't find any remaining places where .editor-styles-wrapper was programmatically added as scoping selector
✅ Element styles on block instances continue to work
On a side note, the use of snapshots for simple CSS assertions is a bit of a pain when trying to see what the expected outcome is. I like the switch away from snapshots for the new tests. If we proceed with this PR, is it then the best opportunity to get rid of the snapshots entirely?
They also caught me out checking the diff as it looked like a test had been removed.
I'll give this a further test on Monday. If there are any specific areas you'd like focus on, add them to the test instructions and I'll cast an eye over them.
| @@ -212,7 +212,7 @@ describe( 'transformStyles', () => { | |||
| } ); | |||
|
|
|||
| it( 'should not double wrap selectors', () => { | |||
| const input = ` .my-namespace h1, .red { color: red; }`; | |||
| const input = ` .my-namespace h1, .my-namespace .red { color: red; }`; | |||
There was a problem hiding this comment.
Would it be better to leave this as it is, covering different specificities in the selector list, and just update the output?
To me, the original input covered the scenario when the plugin detects that it does need to prefix something and then ensures it doesn't go overboard resulting in a double prefix.
There was a problem hiding this comment.
I liked being able to make the test expect( output ).toEqual( [ input ] ), but I've updated it in 6b43b57 to be as it was before (though without snapshots)
| return replaceDoublePrefix( | ||
| prefixedSelector, | ||
| prefix | ||
| ); |
There was a problem hiding this comment.
Would it be worth a comment here explaining why this is needed?
My understanding of the old plugin was that it ignored all selectors already starting with the prefix. This one seems to only do a simple comparison between the selectors in exclude and so requires this double handling. Is that correct?
There was a problem hiding this comment.
The 'should not double wrap selectors' was failing when I switched to the new library. You're right that the ignoredSelectors seems to be where the old library works differently to the new one. We've been using the string '.editor-styles-wrapper' as an ignored selector. With the old library it uses something akin to selector.includes( ignoredSelector ) to check if it should be ignored, whereas the new library is more like selector === ignoredSelector. Both also support regular expressions, but we weren't using that.
I think this will be an issue as transformStyles is a public API, so ignoredSelectors should work the same as before. 🤔
Probably the best bet would be to replicate how the old library works in the callback rather than the way I've done it using replaceDoublePrefix 😄
There was a problem hiding this comment.
I haven't had a chance to look deeply at the code after the latest updates but did you account for the fact the old plugin allows regex in those ignored selectors?
Here's a snippet from the plugin's readme.
// You may want to exclude some selectors from being prefixed, this is
// enabled using the `ignoredSelectors` option.
ignoredSelectors: [":root", "#my-id", /^\.some-(.+)$/],
As noted in my earlier comment, this new plugin appears to do a direct comparison of selectors. Take that with a grain of salt but something to double all the same.
There was a problem hiding this comment.
Yep, the pseudocode is selector.match( ignoredSelector ), and match which will work for both plain strings and regexes.
edit: pushed a commit to make sure there's test coverage for regex ignored selectors.
There was a problem hiding this comment.
Nice. Appreciate the extra test coverage there 👌
|
If we proceed with the new scoping selector for the non-iframed editor, we might also need to revisit quite a few |
Agree, I also didn't like the use of snapshots.
This is also a concern for any themes that might have (e.g. I'm thinking the best option would be to ignore those selectors and not try prefixing them. Alternatively, we could try replacing I can go through and manually update the block library selectors though. |
Honestly, I'm not sure either. Part of the fix/goal here is to ensure consistent specificity. Styles with selectors prefixed with Perhaps we could try out that approach here and see how it plays with the block library editor styles before we go changing them? |
|
I've pushed a commit that ignores selectors that already contain
.editor-styles-wrapper h2 {
color: orange !important;
}
I think this is a good thing to do, even if using |
talldan
requested review from
adamziel,
kevin940726,
tellthemachines,
ellatrix 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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @ktmn. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
There was a problem hiding this comment.
Thank you for all the hard work wrangling edges on this front 💪
✅ After the latest round of tweaks, the tests are looking good and I couldn't find any plausible ways of breaking the prefixing.
✅ I've searched through the code for remaining uses of .editor-styles-wrapper and they all seem to check out.
✅ The culling of outdated comments look good
✅ Each of the touched editor block styles make sense and I didn't spot any issues there
This is looking good to me @talldan and I'm happy to give it a tentative approval. I think it would be prudent though to get some extra eyes on it given the scope of the changes.
talldan
added
the
Backport to WP Minor Release
|
Thanks for the testing and reviews! I'll go ahead and merge this, but further testing feedback or code review feedback is still welcome, I'll address anything in follow-up PRs. @vcanales I've labelled this for backport to 6.6.2, though it might be worth having a conversation about the size and scope of this PR. I think it'd fix a lot of issues for users though, so hopefully it's ok. |
talldan
deleted the
try/keeping-same-specificity-for-non-iframed-editor
branch
|
Thanks for your work, @talldan! I hope your fix makes it into 6.6.2. |
|
Will discuss in today's bug scrub (starting in about 10 minutes). I'm thinking this PR is not well suited for a minor release. Why?
So I'm thinking it's an enhancement that subsequently also fixes a regression bug that (may have) happened in 6.6.0 (or earlier). As an enhancement, it's better suited for 6.7.0. |
|
Some additional info to consider: It is fixing an issue (#63925) that is labeled as a regression. If it's a regression from 6.5 to 6.6, that seems to me like this should be considered. That said, this is not a small PR and based on the other tickets connected, I'm not sure it is a regression from 6.5 or from much earlier (making it a long standing bug that is better fixed in a major version) |
|
Summary of discussion during today's Core bug scrub:
Also noting, an enhancement can be considered for a minor release per Core's handbook:
TIL |
* WIP * Small fixes, improvements to root selector handling, avoid double prefixing * Add test for compound :where statements * Fix too specific layout styles in non-iframed editor * Update handling of root selectors * Fix all the problems * Update comment * Add text for root selector text in middle of selector * Also avoid double-prefix of root selectors * Change how root selectors are transformed, inject the prefix after the root part of the selector * Remove hard-coded editor-styles-wrapper prefix from selectors * Update tests and fix double prefixing for root selectors * Fix test * Update comment * Update another test * Avoid transforming selectors that already contain .editor-styles-wrapper * Update ignored selectors to match behavior of postcss-prefix-wrap * Stop using snapshots * Add more tests * Fix typo * Add tests for regex ignores * Fix ropey root selectors and add tests * Use a tokenizer for root prefixing * Small optimization, Use a for loop to avoid unneccessary looping * Remove outdated comment * Update block library editor-styles-wrapper styles * Update comment * Add comment * Add more detail to nav block style comment * Also use same scope specificity in widget editor * Fix strings that look like regular expressions in ignoredSelectors * Reduce diff * Explain the function behavior further ---- Unlinked contributors: ktmn. Co-authored-by: talldan <talldanwp@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: kevin940726 <kevin940726@git.wordpress.org> Co-authored-by: mrleemon <leemon@git.wordpress.org> Co-authored-by: apmeyer <apmeyer@git.wordpress.org> Co-authored-by: wongjn <wongjn@git.wordpress.org>
talldan
added
[Type] Bug
|
Thanks for discussing it. I've removed the Backport label, so it isn't accidentally included 😄
Mislabelled, it's a shame that wasn't questioned a bit more, it's definitely a bug fix 🤦 Agree with the other points discussed though, this is mostly why I raised the question. 👍 |
|
To keep it on the radar for tracking, I'm adding it to the 6.6.x project board. It's already being tracked in the 6.6 Specificity/Style tracking issue #64243. |
|
What's the misunderstanding? |
|
Me not reading it correctly 🤦♀️ 🤣 Sorry about that @talldan. I re-read and now understand what you're saying here:
Deserves another 🤦♀️ from me. |
|
No worries, thanks for handling all the release coordination! |
|
@ktmn mind sharing your WordPress.org username so I can ensure its included in the 6.7 credits listing? |
What?
Alternative to #56649
Fixes #63925
Tries to maintain specificity across styles between the iframed and non-iframed canvas. This PR is slightly different to #56649 in that it also seeks to maintain specificity for root selectors.
For a quick recap, in a a non-iframed canvas
.editor-styles-wrapperis prefixed onto the majority of selectors to scope the styles. This results in a bump in specificity. For root selectors (any that begin withbody,htmlor:root) prefixing the classname would create an invalid selector, so the root part of the classname is replaced instead. This results in some of these selectors maintaining specificity, while others change ever so slightly.This inconsistency creates a situation where the specificity of some styles is bumped, others aren't, and so in a non-iframed editor styles often don't apply in the same way and can look completely different to the front end or iframed editor.
How?
For most selectors, the prefix is changed from
.editor-styles-wrapperto:where(.editor-styles-wrapper).For root selectors, a selector like
body .some-selectorchanges tobody :where(.editor-styles-wrapper) .some-selector.Both are maintaining the same specificity.
The tool of choice for prefixing is
postcssand in particular apostcssplugin calledpostcss-prefixwrapis used in trunk. In this PR I've replaced it with thepostcss-prefix-selectorplugin. This one has a callback that allows us to change how scoping works on a per selector basis, which is how the root selector behavior has been changed.One drawback of the new library is that it's
excludeoption doesn't work the same way as theignoredSelectorsoption of the old library. Fortunately the callback means it's possible to replicate that behavior.Testing Instructions
This needs a lot of general smoke testing. Below are instructions to check whether #63925 was fixed:
Test using WordPress playground
styles.blocks:Here are some diffs that show the styles before and after transformation: