WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 months ago

#53494 closed defect (bug) (fixed)

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

Reported by: walbo Owned by: 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 4 months ago.
with-lazy-loading.png (776.4 KB) - added by walbo 4 months ago.

Download all attachments as: .zip

Change History (40)

#1 @walbo
4 months 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
4 months 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
4 months 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.


4 months ago

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


4 months ago

#6 @desrosj
4 months ago

Possibly related: #53375.

#7 @aristath
4 months 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
4 months 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 4 months ago by oandregal (previous) (diff)

#9 @aristath
4 months 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.


4 months ago

  • Keywords has-patch added

#11 @oandregal
4 months 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
4 months ago

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

Fixed in [51240].

#13 @walbo
4 months 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.

#14 @prbot
4 months ago

aristath commented on PR #1435:

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:

Last edited 4 months ago by aristath (previous) (diff)

#15 @prbot
4 months ago

desrosj commented on PR #1435:

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.


4 months ago

#18 @desrosj
4 months 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.


4 months ago

#20 @desrosj
4 months 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
4 months 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
4 months 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
4 months 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.


4 months ago

#25 @aristath
4 months 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 4 months ago by aristath (previous) (diff)

#27 in reply to: ↑ 22 @aristath
4 months 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
4 months 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 about styles beeing loaded in the footer on classic themes.

Last edited 4 months ago by walbo (previous) (diff)

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


4 months ago

#30 @desrosj
4 months ago

  • Keywords commit added

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

#31 @prbot
4 months ago

nosolosw commented on PR #1447:

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
4 months ago

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

#33 @desrosj
4 months 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
4 months ago

  • Keywords dev-feedback fixed-major added

Reopening for backport.

#35 @prbot
4 months ago

desrosj commented on PR #1447:

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

#36 @jorbin
4 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good, ready for backport.

#37 @walbo
4 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening the ticket so the backporting isn't missed.

#38 @desrosj
4 months 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.

Note: See TracTickets for help on using tickets.