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.

Sign up for GitHub

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

Make sure base global styles are loaded before block styles. #5892

Closed
wants to merge 12 commits into from
Closed

Make sure base global styles are loaded before block styles. #5892

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b643d7a
Make sure base global styles are loaded before block styles.
tellthemachines Jan 18, 2024
83367d8
Separate base and per-block global styles when single stylesheet is e…
tellthemachines Jan 22, 2024
00dc2c3
Separate base and per-block custom CSS and enqueue in different places.
tellthemachines Jan 22, 2024
48ce988
Fix lint error.
tellthemachines Jan 22, 2024
be210d6
Fix undefined variable
tellthemachines Jan 22, 2024
aef490b
Remove custom CSS changes and address review feedback
tellthemachines Jan 24, 2024
0a7045a
Remove extra spaces
tellthemachines Jan 24, 2024
bef9311
Make sure global styles only load in the front end.
tellthemachines Jan 25, 2024
9010293
Remove unnecessary filter and function call.
tellthemachines Jan 29, 2024
6d050e0
Split out tests for separate core block assets
tellthemachines Jan 30, 2024
ca1698e
remove redundant test
tellthemachines Jan 30, 2024
1226e21
Fix spacing
tellthemachines Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/wp-includes/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,6 +1200,44 @@ public function get_custom_css() {
return $stylesheet;
}

/**
* Returns the global styles base custom CSS.
*
* @since 6.5.0
*
* @return string The global styles base custom CSS.
*/
public function get_custom_base_css() {
return isset( $this->theme_json['styles']['css'] ) ? $this->theme_json['styles']['css'] : '';
}


/**
* Returns the global styles per-block custom CSS.
*
* @since 6.5.0
*
* @return string The global styles per-block custom CSS.
*/
public function get_custom_block_css() {
$stylesheet = '';

// Add the global styles block CSS.
if ( isset( $this->theme_json['styles']['blocks'] ) ) {
foreach ( $this->theme_json['styles']['blocks'] as $name => $node ) {
$custom_block_css = isset( $this->theme_json['styles']['blocks'][ $name ]['css'] )
? $this->theme_json['styles']['blocks'][ $name ]['css']
: null;
if ( $custom_block_css ) {
$selector = static::$blocks_metadata[ $name ]['selector'];
$stylesheet .= $this->process_blocks_custom_css( $custom_block_css, $selector );
}
}
}

return $stylesheet;
}
Copy link

Choose a reason for hiding this comment

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

The old get_custom_css function should be refactored to call these two to make it clear where the code came from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to pull these changes out and do them in a separate PR.


