WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

#53327 closed defect (bug) (fixed)

Twenty Twenty editor stylesheet causing issues on block Widgets screen

Reported by: isabel_brison Owned by:
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-screenshots
Focuses: Cc:

Description

As reported here: https://github.com/WordPress/gutenberg/issues/31947, the non-standard way in which Twenty Twenty enqueues editor styles is causing some styling issues in the block Widgets screen.

Attachments (2)

twenty-twenty-with-editor-styles.png (83.1 KB) - added by desrosj 3 months ago.
after-beta-3-updates.png (673.4 KB) - added by desrosj 3 months ago.

Download all attachments as: .zip

Change History (16)

#1 @joyously
4 months ago

I disagree that this is a theme issue. There could be many themes that choose this method of styling the editor, and I think the fix is to make the environment distinguished from page to page. The post editor has the action for loading the block editor assets, but that should not be the same action on the Widget page since the Widget page is not the block editor.

In other words, make the new screen (Widget page) behave better and match what the themes are already doing (and allowed to do). It is not the theme's problem. The theme should remain this way so that it is a good way to test, representing all the themes out there that are doing it this way.

#2 follow-up: @noisysocks
3 months ago

  • Milestone changed from Awaiting Review to 5.8

The post editor has the action for loading the block editor assets, but that should not be the same action on the Widget page since the Widget page is not the block editor.

Are there any disadvantages to having the widgets page use a different hook? For example, would it mean that some third party blocks don't work in the widgets page?

#3 @desrosj
3 months ago

This one is directly related to a feature being introduced in 5.8, so this should remain in the milestone past today's 5.8 beta 1 deadline.

This ticket was mentioned in Slack in #core by chaion07. View the logs.


3 months ago

#5 in reply to: ↑ 2 @caseymilne
3 months ago

Replying to noisysocks:

The post editor has the action for loading the block editor assets, but that should not be the same action on the Widget page since the Widget page is not the block editor.

Are there any disadvantages to having the widgets page use a different hook? For example, would it mean that some third party blocks don't work in the widgets page?

Code needs to be reviewed and tests run to determine the answer to this, so what is the name of the hook for "loading the block editor assets"? Let's start by linking to that from the ticket so others can delve into it... and search some plugins and themes for use of that hook to find suitable candidates to test?

#6 @noisysocks
3 months ago

enqueue_block_editor_assets is the hook we're talking about. It's intended to be used for blocks to enqueue their scripts and styles but Twenty Twenty is using it to enqueue editor styles because of a limitation in add_editor_style(). See GB18571 for more about that.

You can see this issue manifest in a different way by activating Twenty Twenty, creating a new post, opening Preferences, and disabling "Use theme styles". The editor should appear unsettled but, because Twenty Twenty is using enqueue_block_editor_assets, the styles are still loaded and toggling the preference does nothing.

There's a few potential solutions to this issue:

  1. We could fix the underlying issue with add_editor_style() which caused Twenty Twenty to use this workaround in GB18571. I don't think it's feasible to do this in time for WordPress 5.8, though.
  1. We could add editor styles to the widgets editor. GB26163 tracks this. I don't think that it is feasible to do this in time for WordPress 5.8 though as there are some design considerations (see GB32449) which need to be thought out. Also, this wouldn't fix the underlying issue because Twenty Twenty would still style the editor even when "Use theme styles" is disabled.
  1. We could add an "are editor styles enabled?" check to Twenty Twenty's twentytwenty_block_editor_styles function which does not add the styles if editor styles are disabled. This might not be possible though, as the "Use theme styles" setting is stored in localStorage and not accessible by PHP. It would be a nice solution if possible though.
  1. We could have the widgets screen fire a different hook to enqueue_block_editor_assets. For example, enqueue_widgets_block_editor_assets. This would fix the problem with Twenty Twenty but has the very unfortunate side effect of requiring that all third party blocks need to enqueue their scripts and styles in both enqueue_block_editor_assets and enqueue_widgets_block_editor_assets. That is, the widgets editor would not support third party blocks by default.
  1. We could add an "is this the widgets screen?" check to Twenty Twenty's twentytwenty_block_editor_styles function which does not add the styles if e.g. $pagenow === 'widgets.php'. This doesn't solve any of the underlying issues but, of course, is very doable in time for WordPress 5.8.

So, in my view, (1) is the proper fix but perhaps not feasible for WordPress 5.8. (3) is a good alternative but perhaps not possible. (5) is potentially the best temporary solution.

