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

Try: make logic for conditional editor_styles consistent #45351

Closed
wants to merge 12 commits into from

Conversation

mikachan
Copy link
Member

@mikachan mikachan commented Oct 27, 2022

What?

In #44640, we added a check for wp-block-styles theme support before including the wp-block-library-theme CSS in the editor.

This seemed to work well in most cases, however, there are still some cases where this CSS is included. As seen in this issue for TT3 WordPress/twentytwentythree#295, the opinionated width of 100px for the separator is still being loaded from the theme.css file in the editor, but this isn't used on the front end.

Editor Front end
image image

Why?

The editor styles should match the front end.

How?

This PR is an attempt to never allow the wp-block-library-theme CSS to be loaded under the conditions we introduced in #44640.

I've made the logic consistent between /template-parts-screen.php and /script-loader.php, so the condition to include the theme.css file is only if the active theme supports wp-block-styles AND there are no editor_styles set.

Testing Instructions

  1. Activate TT3 / a theme that doesn't support wp-block-styles AND doesn't include add_editor_style( 'style.css' );
  2. Insert a Separator block, see the difference in width between the editor and front end

Example separator markup for copy/pasting:

<!-- wp:separator -->
<hr class="wp-block-separator has-alpha-channel-opacity"/>
<!-- /wp:separator -->

@codesandbox
Copy link

codesandbox bot commented Oct 27, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@mikachan mikachan added [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Type] Bug An existing feature does not function as intended labels Oct 27, 2022
@scruffian
Copy link
Contributor

