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.

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

Gutenberg: Add editor styles to WordPress.com Simple sites' site editor #22640

Merged
merged 7 commits into from
Feb 9, 2022

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Feb 3, 2022

Fixes Automattic/wp-calypso#59501

Changes proposed in this Pull Request:

  • Add a new method to inline styles in the block editor with the Core add_editor_style function.
  • Add a new method to inline styles in the site editor.

The Issue

The site editor is currently rendered inside an editor-canvas iframe, which natively supports stylesheets added with add_editor_style.

To apply those enqueued with wp_enqueue_style to the iframed canvas, Gutenberg copies the relevant stylesheets from the document's head and inlines them inside the iframe.

If the CSS is served from a different origin, this compatibility solution fails.

This is what is happening on Simple sites in production.

We have an aggressive optimization process that minifies, concatenates, and serves from a CDN all Jetpack assets.
The JP editor styles are enqueued in "the old way", and served from s0.wp.com, which breaks the Gutenberg compatibility approach.

The result is that JP blocks and editor extensions are missing all styles when rendered in the site editor.

The Fix (Kudos to @creativecoder)

add_editor_style single-handedly fixes the issue, and it also works with stylesheets provisioned by the CDN.
The function is not supposed to run with the enqueue hooks but works fine in admin_init.
It also automatically provides an RTL version of the stylesheet whenever needed.

We call add_editor_style through Jetpack_Gutenberg::add_site_editor_style, which handles most of the logic.

We'll need to keep the situation monitored, though, and come up with another solution before Core Gutenberg iframes the post editor too.


To get the full picture, please read this: paYJgx-1Kl-p2

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

There are no functional changes to Jetpack, so please follow the testing instructions in D74363-code.

@Copons Copons added [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Package] Assets [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Focus] FSE Issues related to the site editor / Full Site Editing / FSE feature in Gutenberg labels Feb 3, 2022
@Copons Copons self-assigned this Feb 3, 2022
Copy link

@test-case-reminder test-case-reminder bot left a comment

Choose a reason for hiding this comment

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

Here are some suggested test cases for this PR.

Gutenberg extensions

  • Use Core's block editor
  • Use latest stable Gutenberg plugin

Blocks

  • Tiled Gallery
  • Business Hours
  • Calendly
  • Form
  • Contact Info
  • Eventbrite
  • Google calendar
  • Mailchimp
  • Map
  • OpenTable
  • Pinterest
  • Podcast player
  • Star rating
  • Recurring Payments
  • Repeat Visitor
  • Revue
  • Simple Payments
  • Slideshow

Extensions

  • Publicize
  • Likes

If you think that suggestions should be improved please edit the configuration file here. You can also modify/add test-suites to be used in the configuration file.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello Copons! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D74357-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.


Jetpack plugin:

  • Next scheduled release: March 1, 2022.
  • Scheduled code freeze: February 22, 2022.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Feb 3, 2022
@Copons Copons marked this pull request as ready for review February 3, 2022 13:21
@Copons Copons added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 3, 2022
@Copons Copons requested review from a team February 3, 2022 13:21
@Copons Copons changed the title Blocks: Add editor styles Blocks: Add add_editor_style function Feb 3, 2022
@Copons Copons changed the title Blocks: Add add_editor_style function Blocks: Add add_editor_style functions Feb 3, 2022
@github-actions github-actions bot added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 3, 2022
@anomiex
Copy link
Contributor

anomiex commented Feb 3, 2022

I'm finding myself a bit confused here. add_editor_style is documented as being for themes to add styles to the TinyMCE editor, not the block editor?

As far as I can tell from looking at WordPress Core's block editor API documentation, it seems they want the style handle to be declared as being for the block editor via register_block_type() (specifically a style or editor_style in $args). Which also seems like it may be wanting to be done per block rather than having a giant "editor" blob like Jetpack currently does.

We'll need to keep the situation monitored, though, and come up with another solution before Core Gutenberg iframes the post editor too.

Overall this smells like a hack rather than an actual fix. A hack is ok, but maybe we should put the hack directly in the Jetpack_Gutenberg::add_site_editor_style method being added here and document it as such rather than putting it in Assets like it's something people should actually be using long term?

A non-hack Assets function seems like it would be building the args for register_block_type() (including the scripts too, not just styles) and calling that function.

@Copons
Copy link
Contributor Author

Copons commented Feb 3, 2022

@anomiex add_editor_style is indeed built with themes in mind: it looks for the stylesheet in the theme folder.
Although, it also works with absolute URLs — as we do in this particular workaround.

You are also correct when you say this is a hack, and it should be temporary until we find a more appropriate solution! 😄
The problem with registering block assets as suggested by Gutenberg is that now JP builds all of them together with general-purpose editor extensions (such as the upgrade nudge banner, which was the issue that originated this change and uncovered the missing block styles).

The Metadata API (and many, many other block improvements) has been added in 5.8, which means it has only just recently become available to JP, while JP blocks have been typically developed in 2020 with some updates in 2021.

This kind of change is outside of my knowledge area and is definitely outside of the scope of a high priority Dotcom bug fix. 🙂
The workaround proposed here is effective without introducing regressions (AFAICS, but never say never 😄).
With this in mind, I agree that updating the Assets package is probably unwarranted.
I'll update the PR consolidating the code into Jetpack_Gutenberg, clarifying the purpose of the fix.