/**
* Returns the page templates of the active theme.
*
Expand Down
4 changes: 3 additions & 1 deletion src/wp-includes/default-filters.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,9 +596,11 @@
add_filter( 'block_editor_settings_all', 'wp_add_editor_classic_theme_styles' );

// Global styles can be enqueued in both the header and the footer. See https://core.trac.wordpress.org/ticket/53494.
add_action( 'wp_enqueue_scripts', 'wp_enqueue_global_styles' );
add_action( 'init', 'wp_enqueue_global_styles', 1 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to init as it seems to be the only way to get it to run before the core block styles are registered

Copy link
Member

@oandregal oandregal Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

cc @scruffian @ajlende @aristath as they may have the most experience with all the scenarios, IIRC.

I couldn't experiment/test this change because my wordpress-develop env wouldn't start up, so I'm trying to recall from memory / skimming through the code. There are two scenarios: the core blocks styles are loaded with a separate stylesheet or bundled as part of the block library.

As I understand it, the order of styles after this change would be

Single stylesheet

Before:

  1. wp-block-library-css
  2. global-styles-inline-css (both top-level & block-level CSS)
  3. theme-style-css

After

  1. global-styles-inline-css (both top-level & block-level CSS)
  2. wp-block-library-css
  3. theme-style-css

Separate stylesheets

Before:

  1. wp-block-blockname-inline-css (the block styles + the block-level CSS coming from global styles)
  2. wp-block-library-inline-css
  3. global-styles-inline-css (only top-level CSS)
  4. core-block-supports-inline-css

After:

  1. global-styles-inline-css (only top-level CSS)
  2. wp-block-blockname-inline-css (the block styles + the block-level CSS coming from global styles)
  3. wp-block-library-inline-css
  4. core-block-supports-inline-css

Is this the goal / what happens?

Notes:

  • There are some block styles that are absorbed as part of the global styles. I've found some for navigation and pullquote. These are enqueued where the block-level global styles are. I presume block registration and block styles registration happen at different steps of the lifecycle. I mention it because block registration is essential for theme.json/global styles generation, but block styles registration does not affect it.
  • In testing how it works now using a different env, I've noticed that even in the case of "separate stylesheets", the global-styles-inline-css stylesheet has a lot of block styles (tested with TwentyTwentyFour). I may be a bit rusty on how this all is supposed to work, but it sounds like a bug to me. I've found styles for .wp-block-calendar, .wp-block-categories, .wp-block-post-comments-form, .wp-block-loginout, .wp-block-post-terms, .wp-block-query-title, .wp-block-quote, .wp-block-search, .wp-block-separator. Some of them do not have style rules, for example, .wp-block-separator {}.

Copy link
Contributor

@azaozz azaozz Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The change LGTM. Of course "global" styles should always be loaded before "local" styles :)
Was just wondering (nitpick) if using wp_enqueue_scripts priority 1 would be sufficient. As we are enqueueing stylesheets using the appropriate hook would be better for "educational purposes" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking into this!

Was just wondering (nitpick) if using wp_enqueue_scripts priority 1 would be sufficient

I tried and it didn't work, I think because wp_enqueue_scripts always runs after init.

@oandregal the separate stylesheets scenario you outline is correct; I'll need to double-check the single stylesheet but I think it's also right.

Copy link
Member

@oandregal oandregal Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The change LGTM. Of course "global" styles should always be loaded before "local" styles :)

This is incorrect, it lacks a bit of nuance. Styles is a very loaded domain, the same words mean different things to people. It's important to me that we all share the same context, and clarify what we mean.

This is how it works now:

  1. block-level styles provided by the block (the block stylesheet)
  2. global styles (top or block-level) provided by core/blocks/theme/user (different theme.json files & user via global styles sidebar)
  3. block-level styles provided by the user (what we should call "local", these are serialized in the post as classes/style, and they should have the maximum priority)

My understanding is that we want to do this instead:

  1. global styles (top level) provided by core/blocks/theme/user
  2. block-level styles provided by the block (the block stylesheet)
  3. global styles (block-level) provided by core/blocks/theme/user
  4. block-level styles provided by the user

The top-level global styles should be: presets + some styles that are attached to the body + (block style variations?).

I mentioned above that I see still some "block styles" loaded in the "separate stylesheet" scenario (I understand this is a bug): with this change, they'll be enqueued in 1, and can create conflicts because they should be enqueued in 3.

Copy link
Contributor

@azaozz azaozz Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it lacks a bit of nuance. Styles is a very loaded domain, the same words mean different things to people

Yeaaa, can see that too :)

In my mind "global styles" means styles that apply to all similar elements, for example blocks, or image blocks, or paragraph blocks, etc. "Local styles" means styles that apply to only one of many elements. For example only to one image block when there are 5 on the page, etc.

Thinking it would be great to add some explanations in comments at the top of all CSS files in an attempt to get everybody on the same page. Can also be added in docblocks where the CSS is outputted from JS or PHP.

Another idea might be to drop the "global" and "local" terms as inadequate and use something a bit more granular by specificity. Perhaps "Top level" "Level 1", "Level 2", etc.

Copy link
Member

Choose a reason for hiding this comment

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

For reference, this is the documentation we have for styles (naming, how they work, etc.).

Copy link
Contributor Author

@tellthemachines tellthemachines Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK I looked at this carefully and can confirm the following order of styles:

Single stylesheet

trunk

  • wp-block-library-css is loaded as an external stylesheet
  • wp-block-library-inline-css
  • global-styles-inline-css containing base global styles followed by block-specific global styles
  • core-block-supports-inline-css

this PR

  • global-styles-inline-css containing base global styles followed by block-specific global styles
  • wp-block-library-css is loaded as an external stylesheet
  • wp-block-library-inline-css
  • core-block-supports-inline-css

Separate stylesheets

trunk

  • wp-block-[blockname]-inline-css containing both the block library styles for that block and the block-specific global styles
  • in between the block style tags, external stylesheets are loaded for some blocks (e.g. I've seen Navigation, Image and Button, seems they change depending on the combination of blocks on the page)
  • wp-block-library-inline-css (sometimes this is replaced by a block library "common" external stylesheet, no idea why)
  • global-styles-inline-css containing only base global styles
  • core-block-supports-inline-css

this PR

  • global-styles-inline-css containing only base global styles
  • in between style tags, external stylesheets for some blocks
  • wp-block-library-inline-css (sometimes this is replaced by a block library "common" external stylesheet, no idea why)
  • core-block-supports-inline-css

So your assessment is essentially correct @oandregal.

I think the only suboptimal change in this PR is in the single stylesheet scenario, where block library CSS is now loaded after block-specific global styles. It should be the other way around, but ideally base global styles would also be added before block library styles. I'll look into splitting base and block-specific global styles into separate style tags as a workaround.

There are some block styles that are absorbed as part of the global styles. I've found some for navigation and pullquote.

I'm probably missing something, but I don't see those styles appearing at all either on trunk or this PR branch. Tested by adding a Pullquote block to a page and checking that a line-height of 1.6 doesn't appear anywhere in the styles cascade.

I've noticed that even in the case of "separate stylesheets", the global-styles-inline-css stylesheet has a lot of block styles

Those extra block styles added to global-styles-inline-css are coming from per-block custom css defined in the theme's theme.json, e.g. here. TT4 has a bunch of instances.

with this change, they'll be enqueued in 1, and can create conflicts because they should be enqueued in 3

Good point. You mean they should be enqueued after the block library styles, right? I'll look into it.

Copy link
Member

Choose a reason for hiding this comment

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

Those extra block styles added to global-styles-inline-css are coming from per-block custom css defined in the theme's theme.json, e.g. here. TT4 has a bunch of instances.

Oh, thanks for sharing. I think this is a bug: block styles should always be together. Why styles.block/search.css should be processed differently than styles.block/search.typography?

add_action( 'wp_footer', 'wp_enqueue_global_styles', 1 );

add_action( 'wp_enqueue_scripts', 'wp_add_global_styles_for_blocks' );
Copy link
Member

@oandregal oandregal Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't this add the block-level global styles twice to the global-styles-inline-css stylesheet when there's a single stylesheet for blocks?

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 that's a good point, I'll have to test that scenario better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just confirmed and there's no duplication of block-level global styles in the single stylesheet scenario. I removed it from where it was called inside wp_enqueue_global_styles in the script loader, so it's only getting called here, independently of the stylesheet setting.


// Global styles custom CSS.
add_action( 'wp_enqueue_scripts', 'wp_enqueue_global_styles_custom_css' );

Expand Down
80 changes: 79 additions & 1 deletion src/wp-includes/global-styles-and-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,74 @@ function wp_get_global_styles_custom_css() {
return $stylesheet;
}

/**
* Gets the global styles base custom CSS from theme.json.
* Logic should follow wp_get_global_styles_custom_css.
*
* @since 6.5.0
*
* @return string The global base custom CSS.
*/
function wp_get_global_styles_base_custom_css() {
if ( ! wp_theme_has_theme_json() ) {
return '';
}

$can_use_cached = ! wp_is_development_mode( 'theme' );

$cache_key = 'wp_get_global_styles_base_custom_css';
$cache_group = 'theme_json';
if ( $can_use_cached ) {
$cached = wp_cache_get( $cache_key, $cache_group );
if ( $cached ) {
return $cached;
}
}

$tree = WP_Theme_JSON_Resolver::get_merged_data();
$stylesheet = $tree->get_custom_base_css();

if ( $can_use_cached ) {
wp_cache_set( $cache_key, $stylesheet, $cache_group );
}

return $stylesheet;
}

