#53494 closed defect (bug) (fixed)
Styles not applied to block using theme.json with seperate assets loading enabled
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| 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:
- Create a block:
npx @wordpress/create-block wp-test-block. - Install the plugin created from step one. No customizations is required.
- Insert your new test block into a post and publish it.
- Create a
theme.jsonin 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"
}
}
}
}
}
- Visit your post and confirm the background color of your block is changed to
#333. - Enable lazy loading by adding
add_filter( 'should_load_separate_core_block_assets', '__return_true' ) - Revisit your post and the background color from
theme.jsonis no longer applied.
Attachments (2)
Change History (46)
#1
@
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
@
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
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
#7
@
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
@
3 years ago
Hey, looked a bit at this issue and this is what I find:
- Classic themes:
- styles load in the
<head>withshould_load_separate_core_block_assetsdisabled - styles load at the bottom of the
<body>withshould_load_separate_core_block_assetsenabled
- styles load in the
- FSE themes: styles always load in the head both with and without
should_load_separate_core_block_assetsenabled.
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_contentis pre-rendered for classic themes that haveshould_load_separate_core_block_assetsenabled 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.
#9
@
3 years ago
can we make it so that the
post_contentis pre-rendered for classic themes that haveshould_load_separate_core_block_assetsenabled 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
@
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.
#13
@
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.
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:
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
This ticket was mentioned in PR #1447 on WordPress/wordpress-develop by desrosj.
3 years ago
#17
Trac ticket: https://core.trac.wordpress.org/ticket/53494
This ticket was mentioned in Slack in #core by desrosj. View the logs.
3 years ago
#20
@
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
@
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:
↓ 27
@
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
@
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
@
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. 👍
3 years ago
#26
Going close this one in favor of https://github.com/WordPress/wordpress-develop/pull/1447
#27
in reply to:
↑ 22
@
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_assetsdoes 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
@
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 about styles beeing loaded in the footer on classic themes.
This ticket was mentioned in Slack in #core-editor by nosolosw. View the logs.
3 years ago
#30
@
3 years ago
- Keywords commit added
Marking for commit pending a final review of the PR from another contributor.
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.
3 years ago
#35
Merged into Core in https://core.trac.wordpress.org/changeset/51309. Thanks everyone!
#37
@
3 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening the ticket so the backporting isn't missed.
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-cssembedded 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 theglobal-styles-inline-cssembedded 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 itsfunctions.php. - Load the site and verify that the
global-styles-inline-cssis loaded at the bottom of the<body>.
#40
@
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
#42
@
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.
#43
@
3 years ago
Ok, created https://core.trac.wordpress.org/ticket/55148 I'm not able to assign the 5.9.1 milestone, though.
3 years ago
#44
Committed in https://core.trac.wordpress.org/changeset/52738
Moving to 5.8 for investigation and consideration as it relates to new functionality introduced there.