Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retain the same specificity for non iframed selectors #64534

Merged
merged 33 commits into from
Aug 28, 2024
Merged

Retain the same specificity for non iframed selectors #64534

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
121ab38
WIP
talldan Jul 26, 2024
63daf74
Small fixes, improvements to root selector handling, avoid double pre…
talldan Jul 26, 2024
fa01d08
Add test for compound :where statements
talldan Jul 26, 2024
ca755ce
Fix too specific layout styles in non-iframed editor
talldan Jul 26, 2024
e73b3ae
Update handling of root selectors
talldan Jul 29, 2024
36b6fb4
Fix all the problems
talldan Jul 29, 2024
05282b1
Update comment
talldan Jul 29, 2024
5b02106
Add text for root selector text in middle of selector
talldan Jul 31, 2024
8e94cb1
Also avoid double-prefix of root selectors
talldan Jul 31, 2024
cbd97fc
Change how root selectors are transformed, inject the prefix after th…
talldan Aug 15, 2024
e65d65a
Remove hard-coded editor-styles-wrapper prefix from selectors
talldan Aug 15, 2024
b271945
Update tests and fix double prefixing for root selectors
talldan Aug 15, 2024
a9f8b10
Fix test
talldan Aug 15, 2024
17ac050
Update comment
talldan Aug 15, 2024
93455f8
Update another test
talldan Aug 15, 2024
25bfa9d
Avoid transforming selectors that already contain .editor-styles-wrapper
talldan Aug 21, 2024
6b43b57
Update ignored selectors to match behavior of postcss-prefix-wrap
talldan Aug 21, 2024
1cf8d6c
Stop using snapshots
talldan Aug 21, 2024
06051c3
Add more tests
talldan Aug 21, 2024
53ee8cf
Fix typo
talldan Aug 21, 2024
2fae7f6
Add tests for regex ignores
talldan Aug 22, 2024
8943a84
Fix ropey root selectors and add tests
talldan Aug 22, 2024
cef751e
Use a tokenizer for root prefixing
talldan Aug 23, 2024
170a313
Small optimization, Use a for loop to avoid unneccessary looping
talldan Aug 26, 2024
5292a44
Remove outdated comment
talldan Aug 26, 2024
65071fd
Update block library editor-styles-wrapper styles
talldan Aug 26, 2024
88e328e
Update comment
talldan Aug 26, 2024
9ded287
Add comment
talldan Aug 26, 2024
15f00de
Add more detail to nav block style comment
talldan Aug 27, 2024
f6a8476
Also use same scope specificity in widget editor
talldan Aug 27, 2024
69db0cb
Fix strings that look like regular expressions in ignoredSelectors
talldan Aug 27, 2024
534a8b5
Reduce diff
talldan Aug 27, 2024
b8db01a
Explain the function behavior further
talldan Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/block-editor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@
"fast-deep-equal": "^3.1.3",
"memize": "^2.1.0",
"postcss": "^8.4.21",
"postcss-prefixwrap": "^1.51.0",
"postcss-prefix-selector": "^1.16.0",
"postcss-urlrebase": "^1.4.0",
"react-autosize-textarea": "^7.1.0",
"react-easy-crop": "^5.0.6",
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/block-canvas/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export function ExperimentalBlockCanvas( {
>
<EditorStyles
styles={ styles }
scope=".editor-styles-wrapper"
scope=":where(.editor-styles-wrapper)"
/>
<WritingFlow
ref={ contentRef }
Expand Down
5 changes: 1 addition & 4 deletions packages/block-editor/src/hooks/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,10 +381,7 @@ function useBlockProps( { name, style } ) {
useBlockProps
) }`;

// The .editor-styles-wrapper selector is required on elements styles. As it is
// added to all other editor styles, not providing it causes reset and global
// styles to override element styles because of higher specificity.
const baseElementSelector = `.editor-styles-wrapper .${ blockElementsContainerIdentifier }`;
const baseElementSelector = `.${ blockElementsContainerIdentifier }`;
const blockElementStyles = style?.elements;

const styles = useMemo( () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/layouts/test/grid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import grid from '../grid';

describe( 'getLayoutStyle', () => {
it( 'should return only `grid-template-columns` and `container-type` properties if no non-default params are provided', () => {
const expected = `.editor-styles-wrapper .my-container { grid-template-columns: repeat(auto-fill, minmax(min(12rem, 100%), 1fr)); container-type: inline-size; }`;
const expected = `.my-container { grid-template-columns: repeat(auto-fill, minmax(min(12rem, 100%), 1fr)); container-type: inline-size; }`;

const result = grid.getLayoutStyle( {
selector: '.my-container',
Expand All @@ -19,7 +19,7 @@ describe( 'getLayoutStyle', () => {
expect( result ).toBe( expected );
} );
it( 'should return only `grid-template-columns` if columnCount property is provided', () => {
const expected = `.editor-styles-wrapper .my-container { grid-template-columns: repeat(3, minmax(0, 1fr)); }`;
const expected = `.my-container { grid-template-columns: repeat(3, minmax(0, 1fr)); }`;

const result = grid.getLayoutStyle( {
selector: '.my-container',
Expand Down
14 changes: 6 additions & 8 deletions packages/block-editor/src/layouts/test/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const layoutDefinitions = {
describe( 'getBlockGapCSS', () => {
it( 'should output default blockGap rules', () => {
const expected =
'.editor-styles-wrapper .my-container > * { margin-block-start: 0; margin-block-end: 0; }.editor-styles-wrapper .my-container > * + * { margin-block-start: 3em; margin-block-end: 0; }';
'.my-container > * { margin-block-start: 0; margin-block-end: 0; }.my-container > * + * { margin-block-start: 3em; margin-block-end: 0; }';

const result = getBlockGapCSS(
'.my-container',
Expand All @@ -50,7 +50,7 @@ describe( 'getBlockGapCSS', () => {
} );

it( 'should output flex blockGap rules', () => {
const expected = '.editor-styles-wrapper .my-container { gap: 3em; }';
const expected = '.my-container { gap: 3em; }';

const result = getBlockGapCSS(
'.my-container',
Expand Down Expand Up @@ -97,7 +97,7 @@ describe( 'getBlockGapCSS', () => {
} );

it( 'should treat a blockGap string containing 0 as a valid value', () => {
const expected = '.editor-styles-wrapper .my-container { gap: 0; }';
const expected = '.my-container { gap: 0; }';

const result = getBlockGapCSS(
'.my-container',
Expand All @@ -113,21 +113,19 @@ describe( 'getBlockGapCSS', () => {
describe( 'appendSelectors', () => {
it( 'should append a subselector without an appended selector', () => {
expect( appendSelectors( '.original-selector' ) ).toBe(
'.editor-styles-wrapper .original-selector'
'.original-selector'
);
} );

it( 'should append a subselector to a single selector', () => {
expect( appendSelectors( '.original-selector', '.appended' ) ).toBe(
'.editor-styles-wrapper .original-selector .appended'
'.original-selector .appended'
);
} );

it( 'should append a subselector to multiple selectors', () => {
expect(
appendSelectors( '.first-selector,.second-selector', '.appended' )
).toBe(
'.editor-styles-wrapper .first-selector .appended,.editor-styles-wrapper .second-selector .appended'
);
).toBe( '.first-selector .appended,.second-selector .appended' );
} );
} );
10 changes: 1 addition & 9 deletions packages/block-editor/src/layouts/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,11 @@ import { LAYOUT_DEFINITIONS } from './definitions';
* @return {string} - CSS selector.
*/
export function appendSelectors( selectors, append = '' ) {
// Ideally we shouldn't need the `.editor-styles-wrapper` increased specificity here
// The problem though is that we have a `.editor-styles-wrapper p { margin: reset; }` style
// it's used to reset the default margin added by wp-admin to paragraphs
// so we need this to be higher speficity otherwise, it won't be applied to paragraphs inside containers
// When the post editor is fully iframed, this extra classname could be removed.

return selectors
.split( ',' )
.map(
( subselector ) =>
`.editor-styles-wrapper ${ subselector }${
append ? ` ${ append }` : ''
}`
`${ subselector }${ append ? ` ${ append }` : '' }`
)
.join( ',' );
}
Expand Down
26 changes: 10 additions & 16 deletions packages/block-editor/src/utils/test/__snapshots__/transform-styles.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ exports[`transformStyles URL rewrite should rewrite relative paths 1`] = `

exports[`transformStyles error handling should handle multiple instances of \`:root :where(body)\` 1`] = `
[
".my-namespace { color: pink; } .my-namespace { color: orange; }",
":root :where(body) .my-namespace { color: pink; } :root :where(body) .my-namespace { color: orange; }",
]
`;

Expand All @@ -40,6 +40,12 @@ exports[`transformStyles selector wrap should ignore font-face selectors 1`] = `
]
`;

exports[`transformStyles selector wrap should ignore ignored selectors 1`] = `
[
".my-namespace h1, body { color: red; }",
]
`;

exports[`transformStyles selector wrap should ignore keyframes 1`] = `
[
"
Expand All @@ -51,30 +57,18 @@ exports[`transformStyles selector wrap should ignore keyframes 1`] = `
]
`;

exports[`transformStyles selector wrap should ignore selectors 1`] = `
[
".my-namespace h1, body { color: red; }",
]
`;

exports[`transformStyles selector wrap should not double wrap selectors 1`] = `
[
" .my-namespace h1, .my-namespace .red { color: red; }",
]
`;

exports[`transformStyles selector wrap should replace :root selectors 1`] = `
[
"
.my-namespace {
:root .my-namespace {
--my-color: #ff0000;
}",
]
`;

exports[`transformStyles selector wrap should replace root tags 1`] = `
exports[`transformStyles selector wrap should replace body selectors 1`] = `
[
".my-namespace, .my-namespace h1 { color: red; }",
"body .my-namespace, .my-namespace h1 { color: red; }",
]
`;

Expand Down
25 changes: 21 additions & 4 deletions packages/block-editor/src/utils/test/transform-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe( 'transformStyles', () => {
expect( output ).toMatchSnapshot();
} );

it( 'should ignore selectors', () => {
it( 'should ignore ignored selectors', () => {
const input = `h1, body { color: red; }`;
const output = transformStyles(
[
Expand All @@ -111,7 +111,7 @@ describe( 'transformStyles', () => {
expect( output ).toMatchSnapshot();
} );

it( 'should replace root tags', () => {
it( 'should replace body selectors', () => {
const input = `body, h1 { color: red; }`;
const output = transformStyles(
[
Expand Down Expand Up @@ -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; }`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


const output = transformStyles(
[
Expand All @@ -223,7 +223,24 @@ describe( 'transformStyles', () => {
'.my-namespace'
);

expect( output ).toMatchSnapshot();
expect( output ).toEqual( [ input ] );
} );

it( 'should not double prefix a root selector', () => {
const input = 'body .my-namespace h1 { color: goldenrod; }';

const output = transformStyles(
[
{
css: input,
},
],
'.my-namespace'
);

expect( output ).toEqual( [
'body .my-namespace h1 { color: goldenrod; }',
] );
} );

it( 'should not try to wrap items within `:where` selectors', () => {
Expand Down
60 changes: 49 additions & 11 deletions packages/block-editor/src/utils/transform-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,76 @@
* External dependencies
*/
import postcss, { CssSyntaxError } from 'postcss';
import wrap from 'postcss-prefixwrap';
import prefixSelector from 'postcss-prefix-selector';
import rebaseUrl from 'postcss-urlrebase';

const cacheByWrapperSelector = new Map();

const ROOT_SELECTORS = [
':root :where(body)',
':where(body)',
':root',
'html',
'body',
];

function replaceDoublePrefix( selector, prefix ) {
// Avoid prefixing an already prefixed selector.
const doublePrefix = `${ prefix } ${ prefix }`;
if ( selector.includes( doublePrefix ) ) {
return selector.replace( doublePrefix, prefix );
}
return selector;
}

function transformStyle(
{ css, ignoredSelectors = [], baseURL },
wrapperSelector = ''
) {
// When there is no wrapper selector or base URL, there is no need
// When there is no wrapper selector and no base URL, there is no need
// to transform the CSS. This is most cases because in the default
// iframed editor, no wrapping is needed, and not many styles
// provide a base URL.
if ( ! wrapperSelector && ! baseURL ) {
return css;
}
const postcssFriendlyCSS = css
.replace( /:root :where\(body\)/g, 'body' )
.replace( /:where\(body\)/g, 'body' );
try {
return postcss(
[
wrapperSelector &&
wrap( wrapperSelector, {
ignoredSelectors: [
...ignoredSelectors,
wrapperSelector,
],
prefixSelector( {
prefix: wrapperSelector,
exclude: [ ...ignoredSelectors, wrapperSelector ],
transform( prefix, selector, prefixedSelector ) {
const rootSelector = ROOT_SELECTORS.find(
( rootSelectorCandidate ) =>
selector.startsWith( rootSelectorCandidate )
);

// Reorganize root selectors such that the root part comes before the prefix,
// but the prefix still comes before the remaining part of the selector.
if ( rootSelector ) {
const selectorWithoutRootPart = selector
.replace( rootSelector, '' )
.trim();
const updatedRootSelector =
`${ rootSelector } ${ prefix } ${ selectorWithoutRootPart }`.trim();

return replaceDoublePrefix(
updatedRootSelector,
prefix
);
}

return replaceDoublePrefix(
prefixedSelector,
prefix
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@talldan talldan Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6b43b57

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@talldan talldan Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Appreciate the extra test coverage there 👌

},
} ),
baseURL && rebaseUrl( { rootUrl: baseURL } ),
].filter( Boolean )
).process( postcssFriendlyCSS, {} ).css; // use sync PostCSS API
).process( css, {} ).css; // use sync PostCSS API
} catch ( error ) {
if ( error instanceof CssSyntaxError ) {
// eslint-disable-next-line no-console
Expand Down
Loading