Does Jetpack have a naming convention for functions that are not recommended for general use (something like unstable__)?

@Copons Copons force-pushed the try/inline_editor_styles branch from 620866f to 1c19fe8 Compare February 3, 2022 15:34
@Copons Copons changed the title Blocks: Add add_editor_style functions Gutenberg: Add editor styles to WordPress.com Simple sites' site editor Feb 3, 2022
@Copons Copons added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 3, 2022
@creativecoder
Copy link
Contributor

Actually, I don't think this is a hack. It seems that Gutenberg is reviving the add_editor_style function for declaring block styles that should load in the editor.

Here you can see the Site editor getting the editor styles (via the get_block_editor_theme_styles function, which uses the $editor_styles global, https://github.com/WordPress/wordpress-develop/blob/1712bfcf79097df6766f093f3695c84f058e2666/src/wp-admin/site-editor.php#L56)

And there are comments on Gutenberg issues recommending using add_editor_style for adding styles to the Site editor iframe, both for themes and plugins: WordPress/gutenberg#33212 (comment)

I believe the plan is to eventually iframe the Post editor as well, so it seems that loading block styles this way will only become more necessary.

Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

I wouldn't tie the naming to the Site editor. I think any of the editors may end up using an iframe (post, site, navigation, widgets), so I would keep this flexible.

For instance, the post editor loads these styles as well (https://github.com/WordPress/wordpress-develop/blob/1712bfcf79097df6766f093f3695c84f058e2666/src/wp-admin/edit-form-blocks.php#L194) and may lean on theme for inlining styles in an iframe. Anywhere blocks are used in a block editor, we may need to load these.

projects/plugins/jetpack/class.jetpack-gutenberg.php Outdated Show resolved Hide resolved
@Copons
Copy link
Contributor Author

Copons commented Feb 3, 2022

Actually, I don't think this is a hack.

@creativecoder Apologies, I could have detailed more.

IMHO the appropriate solution would be to build and enqueue block assets on a per-block basis through the block's metadata, as suggested above.
It would require rewriting the whole JP's build logic (it could be trivial? I don't know, but I'd rather not add the burden of regressions on me 😄 ) to split the files.
It would also leave the editor extensions assets without a good place — and yes, AFAICS at the moment the only way to enqueue non-block styles for iframed editors is add_editor_style.

I mean, we could call this hack or workaround, but either way, I wouldn't consider this a permanent solution, instead just a stopgap to take time to address the whole picture without fires. 🙂

@anomiex
Copy link
Contributor

anomiex commented Feb 4, 2022

Actually, I don't think this is a hack. It seems that Gutenberg is reviving the add_editor_style function for declaring block styles that should load in the editor.

What is Gutenberg intending to do about things like this code disabling some other styles when $editor_styles is set? https://github.com/WordPress/WordPress/blob/e8e20a282371286e958cca39ced6c9b8a76641fc/wp-includes/script-loader.php#L1550-L1553
Or the check here for theme support before processing $editor_styles? https://github.com/WordPress/WordPress/blob/e8e20a282371286e958cca39ced6c9b8a76641fc/wp-includes/block-editor.php#L522

IMHO the appropriate solution would be to build and enqueue block assets on a per-block basis through the block's metadata, as suggested above.
It would require rewriting the whole JP's build logic (it could be trivial? I don't know, but I'd rather not add the burden of regressions on me smile ) to split the files.

I can help with figuring out the build logic in Jetpack. I don't know much about the actual blocks though.

@Copons
Copy link
Contributor Author

Copons commented Feb 4, 2022

@anomiex I don't know Core plans, but at least in regards to this:

Or the check here for theme support before processing $editor_styles?

add_editor_style automatically adds support for editor-styles when used:

https://github.com/WordPress/wordpress-develop/blob/871afd0205214a745d85d20349a1f2bc0fe11895/src/wp-includes/theme.php#L2086

@anomiex
Copy link
Contributor

anomiex commented Feb 4, 2022

add_editor_style automatically adds support for editor-styles when used:

Ah, good to know. But then does that have any side effects elsewhere? I don't see any at a quick grep, but it is apparently output in the REST API.

Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

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

✅ for this fixing the WP.com issue with Jetpack block styles in the Site editor (when combined with D74363-code). I left comments on both with a couple of tweaks, then let's get this fix out there as soon as we can!

@sdixon194 sdixon194 enabled auto-merge (squash) February 9, 2022 16:28
@sdixon194 sdixon194 merged commit 2f77f6a into master Feb 9, 2022
@sdixon194 sdixon194 deleted the try/inline_editor_styles branch February 9, 2022 16:29
@sdixon194 sdixon194 added this to the jetpack/10.7 milestone Feb 9, 2022
@github-actions github-actions bot removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Feb 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2022

Great news! One last step: head over to your WordPress.com diff, D74357-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@Copons
Copy link
Contributor Author

Copons commented Feb 9, 2022

r239822-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Focus] FSE Issues related to the site editor / Full Site Editing / FSE feature in Gutenberg [Package] Assets [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FSE Beta: Upgrade Nudge on Cover Blocks Breaks Layout in Site Editor
5 participants