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

Editor styles: if scope is needed, don't increase specificity #56649

Closed
wants to merge 5 commits into from
Closed

Editor styles: if scope is needed, don't increase specificity #56649

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ee81eff
Editor styles: if scope is needed, don't increase specificity
ellatrix Nov 29, 2023
bfc40d2
Define scope specificity per-editor and remove extra layout scope.
tellthemachines Mar 26, 2024
ae30987
Update test strings
tellthemachines Mar 27, 2024
d11bd99
Remove specificity bump from elements selector.
tellthemachines Apr 8, 2024
5d23d70
Add explanatory comment
tellthemachines Apr 8, 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
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="div:where(.editor-styles-wrapper)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we add a comment inline?

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

/>
<WritingFlow
ref={ contentRef }
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' );
} );
} );
4 changes: 1 addition & 3 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 @@ -27,9 +27,7 @@ export function appendSelectors( selectors, append = '' ) {
.split( ',' )
.map(
( subselector ) =>
`.editor-styles-wrapper ${ subselector }${
append ? ` ${ append }` : ''
}`
`${ subselector }${ append ? ` ${ append }` : '' }`
)
.join( ',' );
}
Expand Down
Loading