Skip to content

Conversation

@creativecoder
Copy link
Contributor

@creativecoder creativecoder commented Jul 16, 2021

Description

Prevent loading the editor styles reset in the Site Editor.

Related to #33204. Removes the reset from wp-edit-blocks dependencies when the site editor loads, for better compatibility with asset concatenation plugins (for example, https://wordpress.org/plugins/page-optimize/).

Fixes a problem when some Global styles settings do not appear to work in the editor with such plugins because wp-reset-editor-styles is concatenated and and then loaded in the Site Editor iframe with along with all concatenated scripts.

How has this been tested?

  • With a block based theme activated, load the site editor
  • Verify that wp-reset-editor-styles is not loaded, by searching the page markup for the stylesheet id (wp-reset-editor-styles-css) and/or checking to make sure the styles are not applied to .editor-styles-wrapper within the iframe
  • Also load the post editor to make sure the styles reset continues to load there

Screenshots

Types of changes

Update to existing feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@youknowriad youknowriad requested a review from ellatrix July 19, 2021 08:03
@creativecoder creativecoder force-pushed the fix/site-editor--remove-styles-reset branch from 6bfc13a to ca5916c Compare July 19, 2021 17:41
@jeyip
Copy link
Contributor

jeyip commented Jul 20, 2021

Hmmm...I had trouble getting this working to test.

Whenever I activate the page-optimize plugin to try and confirm the stylesheet concatenation bug, everything borks (with or without this branch applied). Did you do anything special to test this locally @creativecoder?

Screen Shot 2021-07-19 at 10 25 16 PM

Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

Can we get rid of the patch here as well?

// Don't try to add the reset styles, which were removed as a dependency
// from `edit-blocks` for the iframe since we don't need to reset admin
// styles.
if ( ownerNode.id === 'wp-reset-editor-styles-css' ) {
return;
}

@youknowriad
Copy link
Contributor

I think @ellatrix has an alternative PR for this. Basically, that dependency need to be removed whenever the editor is rendered in an iframe because the "reset" is only needed to reset WP-Admin styles in the editor canvas, and since iframe styles are sandboxes, that's not needed.

@creativecoder creativecoder force-pushed the fix/site-editor--remove-styles-reset branch from ca5916c to 6aa0d70 Compare July 26, 2021 15:25
@creativecoder creativecoder self-assigned this Jul 26, 2021
@creativecoder creativecoder added the [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") label Jul 26, 2021
@creativecoder creativecoder marked this pull request as ready for review July 26, 2021 15:49
@creativecoder
Copy link
Contributor Author

Can we get rid of the patch here as well? (

// Don't try to add the reset styles, which were removed as a dependency
// from `edit-blocks` for the iframe since we don't need to reset admin
// styles.
if ( ownerNode.id === 'wp-reset-editor-styles-css' ) {
return;
}
)

@jeyip We probably could, though I left it alone thinking it would be good to keep as a fallback, in case wp-reset-editor-styles is re-introduced as a dependency by a plugin.

@creativecoder
Copy link
Contributor Author

FYI I've updated the PR description to explain the reason for this change a little better (mainly better compatibility with asset concatenation plugins).

@ellatrix ellatrix requested a review from jasmussen July 26, 2021 18:08
@ellatrix
Copy link
Member

Are the common and forms stylesheets still omitted from the iframe?

@jeyip jeyip force-pushed the fix/site-editor--remove-styles-reset branch from 1cdfd4e to 183004b Compare July 26, 2021 22:44
@jeyip
Copy link
Contributor

jeyip commented Jul 27, 2021

Testing

Requirements

  • Verify that wp-reset-editor-styles is not loaded, by searching the edit site page markup for the stylesheet id
  • Verify that wp-reset-editor-styles is loaded, by searching the post editor page markup for the stylesheet id

Browsers

  • Chrome

Notes

  • I'll test and see if common and forms stylesheets are still omitted. I can confirm that I don't see forms-cssor common-css style <link /> tags in the iframe head element with this branch applied.

@jeyip
Copy link
Contributor

jeyip commented Jul 30, 2021

Friendly ping @ellatrix. Curious if you have further thoughts on this approach.

@ellatrix
Copy link
Member

  • I can confirm that I don't see forms-cssor common-css style <link /> tags in the iframe head element with this branch applied.

If I read the code correctly, the dependency is only removed from the site editor and not from the template editor (page editor => and then edit the template). This template editor is also iframed. Additionally all previews will be iframed soon for the post editor, and at some point the post editor itself.

@creativecoder creativecoder force-pushed the fix/site-editor--remove-styles-reset branch from b780177 to 6879c32 Compare August 12, 2021 15:51
@creativecoder
Copy link
Contributor Author

@ellatrix Thanks for the feedback!

I've modified the PR to keep asset loading as is for the Template editor, but additionally remove wp-reset-editor-styles as a dependency when loading the Site editor. This prevents the problem with Global styles settings not appearing to not work when the styles reset is concatenated and inadvertently loaded into the Site Editor iframe.

I don't see a way to do the same dependency manipulation for the Template editor because of how it's loaded dynamically inside the Post editor... but I'd anticipate this will eventually resolve itself if/when the Post editor is iframed.

@creativecoder creativecoder changed the title Site Editor: Remove styles reset Site Editor: Remove styles reset as script dependecy Aug 12, 2021
@creativecoder creativecoder changed the title Site Editor: Remove styles reset as script dependecy Site Editor: Remove styles reset as script dependency Aug 12, 2021
@jeyip jeyip deleted the fix/site-editor--remove-styles-reset branch August 22, 2023 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing")

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants