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

tellthemachines · 2024-01-18T05:49:42Z

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

Not sure if this is the best approach so would appreciate feedback on that! I'm seeing some weirdness in the editor where the theme heading font family is overriding the default editor one, which is a bit unexpected:

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

tellthemachines · 2024-01-18T05:52:32Z

src/wp-includes/default-filters.php

@@ -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 );


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

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:

wp-block-library-css

global-styles-inline-css (both top-level & block-level CSS)

theme-style-css

After

global-styles-inline-css (both top-level & block-level CSS)

wp-block-library-css

theme-style-css

Separate stylesheets

Before:

wp-block-blockname-inline-css (the block styles + the block-level CSS coming from global styles)

wp-block-library-inline-css

global-styles-inline-css (only top-level CSS)

core-block-supports-inline-css

After:

global-styles-inline-css (only top-level CSS)

wp-block-blockname-inline-css (the block styles + the block-level CSS coming from global styles)

wp-block-library-inline-css

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 {}.

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" :)

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.

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:

block-level styles provided by the block (the block stylesheet)

global styles (top or block-level) provided by core/blocks/theme/user (different theme.json files & user via global styles sidebar)

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:

global styles (top level) provided by core/blocks/theme/user

block-level styles provided by the block (the block stylesheet)

global styles (block-level) provided by core/blocks/theme/user

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.

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.

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

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.

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?

github-actions · 2024-01-18T06:06:34Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

oandregal · 2024-01-18T19:32:18Z

src/wp-includes/default-filters.php

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

+add_action( 'wp_enqueue_scripts', 'wp_add_global_styles_for_blocks' );


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?

Oh that's a good point, I'll have to test that scenario better.

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.

…nabled.

tellthemachines · 2024-01-22T06:50:55Z

OK I think I've addressed the issues mention above:

Split base and block-specific global styles into separate style tags and ordered them so that block-specific global styles come after block library styles when should_load_separate_core_block_assets is false.
Split base and block-specific custom CSS and ordered them so base is attached to base global styles, and block-specific comes after the block global styles. It's still not awesome that the CSS for all blocks will always be enqueued - maybe should look into following the logic for block global styles when should_load_separate_core_block_assets is true.

Looking into the test failures now, they look legit so there's a good chance I messed up somewhere 😅

oandregal · 2024-01-22T11:37:54Z

I was able to test this and gave it more thought. I've got two concerns:

The top-level global styles contain styles coming from WordPress, the theme, and/or the user. These should not come after block styles. Not for 6.5, but I think we should explore absorving block styles as part of the global styles algorithm, instead. See Iterate on absorbing block styles as part of the global styles mechanism gutenberg#45198
From a practical point of view, the top-level global stylesheet contains rules for elements:

h1, h2, h3, ...
.wp-element-button:focus,
.wp-block-button__link:focus

These can easily conflict with styles coming in block stylesheets. If top-level global styles only had body and preset classes, I'd feel more confident about this approach (though, presets classes can conflict with some old presets that exist in block-library for backward compatibility).

I'd like to provide an alternative. My understanding is that the goal is:

top-level global styles come before block-level global styles
all block-level global styles are bundled together (no separation between styles.[block-name].css and styles.[block-name].typograpy) (see):

Can we achieve this by updating the code around wp_add_global_styles_for_blocks, so block-level global styles are always loaded as part of the global stylesheet but we only bundle those in use if should_load_separate_block_assets is true?

AFAIK, @scruffian @ajlende @aristath are the ones who implemented/worked the most with this (elements, the filter for blocks, etc.), so it'd be great if they provided feedback as well, in case I'm missing something.

tellthemachines · 2024-01-23T04:36:06Z

Thanks for the feedback!

top-level global styles come before block-level global styles

all block-level global styles are bundled together (no separation between styles.[block-name].css and styles.[block-name].typograpy) (see):

