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

Add Core CSS to styles compat hook for editor iframe #40846

Closed
wants to merge 1 commit into from
Closed

Add Core CSS to styles compat hook for editor iframe #40846

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 9 additions & 2 deletions packages/block-editor/src/components/iframe/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ function useStylesCompatibility() {
} catch ( e ) {
return;
}

const { ownerNode, cssRules } = styleSheet;

if ( ! cssRules ) {
Expand All @@ -65,6 +64,14 @@ function useStylesCompatibility() {
return;
}

let isFilenameMatch = false;
if ( styleSheet.href ) {
const url = new URL( styleSheet.href );
if ( url.pathname === '/wp-admin/css/common.css' ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it okay to check for the full pathname? Is the check sufficient?

Do we need an allowlist of other Core styles that we want to carry over to the iframe?

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this stylesheet inside the iframe? AFAIK it's not something that targets frontend?

AFAIK alignleft and alignright are either supposed to be provided by the theme for classic themes or injected in layout styles for block themes.

Is the issue here about classic or block themes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we need this stylesheet inside the iframe? AFAIK it's not something that targets frontend?

AFAICT, it is being used on the frontend by Core's theme-compat/comments.php fallback, which in turn is used by the Post Comments block (which is a thin wrapper around comments_template()):

https://github.com/WordPress/wordpress-develop/blob/b27069117eb22c39d5230ceb39439f2dab82dc08/src/wp-includes/theme-compat/comments.php#L52-L64

Is the issue here about classic or block themes?

Block themes -- I think that's the main target for the Post Comments block.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT, it is being used on the frontend by Core's theme-compat/comments.php fallback, which in turn is used by the Post Comments block (which is a thin wrapper around comments_template()):

I guess that also means that the current fix is not enough since it only impacts the editor. I think the best solution here is to just add this to the "style.css" of the block like suggested by @Mamaduka that way it applies in both editor and frontend regardless of the theme used. Also, you might you want to target these links specifically and avoid a global rule that apply cross blocks and markup.

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 guess that also means that the current fix is not enough since it only impacts the editor. I think the best solution here is to just add this to the "style.css" of the block like suggested by @Mamaduka that way it applies in both editor and frontend regardless of the theme used.

Thanks! I had some reservations about this, but I guess it might be the most pragmatic approach then 😅

Also, you might you want to target these links specifically and avoid a global rule that apply cross blocks and markup.

Can do. Still feels like there might be some need for a more far-reaching solution that makes some of these classes available that are relied on by Core functions that are used by some dynamic blocks on the frontend, but maybe we don't have to solve that here and now... 😬

Copy link
Contributor

@youknowriad youknowriad May 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Still feels like there might be some need for a more far-reaching solution that makes some of these classes available that are relied on by Core functions that are used by some dynamic blocks on the frontend, but maybe we don't have to solve that here and now...

No, I actually think the opposite. The alignment styles are now generated by layout in block themes for all use-cases. The issue here is an outlier and that doesn't deserve a global fix. In other words blocks shouldn't be assuming that they can use "alignleft", "alignright" and expect these to be defined in all situations. These define on their container for block themes.

So actually, the block could have use custom classes and just target these instead but since it's an old template from Core. We can just add specific styles to the block using the old template.

isFilenameMatch = true;
}
}

const isMatch = Array.from( cssRules ).find(
( { selectorText } ) =>
selectorText &&
Expand All @@ -73,7 +80,7 @@ function useStylesCompatibility() {
);

if (
isMatch &&
( isMatch || isFilenameMatch ) &&
! node.ownerDocument.getElementById( ownerNode.id )
) {
// Display warning once we have a way to add style dependencies to the editor.
Expand Down