Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#53494 closed defect (bug) (fixed)

Styles not applied to block using theme.json with seperate assets loading enabled

Reported by: walbo's profile walbo Owned by: desrosj's profile desrosj
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: Editor Keywords: has-patch commit fixed-major dev-reviewed
Focuses: Cc:

Description

When adding styles trough theme.json it is added in <head /> of your document. With lazy loading styles enabled the block styles is added in a <style> tag below the content, and overwrites your theme.json style.

The block styles need to be added to the document before the theme.json styles, so the theme.json overwrites the block style.

Backend and lazy loading disabled works as expected.

Steps to reproduce:

  1. Create a block: npx @wordpress/create-block wp-test-block.
  2. Install the plugin created from step one. No customizations is required.
  3. Insert your new test block into a post and publish it.
  4. Create a theme.json in your theme with styles to replace the background color of the block. ex:
{
  "version": 1,
  "styles": {
    "blocks": {
      "create-block/wp-test-block": {
        "color": {
          "background": "#333",
          "color": "#fff"
        }
      }
    }
  }
}
  1. Visit your post and confirm the background color of your block is changed to #333.
  2. Enable lazy loading by adding add_filter( 'should_load_separate_core_block_assets', '__return_true' )
  3. Revisit your post and the background color from theme.json is no longer applied.

Attachments (2)

without-lazy-loading.png (768.7 KB) - added by walbo 3 years ago.
with-lazy-loading.png (776.4 KB) - added by walbo 3 years ago.

Download all attachments as: .zip

Change History (46)

#1 @walbo
3 years ago

  • Summary changed from Styles not applied to block using theme.json with lazy loading styles enabled to Styles not applied to block using theme.json with lazy loading assets enabled
  • Version set to trunk

#2 @walbo
3 years ago

  • Summary changed from Styles not applied to block using theme.json with lazy loading assets enabled to Styles not applied to block using theme.json with seperate assets loading enabled

#3 @Clorith
3 years ago

  • Milestone changed from Awaiting Review to 5.8

Moving to 5.8 for investigation and consideration as it relates to new functionality introduced there.

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


3 years ago

This ticket was mentioned in Slack in #core-editor by desrosj. View the logs.


3 years ago

#6 @desrosj
3 years ago

Possibly related: #53375.

#7 @aristath
3 years ago

Adding this fixes the issue, but I'm unsure about whether it's a good solution or not:

remove_action( 'wp_enqueue_scripts', 'wp_enqueue_global_styles' );
add_action( 'wp_footer', 'wp_enqueue_global_styles' );

The problem here is that both global styles and the block's stylesheet use .wp-block-create-block-wp-test-block as a CSS selector.
In a block theme (fse), blocks get parsed in <head> so block stylesheets get added in the <head>.
In classic themes however, blocks get parsed on-render and can't be added to <head> because the header has already been rendered. As a result, individual block styles get added in the footer, changing the CSS load priority (which is why the filter is disabled by default for classic themes).

Should we change the position of global styles?
In all cases I think they should load after block styles to make sure they are applied:

  • In block themes: Change the priority so they get loaded after block styles in the <head>
  • In classic themes: Use the code above to make them load in the footer, right after the block styles.

#8 @oandregal
3 years ago

Hey, looked a bit at this issue and this is what I find:

  1. Classic themes:
    1. styles load in the <head> with should_load_separate_core_block_assets disabled
    2. styles load at the bottom of the <body> with should_load_separate_core_block_assets enabled
  2. FSE themes: styles always load in the head both with and without should_load_separate_core_block_assets enabled.

The only case in which block styles load after global styles is 1.2, which was brought up in Gutenberg a few months ago and, apparently, was no longer happening. I wonder if something changed that introduced 1.2 back?

The options I see forward:

  • I'm a bit out of my expertise here, but wanted to share for others to weigh in: can we make it so that the post_content is pre-rendered for classic themes that have should_load_separate_core_block_assets enabled so we can enqueue the block styles in the <head>? This is: can we make it so that 1.2 doesn't happen?
  • Alternatively, we can enqueue global styles in the "footer" in 1.2, following what block styles do. My concern with this approach is the same I shared before: that by changing the place block styles are enqueued to, we may run into other conflicts with plugins and theme styles. In other words, third parties need to be aware of this logic as well. If they want to override block & global styles they can only do it by enqueuing to the footer with higher priority than the default.
Last edited 3 years ago by oandregal (previous) (diff)

#9 @aristath
3 years ago

can we make it so that the post_content is pre-rendered for classic themes that have should_load_separate_core_block_assets enabled so we can enqueue the block styles in the <head>?

That would be the ideal solution, and IMO something that should happen regardless of how styles get loaded... Ideally all blocks would be parsed before the <head> like we do in block themes and there's an older ticket for that in #46004.