Can we achieve this by updating the code around wp_add_global_styles_for_blocks, so block-level global styles are always loaded as part of the global stylesheet but we only bundle those in use if should_load_separate_block_assets is true?

What about core block-library styles for each block? I think they should come after top-level global styles, but if we bundle block-level global styles with the top-level global stylesheet, then at best the order will be:

top-level global styles
block-level global styles
block-library block styles

which I don't think is ideal because themes might want to override some of the block-library styles with global styles.
I think ideally the order would be

top-level global styles
block-library block styles
block-level global styles

which is what we have in this PR.

In any case, I tried merging block and top-level global styles into the same stylesheet by changing the relevant part of wp_add_global_styles_for_blocks to something like:

add_filter(
            'render_block',
            static function ( $html, $block ) use ( $metadata, $block_css ) {
                if ( isset( $metadata['name'] ) ) {
                    wp_add_inline_style( 'global-styles', $block_css );
                }
                // The likes of block element styles from theme.json do not have  $metadata['name'] set.
                if ( ! isset( $metadata['name'] ) && ! empty( $metadata['path'] ) ) {
                    $block_name = wp_get_block_name_from_theme_json_path( $metadata['path'] );
                    if ( $block_name ) {
                        wp_add_inline_style( 'global-styles', $block_css );
                    }
                }
                return $html;
            },
            10,
            2
        );

based on the block asset enqueuing logic, but isn't working locally for me, not sure why. I'd appreciate any pointers to what I'm doing wrong here 😅

Totally agree with merging block custom CSS with the rest of the block global styles though. I'll give that a go next.

tellthemachines · 2024-01-23T05:14:43Z

Totally agree with merging block custom CSS with the rest of the block global styles though. I'll give that a go next.

Actually, given that the order of custom CSS isn't really relevant for the main cause of this PR, which is unblocking a specificity reduction in global styles, it might be best to pull those changes out and tackle them as a separate ticket/PR. I'm happy to look into it myself, but my first priority now is the specificity reduction work.

ajlende

The ordering is looking okay for me, but it's hard to foresee every impact this will have.

I was concerned about duotone styles with the stylesheets moving around. Duotone is injecting itself directly into the global-styles-inline-css in order to tally which filters are being used before loading them on the page, and I wasn't sure if moving things around would have affected that, but it seems to be fine.

ajlende · 2024-01-23T23:07:37Z

src/wp-includes/class-wp-theme-json.php