Last edited 3 months ago by noisysocks (previous) (diff)

#7 @isabel_brison
3 months ago

I opened this issue without the expectation it would be fixed for 5.8, as there is still no other way for themes to add inline styles to the editor, as outlined here: https://github.com/WordPress/gutenberg/issues/18571

There is also a separate discussion about enabling theme styles in the Widgets editor, here: https://github.com/WordPress/gutenberg/issues/26163 and in #53388, and if we go ahead with that one (not for 5.8), this ticket will become a non-issue.

As an alternative to @noisysocks 's option 5 above, we could also look at adding some temporary CSS overrides in the Widgets editor. With both solutions, we can only be sure to fix the problem with Twenty Twenty, not with other themes that might be doing the same, but no solution is guaranteed to work with all themes, so I think that's acceptable.

#8 @joyously
3 months ago

This same issue will come up in any other new screens that are added, if they all call the same hook that is labeled as the block editor. I think that generalizing a hook is a bad thing, as all the other hooks are specific. If plugins add metaboxes or try to affect the editor sidebar, they will have the same problem because this is not the actual editor.

My theme also enqueues its own styles because it uses :root for custom properties, and that doesn't work with the editor style processing. I was going to say I think there are a lot of themes doing this, but I went and searched and found 1058. https://wpdirectory.net/search/01F86MQD78H4YP4HH659XSPXM6
I don't think "fixing" Twenty Twenty should even be considered, since this is a documented hook and it is well used by many themes. New features should be a bit more backward compatible than has been considered. The widget screen doesn't need to match the front end, actually.

Perhaps what is needed is a 'load_blocks' sort of hook, that could be called from the editor and from any new pages that involve blocks. That way, blocks that want to participate would hook to that.

#9 @noisysocks
3 months ago

Perhaps what is needed is a 'load_blocks' sort of hook, that could be called from the editor and from any new pages that involve blocks. That way, blocks that want to participate would hook to that.

Is this not what enqueue_block_editor_assets is though? The post editor, widgets editor, and site editor are all "block editors" hence why the hook is fired. It's similar to get_current_screen()->is_block_editor().

The problem with adding a new hook is that it means third party blocks will not work by default in the widgets editor (or site editor, when it is added) which is far from ideal. Many plugins use enqueue_block_editor_assets to enqueue the scripts and styles needed by their third party blocks because this is what the official block editor documentation says to do.

https://wpdirectory.net/search/01F86NNZW3NEPJ32DNRTDXZP8V

@youknowriad @gziolo: What do you think about splitting enqueue_block_editor_assets into more discrete hooks?

#10 @youknowriad
3 months ago

I think the more we move towards declarative APIs, the better it is for me. enqueue_block_editor_assets is not really a declarative API, over time we favored APIs like:

  • register_block_type to register blocks and define to their assets
  • add_editor_styles to load editor styles in the post editor (there's no such thing for widgets editor yet as historically, widgets page didn't match the styling of the frontend unlike the classic editor)

In other words, I don't think splitting that hook makes sense to me. I'd rather for us to think about whether there's an official API we should offer for editor styles for the widgets screen (aka #53388) and even there I'm not sure whether it's needed because the widgets.php page was never meant to match the frontend.

I believe with each new feature added to Core, there's some adaptation is needed from existing themes and plugins and a dev note is required in these cases.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 months ago

#12 @desrosj
3 months ago

  • Keywords has-screenshots added

It looks like #53344 will be included in 5.8. I've done some testing with Twenty Twenty active and the changes in the PR attached to #53344, but I'm still seeing a bit of weirdness. However, I'm not sure if it's because I do not have the pending package updates in my local environment or not.

It doesn't prevent the user from interacting with the widget areas though. Where they will more closely resemble the front end now, I'm tempted to punt this and address in a follow up release.

Last edited 3 months ago by desrosj (previous) (diff)

#13 @isabel_brison
3 months ago

However, I'm not sure if it's because I do not have the pending package updates in my local environment or not.

The latest package updates should fix any outstanding issues such as the widget area heading height. Let's test again after Beta 3 and if all is good we can close this ticket :)

#14 @desrosj
3 months ago

  • Resolution set to fixed
  • Status changed from new to closed

After the beta 3 updates in [51198-51200] have resolved this issue. Closing as fixed!

Note: See TracTickets for help on using tickets.