/**
* Gets the global styles per-block custom CSS from theme.json.
* Logic should follow wp_get_global_styles_custom_css.
*
* @since 6.5.0
*
* @return string The global per-block custom CSS.
*/
function wp_get_global_styles_block_custom_css() {
if ( ! wp_theme_has_theme_json() ) {
return '';
}

$can_use_cached = ! wp_is_development_mode( 'theme' );

$cache_key = 'wp_get_global_styles_block_custom_css';
$cache_group = 'theme_json';
if ( $can_use_cached ) {
$cached = wp_cache_get( $cache_key, $cache_group );
if ( $cached ) {
return $cached;
}
}

$tree = WP_Theme_JSON_Resolver::get_merged_data();
$stylesheet = $tree->get_custom_block_css();

if ( $can_use_cached ) {
wp_cache_set( $cache_key, $stylesheet, $cache_group );
}

return $stylesheet;
}

/**
* Adds global style rules to the inline style for each block.
*
Expand All @@ -302,11 +370,15 @@ function wp_get_global_styles_custom_css() {
function wp_add_global_styles_for_blocks() {
$tree = WP_Theme_JSON_Resolver::get_merged_data();
$block_nodes = $tree->get_styles_block_nodes();

if ( ! wp_should_load_separate_core_block_assets() ) {
wp_register_style( 'global-styles-blocks', false );
}
foreach ( $block_nodes as $metadata ) {
$block_css = $tree->get_styles_for_block( $metadata );

if ( ! wp_should_load_separate_core_block_assets() ) {
wp_add_inline_style( 'global-styles', $block_css );
wp_add_inline_style( 'global-styles-blocks', $block_css );
continue;
}

Expand Down Expand Up @@ -336,6 +408,10 @@ function wp_add_global_styles_for_blocks() {
}
}
}

if ( ! wp_should_load_separate_core_block_assets() ) {
wp_enqueue_style( 'global-styles-blocks' );
}
Copy link

Choose a reason for hiding this comment

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

I'm not sure if this should be enqueued here (or registered above) as it's a rather large change for a public function that just used to add styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm good point, I'll see if I can find a more appropriate place.

}

/**
Expand Down Expand Up @@ -431,6 +507,8 @@ function wp_clean_theme_json_cache() {
wp_cache_delete( 'wp_get_global_settings_custom', 'theme_json' );
wp_cache_delete( 'wp_get_global_settings_theme', 'theme_json' );
wp_cache_delete( 'wp_get_global_styles_custom_css', 'theme_json' );
wp_cache_delete( 'wp_get_global_styles_base_custom_css', 'theme_json' );
wp_cache_delete( 'wp_get_global_styles_block_custom_css', 'theme_json' );
wp_cache_delete( 'wp_get_theme_data_template_parts', 'theme_json' );
WP_Theme_JSON_Resolver::clean_cached_data();
}
Expand Down
13 changes: 9 additions & 4 deletions src/wp-includes/script-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2512,9 +2512,6 @@ function wp_enqueue_global_styles() {
wp_register_style( 'global-styles', false );
wp_add_inline_style( 'global-styles', $stylesheet );
wp_enqueue_style( 'global-styles' );

// Add each block as an inline css.
wp_add_global_styles_for_blocks();
}

/**
Expand All @@ -2531,13 +2528,21 @@ function wp_enqueue_global_styles_custom_css() {
remove_action( 'wp_head', 'wp_custom_css_cb', 101 );

$custom_css = wp_get_custom_css();
$custom_css .= wp_get_global_styles_custom_css();
$custom_css .= wp_get_global_styles_base_custom_css();

if ( ! empty( $custom_css ) ) {
wp_add_inline_style( 'global-styles', $custom_css );
}
$block_custom_css = wp_get_global_styles_block_custom_css();

if ( ! empty( $block_custom_css ) ) {
wp_register_style( 'global-styles-block-custom', false );
wp_add_inline_style( 'global-styles-block-custom', $block_custom_css );
wp_enqueue_style( 'global-styles-block-custom' );
}
}


/**
* Checks if the editor scripts and styles for all registered block types
* should be enqueued on the current screen.
Expand Down
Loading