+	/**
+	 * 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;
+	}


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

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

ajlende · 2024-01-23T23:51:16Z

src/wp-includes/global-styles-and-settings.php

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


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.

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

tellthemachines · 2024-01-25T05:57:54Z

Update: I've worked out why the global styles are interfering with editor chrome styling on this branch: enqueuing base global styles on init means they get added in the editor as well as the front end, whereas using wp_enqueue_scripts as they are on trunk they only get added in the front end 🤦

The goal of enqueuing global styles on init was to get them on the page before the block-specific styles were enqueued, which happens when the blocks render. The block-specific styles have to be attached to block render so that we can load them only when the block is present. I'm not sure that there's any other way of loading styles conditionally - at least I can't think of any, but keen to hear ideas anyone has on this!

I guess we can try to only enqueue global styles on init in the front end, but I can't see how to achieve @oandregal 's goal of

Can we achieve this by updating the code around wp_add_global_styles_for_blocks, so block-level global styles are always loaded as part of the global stylesheet but we only bundle those in use if should_load_separate_block_assets is true?

Again, ideas welcome! I'm also curious to know if/how the work in WordPress/gutenberg#45198 is taking conditional loading into account.

tellthemachines · 2024-01-31T04:48:08Z

Ok I've fixed the tests and I think this is ready for further reviews now. The main point I'm looking for feedback on is whether is is acceptable to have base global styles loaded before core block-library styles.

Given the way that the conditional loading of block styles is coupled with render_block, I don't see much in the way of alternatives, but ideas are very welcome!

tellthemachines · 2024-02-07T04:38:06Z

Thanks for the discussion folks! Closing this one in favour of #6010.

Make sure base global styles are loaded before block styles.

b643d7a

tellthemachines requested review from desrosj, oandregal, gziolo, azaozz and felixarntz January 18, 2024 05:50

tellthemachines commented Jan 18, 2024

View reviewed changes

tellthemachines mentioned this pull request Jan 18, 2024

Try reducing specificity of global styles block selector. WordPress/gutenberg#57841

Closed

oandregal reviewed Jan 18, 2024

View reviewed changes

tellthemachines added 4 commits January 22, 2024 15:54

Separate base and per-block global styles when single stylesheet is e…

83367d8

…nabled.

Separate base and per-block custom CSS and enqueue in different places.

00dc2c3

Fix lint error.

48ce988

Fix undefined variable

be210d6

ajlende reviewed Jan 23, 2024

View reviewed changes

tellthemachines added 2 commits January 24, 2024 16:27

Remove custom CSS changes and address review feedback

aef490b

Remove extra spaces

0a7045a

tellthemachines added 5 commits January 25, 2024 17:07

Make sure global styles only load in the front end.

bef9311

Remove unnecessary filter and function call.

9010293

Split out tests for separate core block assets

6d050e0

remove redundant test

ca1698e

Fix spacing

1226e21

tellthemachines marked this pull request as ready for review January 31, 2024 04:45

tellthemachines mentioned this pull request Feb 2, 2024

Always output core block global styles after base global styles. #6010

Closed

tellthemachines closed this Feb 7, 2024

tellthemachines mentioned this pull request Feb 14, 2024

Enqueue block custom CSS only when block renders on the page. WordPress/gutenberg#58991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tellthemachines commented Jan 18, 2024

tellthemachines Jan 18, 2024

oandregal Jan 18, 2024 •

edited

Loading

azaozz Jan 18, 2024 •

edited

Loading

tellthemachines Jan 19, 2024

oandregal Jan 19, 2024 •

edited

Loading

azaozz Jan 19, 2024 •

edited

Loading

oandregal Jan 19, 2024

tellthemachines Jan 22, 2024 •

edited

Loading

oandregal Jan 22, 2024

github-actions bot commented Jan 18, 2024

oandregal Jan 18, 2024 •

edited

Loading

tellthemachines Jan 19, 2024

tellthemachines Jan 22, 2024

tellthemachines commented Jan 22, 2024

oandregal commented Jan 22, 2024

tellthemachines commented Jan 23, 2024

tellthemachines commented Jan 23, 2024

ajlende left a comment

ajlende Jan 23, 2024

tellthemachines Jan 24, 2024

ajlende Jan 23, 2024

tellthemachines Jan 24, 2024

tellthemachines commented Jan 25, 2024

tellthemachines commented Jan 31, 2024

tellthemachines commented Feb 7, 2024

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

		add_action( 'wp_enqueue_scripts', 'wp_add_global_styles_for_blocks' );

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

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

Conversation

tellthemachines commented Jan 18, 2024

Choose a reason for hiding this comment

oandregal Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Single stylesheet

Separate stylesheets

azaozz Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

azaozz Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Single stylesheet

trunk

this PR

Separate stylesheets

trunk

this PR

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2024

Test using WordPress Playground

Some things to be aware of

oandregal Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines commented Jan 22, 2024

oandregal commented Jan 22, 2024

tellthemachines commented Jan 23, 2024

tellthemachines commented Jan 23, 2024

ajlende left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines commented Jan 25, 2024

tellthemachines commented Jan 31, 2024

tellthemachines commented Feb 7, 2024

oandregal Jan 18, 2024 •

edited

Loading

azaozz Jan 18, 2024 •

edited

Loading

oandregal Jan 19, 2024 •

edited

Loading

azaozz Jan 19, 2024 •

edited

Loading

tellthemachines Jan 22, 2024 •

edited

Loading

oandregal Jan 18, 2024 •

edited

Loading