by changing the place block styles are enqueued to, we may run into other conflicts with plugins and theme styles. In other words, third parties need to be aware of this logic as well. If they want to override block & global styles they can only do it by enqueuing to the footer with higher priority than the default.

True... which is why it's disabled by default in classic themes ;)
Changing the priority wouldn't be the only way for themes/plugins to override the CSS when needed btw, they could also just tweak the CSS selectors to make them more specific (though admitedly, changing them to load in the footer is the easier solution)

This ticket was mentioned in PR #1435 on WordPress/wordpress-develop by nosolosw.


3 years ago
#10

  • Keywords has-patch added

#11 @oandregal
3 years ago

I've prepared https://github.com/WordPress/wordpress-develop/pull/1435

If we want to go on with this fix, in the Gutenberg plugin we need to change how global styles are enqueued: we can no simply update the contents of the global-styles style handle. We need to make sure the global styles stylesheet is still enqueued in the <head>) for FSE themes.

#12 @desrosj
3 years ago

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

Fixed in [51240].

#13 @walbo
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Retested trunk an the ticket is still valid.. [51240] seems unrelated to this issue, so going to reopen the ticket.

aristath commented on PR #1435:


3 years ago
#14

The code looks good to me, I'm just wondering if we should introduce new wp_maybe_enqueue_global_styles_in_head and wp_maybe_enqueue_global_styles_in_footer functions, or instead tweak the wp_enqueue_global_styles function and add something like this at the top?


// Enqueue in the footer and early exit if the site is loading separate styles per block.
if ( wp_should_load_separate_core_block_assets() && doing_action( 'wp_enqueue_scripts' ) ) {
    add_action( 'wp_footer', 'wp_enqueue_global_styles' );
    return;
}

I think that would have the same result :thinking:

desrosj commented on PR #1435:


3 years ago
#15

I think I am very much against two unique wp_maybe_enqueue_global_styles_in_head|footer() functions. I'd much prefer there to be a true/false flag accepted by a wp_enqueue_global_styles() that mirrors how wp_enqueue_script() works.

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


3 years ago

#18 @desrosj
3 years ago

@nosolosw @aristath what about something like in the PR I've attached?

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


3 years ago

#20 @desrosj
3 years ago

I was just talking this through with @Clorith a bit in Slack after he was kind enough to run my new PR through some testing.

The PR currently assumes that wp_enqueue_global_styles() is run on both wp_footer and wp_head, but that's not the case. I've pushed an update to also hook the function to wp_footer so that PR should now fix the issue.

One potential side effect here is that a style handle must be registered before inline styles or other data can be added to that style. If we return early when the styles are desired in the footer, the script will not be registered, and any other code calling wp_add_inline_style() before wp_footer will silently fail.

I'm thinking that no matter what, the style should at least be registered within the wp_enqueue_scripts hook, and only enqueued on once based on the result of wp_should_load_separate_core_block_assets().

#21 @desrosj
3 years ago

Alright, I think I've updated the PR to account for that.

I've also changed wp_head references to wp_enqueue_scripts because that's when wp_enqueue_global_styles() is actually called.

#22 follow-up: @walbo
3 years ago

Have tested PR1447 and it does solve the issue ✅.

However I'm not a big fan of adding all the styles to the footer. Users may experience a small "flicker" of the content before the style is added. With this approach should_load_separate_core_block_assets does feel a bit experimental. Ideally it would be in the <head>, but I think that is a bigger job and don't think that is realistic before 5.8 is launched.


#23 @desrosj
3 years ago

Thanks @walbo. I think I'd like to get an additional review from @aristath just to confirm that PR1447 will not cause any problems somewhere else.

I'm going to leave this one until after RC 1 to allow more time for feedback.

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


3 years ago

#25 @aristath
3 years ago

Thank you @desrosj! The PR looks good to me... What we're basically doing there, is always register the stylesheet in the head, and then conditionally enqueue it in the head or footer, depending on the separate_assets var.
The code makes sense. 👍

Last edited 3 years ago by aristath (previous) (diff)

#27 in reply to: ↑ 22 @aristath
3 years ago

Replying to walbo:

Have tested PR1447 and it does solve the issue ✅.

However I'm not a big fan of adding all the styles to the footer. Users may experience a small "flicker" of the content before the style is added. With this approach should_load_separate_core_block_assets does feel a bit experimental. Ideally it would be in the <head>, but I think that is a bigger job and don't think that is realistic before 5.8 is launched.

Well... loading styles in the footer is not the default. Block themes and block templates load these styles in the <head>. For classic themes this is opt-in, so users would have to specifically say "I want this". With that in mind, it's not something that will break someone's site or degrade their experience. It's a choice, and like every choice users make, this too will have its pros and cons.

