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

Fix enqueuing block theme styles when separate asset loading is enabled. #60098

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

tellthemachines
Copy link
Contributor

What?

Fixes this issue encountered in testing #59457.

When wp_should_load_separate_core_block_assets() is true, full block library stylesheets are dequeued in favour of loading only the styles for blocks present on the page, each in its own <style> tag. If the theme has support for wp-block-styles and the block has a theme.scss stylesheet, that also gets enqueued in its own style tag.

The bug in Gutenberg is that the styles coming from theme.scss were being appended to the tag containing the main block styles, so they're being duplicated (because core is still enqueuing the theme styles in their own tag).

This PR changes the logic so that the theme.scss styles get enqueued in their own tag, with the core ones being dequeued first.

I also removed a redundant bit of logic to de-register the main stylesheet, because that already gets removed on the core side when wp_should_load_separate_core_block_assets() is true.

Testing Instructions

  1. Activate Twenty Twenty Two theme;
  2. Add an Audio block to a post and inspect its styles on the front end;
  3. On trunk, verify that the styles inside wp-block-audio-theme-inline-css are duplicated in wp-block-audio-inline-css and changes made to the Audio block theme.scss file end up inside wp-block-audio-inline-css;
  4. On this branch, verify that there is no duplication of styles in these two tags and changes made to the Audio block theme.scss file are shown in wp-block-audio-theme-inline-css.

Testing Instructions for Keyboard

Screenshots or screencast

Trunk:
Screenshot 2024-03-22 at 12 03 06 PM

This PR:
Screenshot 2024-03-22 at 12 06 43 PM

@tellthemachines tellthemachines added [Type] Bug An existing feature does not function as intended [Package] Block library /packages/block-library CSS Styling Related to editor and front end styles, CSS-specific issues. labels Mar 22, 2024
@tellthemachines tellthemachines self-assigned this Mar 22, 2024
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for the quick work on this fix @tellthemachines 🚀

It tests as advertised.

✅ Could replicate the issue on trunk
✅ Block theme styles are now only in their -theme-inline-css style tag
✅ Block theme styles are consistent between editor and frontend
✅ Block theme styles aren't included when wp-block-styles support isn't opted into
✅ Theme styles are still combined and not duplicated when should_load_separate_core_block_assets is false

The only minor difference I noted was that on trunk the core block theme styles appear minified whereas they aren't on this PR.

Comment on lines -276 to -288
// Dequeue the full stylesheet.
// Make sure this only runs once, it doesn't need to run for every block.
static $stylesheet_removed;
if ( ! $stylesheet_removed ) {
add_action(
'wp_enqueue_scripts',
static function () {
wp_dequeue_style( 'wp-block-library-theme' );
}
);
$stylesheet_removed = true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice clean up 👍

lib/blocks.php Outdated Show resolved Hide resolved
lib/blocks.php Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 22, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: andrewserong <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

lib/blocks.php Outdated
@@ -273,43 +273,22 @@ function gutenberg_register_core_block_assets( $block_name ) {
// and instead attach each block's theme-styles to their block styles stylesheet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: one line above this comment (Github wouldn't let me comment on it) the comment says "dequeue the full stylesheet". Looks like we can remove that part of the comment now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh well spotted! I'll update the comment.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Working nicely for me, too!

✅ When trying to manually add a :where() rule to the margin rules, in trunk this only applies to the wp-block-audio-inline-css with the other theme styles overriding it. With this PR applied, the rules get applied to wp-block-audio-theme-inline-css as expected, with no duplication 👍

Before After
image image

LGTM, too! ✨

@tellthemachines tellthemachines merged commit d1c8e3a into trunk Mar 22, 2024
58 of 59 checks passed
@tellthemachines tellthemachines deleted the fix/enqueuing-theme-styles branch March 22, 2024 03:12
@github-actions github-actions bot added this to the Gutenberg 18.1 milestone Mar 22, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
…ed. (WordPress#60098)

Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants