Make WordPress Core

Opened 3 months ago

Closed 2 months ago

Last modified 2 months ago

#60280 closed defect (bug) (fixed)

Ensure base global styles are loaded before block styles.

Reported by: isabel_brison's profile isabel_brison Owned by: isabel_brison's profile isabel_brison
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Script Loader Keywords: has-patch
Focuses: css Cc:

Description

When should_load_separate_core_block_assets is true, block-library stylesheets for each block are enqueued separately, so they only get loaded if the block is on the page.

Global styles are enqueued later on, and any block-specific global styles are appended to the above block stylesheets, so they end up sitting above base global styles in the CSS cascade.

Base global styles should instead be loaded before block styles (or at least before global block styles, so they are applied in the correct order.

Having the correct style loading order in place is essential to reduce the specificity of block global styles, in progress in https://github.com/WordPress/gutenberg/pull/57841. This in turn will allow section styles: https://github.com/WordPress/gutenberg/issues/57537 to be generated with sustainable levels of specificity.

Change History (14)

This ticket was mentioned in PR #5892 on WordPress/wordpress-develop by @isabel_brison.


3 months ago
#1

  • Keywords has-patch added

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:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/8096000/e9c548b4-c4cd-4035-8d00-a7b4b995d3ec

#2 @azaozz
3 months ago

  • Type changed from enhancement to defect (bug)

Base global styles should instead be loaded before block styles

+1. The "global" stylesheets should always go before the "local" stylesheets, no matter what. Makes everything down the line simpler. I'd consider this a bug, not an enhancement.

@isabel_brison commented on PR #5892:


3 months ago
#3

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 commented on PR #5892:


3 months ago
#4

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 https://github.com/WordPress/gutenberg/issues/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.[https://github.com/WordPress/wordpress-develop/pull/5892#discussion_r1461723298 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.

@isabel_brison commented on PR #5892:


3 months ago
#5

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.[https://github.com/WordPress/wordpress-develop/pull/5892#discussion_r1461723298 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 changingthe 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.

@isabel_brison commented on PR #5892:


3 months ago
#6

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.

#7 @poena
3 months ago

I have tested this to the best of my ability but I am still not sure that I am testing the correct thing.

Twenty Twenty-One: Classic theme without theme.json, that enqueue additional CSS for some blocks: I saw no change in the block style.

Twenty Twenty: same as Twenty Twenty-One.

Astra: Classic theme that uses theme.json. The styles in theme.json are very limited, for example it changes the underline on elements.links, but then still overrides this with some custom CSS.
I saw no change in the block style.

---
Reading the discussion, it sounds like the plan is to change which stylesheets contain what CSS:Further testing needs to be done to ensure that themes and plugins can still enqueu but also deque, remove, the CSS.

@isabel_brison commented on PR #5892:


3 months ago
#8

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 https://github.com/WordPress/gutenberg/issues/45198 is taking conditional loading into account.

@isabel_brison commented on PR #5892:


3 months ago
#9

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!

This ticket was mentioned in PR #6010 on WordPress/wordpress-develop by @isabel_brison.


3 months ago
#10

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

Alternative to #5892.

Given that it's undesirable to have base global styles load before block-library styles, this PR changes just the block global styles so they load after the base global styles.

The order of loading should now be:

  • Block library styles
  • Base global styles (element styles, etc)
  • Block-specific global styles

This order should be the same regardless of whether should_load_separate_core_block_assets is true or false.

This addresses part of @oandregal's feedback here. It doesn't touch on custom CSS yet. To fix the existing bug and keep this change small, I'll address the custom CSS separately (it's not really the same issue, because the _order_ of custom CSS is fine, the problem is that it's always loading whether the target blocks are on the page or not)

@oandregal commented on PR #6010:


2 months ago
#11

OK, I had to jump through some hoops but I finally created a test case for this, was able to reproduce it, and this works nicely. Thanks for keep trying!

This is how I reproduce:

  • Using the TwentyTwentyFour theme, with default configuration.
  • Created two posts. One with the quote block and one without. Publish them.
  • Verify that:
    • the post with the quote has the quote styles as part of the global-styles-inline-css stylesheet while the other doesn't.
    • the wp-block-quote-inline-css stylesheet doesn't have the block-level styles that are now part of global-styles-inline-css.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/583546/a3d846df-fcc7-4ae5-8af0-42902cf689cf

  • Go to the functions.php file of the theme and add the following:
add_filter( 'should_load_separate_core_block_assets', '__return_false', 11 );
  • Reload the two posts and verify that
    • both posts have the quote styles as part of the global-styles-inline-css stylesheet.
    • the wp-block-quote-inline-css stylesheet doesn't have the block-level styles that are now part of global-styles-inline-css.

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/583546/c2e39645-93f5-44f0-b67b-5dc736df0b9d

#12 @isabel_brison
2 months ago

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

In 57546:

Script Loader: always output core block global styles after base global styles.

Changes the output of core block global styles when should_load_separate_core_block_assets is true so they are appended to base global styles instead of block-library styles.

Props isabel_brison, oandregal, azaozz, ajlende.
Fixes #60280.

@isabel_brison commented on PR #6010:


2 months ago
#13

Thanks for reviewing @oandregal ! Committed in r57546.

@isabel_brison commented on PR #5892:


2 months ago
#14

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

Note: See TracTickets for help on using tickets.