I'm not sure if something changed in TT3 or in Gutenberg, but this isn't working for me, sorry :(

@mikachan
Copy link
Member Author

mikachan commented Dec 16, 2022

I'm not sure if something changed in TT3 or in Gutenberg, but this isn't working for me, sorry :(

Thanks for testing this out. I checked this just after you left this comment, and I was seeing the same as you where the fix here didn't have any effect. I tried debugging different things but I didn't get anywhere, so I left it. Now however it does seem to be working again. I'm not sure what's different! But there were several hundred commits between this and trunk, so I've rebased 😅

I think I'm understanding the problem more now, as this seems very specific to TT3. TT3 doesn't have a functions.php, which means it doesn't provide any editor styles. I believe this means this logic, ( ! is_array( $editor_styles ) || count( $editor_styles ) === 0 ), is causing the theme.css to load in the editor.

Is this still the logic that we want to determine including the theme.css? I've updated this PR to remove it, which fixes the issue I'm seeing with the separator and means theme.css isn't included.

If we still need this check, I think the order needs to be switched so $editor_styles are checked before the theme supports, like this: ( ! is_array( $editor_styles ) || count( $editor_styles ) === 0 ) && current_theme_supports( 'wp-block-styles' ).

Nevermind, I don't think this is working either. It's certainly related to the missing add_editor_style( 'style.css' ); in a functions.php file for TT3, but the above changes I described don't seem to make a difference. If I remove all the calls to the theme.css file in GB, wp-admin/load-styles.php still seems to load these styles in regardless.

@mikachan mikachan marked this pull request as ready for review December 16, 2022 17:31
@mikachan
Copy link
Member Author

I've narrowed this down to the logic for including the theme.css stylesheet not being consistent, namely in script-loader.php. I've updated the logic in this PR to what I think is correct.

Also, I can confirm how to replicate this now. The theme should not support wp-block-styles AND not include add_editor_style( 'style.css' ); / a functions.php file.

However, the only way I could successfully test this was to update the script-loader.php file in Core. I'd appreciate if anyone could confirm that this should be an update to Core rather than in Gutenberg.

I think this update is important to allow themes to not include a functions.php file if it's not necessary (like in Twenty Twenty-Three).

@github-actions
Copy link

github-actions bot commented Jan 25, 2023

Flaky tests detected in 823dfcc.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4221169591
📝 Reported issues:

@carolinan
Copy link
Contributor

carolinan commented Jan 26, 2023

I am not sure what may have changed but I don't see the incorrect editor styles being loaded if I activate Gutenberg trunk. ?

@mikachan
Copy link
Member Author

Thanks for taking a look, @carolinan.

I'm still seeing the incorrect width on the separator block on trunk in the editor:

Editor on trunk Editor with this PR Front end
image image image

I have to apply the changes in this PR directly to Core (my local version of wordpress-develop) to make this work, rather than just Gutenberg. I'm guessing that's because I'm not sure how the compat files work!

@carolinan
Copy link
Contributor

Hm.. I was adding a new separator in the block editor, there it looks fine.
But when I look at the one already saved in the template in the site editor, it is too short.

@carolinan
Copy link
Contributor

carolinan commented Jan 26, 2023

I'm not sure how the compat files work!

We need someone to do a zoom call with us and explain it :)

Comment on lines +83 to +86
if (
current_theme_supports( 'wp-block-styles' ) &&
( ! is_array( $editor_styles ) || count( $editor_styles ) === 0 )
) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be updated in Core in wp-includes/script-loader.php.

@@ -178,7 +178,7 @@ static function( $classes ) {
wp_enqueue_media();

if (
current_theme_supports( 'wp-block-styles' ) ||
current_theme_supports( 'wp-block-styles' ) &&
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be updated in Core in wp-admin/site-editor.php.

@mikachan mikachan changed the title Try: fix conditional editor block styles Try: make logic for conditional editor_styles consistent Jan 26, 2023
@jffng
Copy link
Contributor

jffng commented Jan 26, 2023

As a sanity check, this PR is working for me in the front-end, post editor, and template part editor!

Unless you want to write some compatibility code to overwrite what assets are enqueued in the site editor, I do think the easiest fix is to patch this in core as you highlighted.

My impression is that the compat files all work a little differently depending on the context, and the context is always changing... I would definitely join a call with an overview though of the major places where these compat files are necessary and how to approach making changes there.

@mikachan
Copy link
Member Author

Thanks @jffng! For completeness (and hopefully to make things easier), I've opened a core patch as well: WordPress/wordpress-develop#3916

@carolinan
Copy link
Contributor

I ran into extra padding being added to group blocks that has a background, only in the site editor.
Theme, Twenty Twenty-Three:
I can confirm that the core patch solves this problem.
Before:
Screenshot 2023-02-02 at 06 20 48

After, the themes global padding is applied correctly:
Screenshot 2023-02-02 at 06 34 57

@carolinan carolinan added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label Feb 2, 2023
@carolinan
Copy link
Contributor

carolinan commented Feb 2, 2023

I do not know if this needs to be merged both into Gutenberg and core? If so it needs to be refreshed and moved to the 6.2 folder.

I know that if we want it to be in 6.2 we need to add the backport label..

@mikachan
Copy link
Member Author

mikachan commented Feb 2, 2023

I do not know if this needs to be merged both into Gutenberg and core? If so it needs to be refreshed and moved to the 6.2 folder.

Me neither! @ndiego, is this something you could help with? To summarise, this PR is changing files in the lib/compat files, but there is also a PR open for core to make the same changes there.

@ndiego
Copy link
Member

ndiego commented Feb 2, 2023

I do not know if this needs to be merged both into Gutenberg and core? If so it needs to be refreshed and moved to the 6.2 folder.

Me neither! @ndiego, is this something you could help with? To summarise, this PR is changing files in the lib/compat files, but there is also a WordPress/wordpress-develop#3916 to make the same changes there.

cc @Mamaduka @ntsekouras are you able to provide guidance on this? I am a little unsure myself. Thanks! 🙏

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Feb 7, 2023

@mikachan my understanding is that if this is to go into 6.2 you would need to:

  • Add a new file lib/compat/wordpress-6.2/template-parts-screen.php
  • Copy your modified gutenberg_template_parts_screen_init method into this file and rename it to gutenberg_template_parts_screen_init_6_2
  • Also add an action to this new file add_action( 'admin_enqueue_scripts', 'gutenberg_template_parts_screen_init_6_2', 5 ); - not 100% sure what priority to use here, I think 5 will work as no priority is set on the 6.1 version.
  • Add the new file to the 6.2 section of lib/load.php
  • Revert the change that you made in lib/compat/wordpress-6.1/template-parts-screen.php
  • Add the Backport to WP RC/Beta label
  • Add details to the relevant sections of Plugin: Backport PHP changes for WordPress 6.2 release #47187

Sing out if it does get through to the backporting stage. I am happy to help out if you are not familiar with the process.

@Mamaduka
Copy link
Member

Mamaduka commented Feb 7, 2023

In this case, I think it's okay to modify gutenberg_template_parts_screen_init directly.

Why?

  • The custom screen is only available when the Gutenberg plugin is active with WP 6.0.
  • We'll drop WP 6.0 support a few weeks after 6.2 is released, and all code living in compat/6.1 will be removed.

@mikachan
Copy link
Member Author

mikachan commented Feb 7, 2023

Thanks, both!

In this case, I think it's okay to modify gutenberg_template_parts_screen_init directly.

Do you mean directly in Core, like in WordPress/wordpress-develop#3916?

@Mamaduka
Copy link
Member

Sorry for the late reply, @mikachan!

Re template-parts-screen.php: The changes are good to merge as they're 👍

@mikachan
Copy link
Member Author

Thanks, @Mamaduka!

So is it OK to merge this PR as it is, or do I need to follow the backporting steps outlined above by @glendaviesnz?

@mikachan
Copy link
Member Author

Closing this, as @hellofromtonya kindly tested and merged the Core patch at WordPress/wordpress-develop#3916.

Thanks for everyone's help with this one! 🙇‍♂️

@mikachan mikachan closed this Feb 20, 2023
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") [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

7 participants