-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Enqueue block custom CSS only when block renders on the page. #58991
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/class-wp-theme-json-gutenberg.php ❔ lib/global-styles-and-settings.php ❔ lib/script-loader.php |
Flaky tests detected in 7424bb0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7954307106
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I rebased on trunk and it's stopped. Must've been me. Sorry! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a drive by test.
This change makes sense to me, and it working pretty well.
I can confirm that custom block CSS is only loaded if that block appears on the page in the frontend.
add_filter( 'should_load_separate_core_block_assets', '__return_false', 11 );
makes it load all the time. 🌮
Not sure if it needs a rebase - I was seeing some strange behaviour in site editor, but my env probably needs nuking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks!
I've tested a few scenarios (how posts with and without quote block behave in a block theme, by default and with the filter disabled) and all work as expected. I'm looking at the code in a bit.
Is it feasible to deprecate As a consumer, if I use This is a bug that has been present in WordPress since the |
It should be, yeah! I'm not sure why it wasn't done that way to start with. One thing I noticed is that the customizer CSS is being enqueued as part of |
Ok so the reason for the current system is explained in #47396: we're de-enqueuing the customizer additional CSS and adding them before global styles custom CSS so that they don't override global styles in hybrid themes. The reason for customizer styles being attached to Hybrid themes are a bit of an edge case, but in order to preserve back compat it would be good to keep the current order of customizer additional CSS followed by global styles custom CSS. I don't think any of this is a blocker to enqueuing custom CSS as part of |
OK I think all feedback has been addressed now! I moved the enqueuing logic into The order of loaded styles should still be the same:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tellthemachines Sorry, it's easier to track commits rather than PRs so I only caught this late. Are you able to follow up?
I'm not sure how well the docs-parser copes with the comment format but better to play nice :)
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Double line break
|
||
$stylesheet_handle = 'global-styles'; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be /*
.
A double asterisk indicates that comments are a docblock to the various parsers for developer.wordpress.org and elsewhere.
@@ -53,6 +53,35 @@ function gutenberg_enqueue_global_styles() { | |||
|
|||
// Add each block as an inline css. | |||
gutenberg_add_global_styles_for_blocks(); | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above re docblock/comment.
* Before that, dequeue the Customizer's custom CSS | ||
* and add it before the global styles custom CSS. | ||
*/ | ||
// Don't enqueue Customizer's custom CSS separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be move in to the existing multiline comment
$custom_css = wp_get_custom_css(); | ||
|
||
if ( ! wp_should_load_separate_core_block_assets() ) { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment v docblock, as above.
@@ -77,8 +107,6 @@ function gutenberg_enqueue_global_styles_custom_css() { | |||
wp_add_inline_style( 'global-styles', $custom_css ); | |||
} | |||
} | |||
remove_action( 'wp_enqueue_scripts', 'wp_enqueue_global_styles_custom_css' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we still need to remove the wp_enqueue_global_styles_custom_css
callback?
The plugin supports the latest two WordPress versions, so, assuming this is backported for 6.6, it's only when WordPress 6.7 is released that we can remove this (at that point, the plugin can assume the minimum version is 6.6, where that function is no longer relevant).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooh good point, it'll still be in core until 6.6 so yeah, I guess it'll need removing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually checking the CSS output on the front end, I'd expect any custom CSS styles to be duplicated with this change (because wp_enqueue_global_styles_custom_css
is still being enqueued in core) but they're not 🤔 I wonder why?
@@ -63,6 +92,7 @@ function gutenberg_enqueue_global_styles() { | |||
* @since 6.2.0 | |||
*/ | |||
function gutenberg_enqueue_global_styles_custom_css() { | |||
_deprecated_function( __FUNCTION__, 'Gutenberg 17.8.0', 'gutenberg_enqueue_global_styles' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume we also need to add a @deprecated 6.6 Use wp_enqueue_global_styles instead
PHP doc, or something along those lines.
* | ||
* @return string The global base custom CSS. | ||
*/ | ||
function gutenberg_get_global_styles_base_custom_css() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify further and remove this function and gutenberg_get_global_styles_custom_css
in favor of gutenberg_get_global_stylesheet
? The styles.custom
property is still the only one that has a dedicated function.
@@ -1284,21 +1284,68 @@ protected function process_blocks_custom_css( $css, $selector ) { | |||
* @return string The global styles custom CSS. | |||
*/ | |||
public function get_custom_css() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as below: can we deprecate/remove get_custom_css
, get_base_custom_css
, and get_block_custom_css_nodes
, get_block_custom_css
, in favor of the generic functions? IMO, it's too much code for a single style property that should behave like any other (and be part of the general algorithm).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point! It would be better if we didn't need as many functions, but I don't know why it was done this way in the first place. I'll look into it as a follow-up, thanks for reviewing!
@tellthemachines Could you include this change with your backport PR? Added @return after @global in php doc |
@ellatrix I'm hoping to have time to address André's feedback and refactor these functions before the backport. If I'm unable to do it in time, we could also hold off on adding this to core for now because it's not an essential piece of work, more of a cleanup. |
What?
Follow up from WordPress/wordpress-develop#5892.
Global styles per-block custom CSS should respect the
wp_should_load_separate_core_block_assets
flag and only be enqueued when the block renders on the page if separate assets are enabled.Ideally this work would be done core-first, but given we're currently in Beta it will have to wait a while, so it's worth trying out here in the meantime.
Testing Instructions
<style id='global-styles-inline-css'>
)add_filter( 'should_load_separate_core_block_assets', '__return_false', 11 );
to the theme'sfunctions.php
and check that custom CSS for all blocks always loads in the front end.Testing Instructions for Keyboard
Screenshots or screencast