As a sidenote, these styles get added inline so the browser doesn't need to make an additional request to get global styles. This makes the FOUC just a few milliseconds since the browser only has to parse the main HTML of the document and these styles are immediately available. In all my tests I was never able to visually perceive a significant difference - unless I throttle my browser to 3G speeds or below.
My 2c: If it takes more than a couple milliseconds then it's safe to assume we're dealing with a slow connection, in which case the FOUC is to many a welcomed side-effect that allows them to start consuming their content faster. Or at least it was for me when a 3G connection was all that was available in my area :D

#28 @walbo
3 years ago

Yes. As you say it is opt-in so should be fine.

The documentation on the filter should probably be updated with a section that says the styles will be loaded in the footer on classic themes.

Version 0, edited 3 years ago by walbo (next)

This ticket was mentioned in Slack in #core-editor by nosolosw. View the logs.


3 years ago

#30 @desrosj
3 years ago

  • Keywords commit added

Marking for commit pending a final review of the PR from another contributor.

nosolosw commented on PR #1447:


3 years ago
#31

Nice! This works as expected for me and the code looks good as well. I can't approve the PR but just wanted to share that this is ready in my view.

#32 @desrosj
3 years ago

  • Owner set to desrosj
  • Status changed from reopened to assigned

#33 @desrosj
3 years ago

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

In 51309:

Editor: Ensure global styles are loaded in the footer when loading core assets individually.

This fixes the logic in wp_enqueue_global_styles() to ensure that global styles are loaded in the footer when a site opts-in to loading Core block assets individually.

This fixes a bug where styles defined in themes.json are not respected.

Props walbo, nosolosw, mcsf, aristath, desrosj.
Fixes #53494.

#34 @desrosj
3 years ago

  • Keywords dev-feedback fixed-major added

Reopening for backport.

desrosj commented on PR #1447:


3 years ago
#35

Merged into Core in https://core.trac.wordpress.org/changeset/51309. Thanks everyone!

#36 @jorbin
3 years ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good, ready for backport.

#37 @walbo
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening the ticket so the backporting isn't missed.

#38 @desrosj
3 years ago

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

In 51347:

Editor: Ensure global styles are loaded in the footer when loading core assets individually.

This fixes the logic in wp_enqueue_global_styles() to ensure that global styles are loaded in the footer when a site opts-in to loading Core block assets individually.

This fixes a bug where styles defined in themes.json are not respected.

Props walbo, nosolosw, mcsf, aristath, desrosj, jorbin.
Merges [51309] to the 5.8 branch.
Fixes #53494.

This ticket was mentioned in PR #2303 on WordPress/wordpress-develop by oandregal.


3 years ago
#39

Trac ticket https://core.trac.wordpress.org/ticket/53494

In the linked trac ticket we added some logic to load the global stylesheet in the bottom of <body> for classic themes that opted-in into loading individual block styles instead of a single stylesheet for them all. At the time, block themes always loaded the global stylesheet in the <head>.

When block themes landed in core during the WordPress 5.9 this logic wasn't updated to consider them, hence the global stylesheet loaded in the <body> for them. This PR fixes this.

## How to test

  • Block theme: load a site using TwentyTwentyTwo and verify that the global-styles-inline-css embedded stylesheet is loaded in the <head>.
  • Classic theme that doesn't opt in into should_load_separate_core_block_assets: load a site using TwentyTwenty and verify that the global-styles-inline-css embedded stylesheet is loaded in the <head>.
  • Classic theme that opts-in into should_load_separate_core_block_assets:
    • Use TwentyTwenty.
    • Add add_filter( 'should_load_separate_core_block_assets', '__return_true' ); to its functions.php.
    • Load the site and verify that the global-styles-inline-css is loaded at the bottom of the <body>.

#40 @oandregal
3 years ago

Hey all, this bug should be reopened for 5.9 (I don't know how to do it). We had a regression: at some point during the WordPress 5.9 cycle, we landed the block themes but didn't update this logic for the global stylesheet to always load in the <head> for them. Got a PR that fixes this at https://github.com/WordPress/wordpress-develop/pull/2303

cc @desrosj @walbo @aristath

I'd like to propose this fix is part of 5.9.1 as well.

Last edited 3 years ago by oandregal (previous) (diff)

#41 @youknowriad
3 years ago

@oandregal should we maybe open a new ticket and target 5.9 ?

#42 @desrosj
3 years ago

@oandregal A new ticket referencing this one would be great! You can set the milestone to 5.9.1. The Version should be 5.8, as that's the first version affected by the regression.

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

#43 @oandregal
3 years ago

Ok, created https://core.trac.wordpress.org/ticket/55148 I'm not able to assign the 5.9.1 milestone, though.

Note: See TracTickets for help on using tickets.