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

Global Styles: Load block CSS conditionally #41160

Merged
merged 5 commits into from
May 20, 2022
Merged

Global Styles: Load block CSS conditionally #41160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d016833
Global Styles: Load block styles as part of the block CSS
scruffian May 19, 2022
933bc2a
Global Styles: Load block styles as part of the block CSS
scruffian May 19, 2022
92ac01d
tidy up
scruffian May 19, 2022
9a95640
fix PHP linting
scruffian May 20, 2022
9475a8b
add comments
scruffian May 20, 2022
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
128 changes: 128 additions & 0 deletions lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,132 @@ protected static function get_blocks_metadata() {

return static::$blocks_metadata;
}

/**
* Builds metadata for the style nodes, which returns in the form of:
*
* [
* [
* 'path' => [ 'path', 'to', 'some', 'node' ],
* 'selector' => 'CSS selector for some node',
* 'duotone' => 'CSS selector for duotone for some node'
* ],
* [
* 'path' => ['path', 'to', 'other', 'node' ],
* 'selector' => 'CSS selector for other node',
* 'duotone' => null
* ],
* ]
*
* @since 5.8.0
*
* @param array $theme_json The tree to extract style nodes from.
* @return array
*/
protected static function get_style_nodes( $theme_json ) {
$nodes = array();
if ( ! isset( $theme_json['styles'] ) ) {
return $nodes;
}

// Top-level.
$nodes[] = array(
'path' => array( 'styles' ),
'selector' => static::ROOT_BLOCK_SELECTOR,
);

if ( isset( $theme_json['styles']['elements'] ) ) {
foreach ( $theme_json['styles']['elements'] as $element => $node ) {
$nodes[] = array(
'path' => array( 'styles', 'elements', $element ),
'selector' => static::ELEMENTS[ $element ],
Copy link
Contributor

Choose a reason for hiding this comment

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

In unrelated testing in Core, I'm seeing notices coming from here:

Notice: Undefined index: button in /var/www/html/wp-includes/class-wp-theme-json.php on line 1482

@scruffian: can someone follow up here to not assume $element exists on the table? Otherwise this will happen whenever someone is using a recent theme with a less recent Core installation, for example.

Copy link
Member

Choose a reason for hiding this comment

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

I've run into similar things a few times (here and here).

In Core, with Gutenberg activated, this line calls get_block_editor_settings() which calls wp_get_global_stylesheet(), and the subsequent chain of methods.

<b>Notice</b>:  Undefined index: button in <b>/var/www/src/wp-includes/class-wp-theme-json.php</b> on line <b>1489</b><br />
#0  WP_Theme_JSON::get_style_nodes() called at [/var/www/src/wp-includes/class-wp-theme-json.php:712]
#1  WP_Theme_JSON->get_stylesheet() called at [/var/www/src/wp-includes/global-styles-and-settings.php:140]
#2  wp_get_global_stylesheet()) called at [/var/www/src/wp-includes/block-editor.php:404]
#3  get_block_editor_settings() called at [/var/www/src/wp-admin/post.php:187]

So it looks like it's running the Core methods/classes which may not have the latest checks ?

I've added a check in Gutenberg:

I tried to find an "in-plugin" work around and discovered the following:

Removing the call to WP_Theme_JSON_Resolver_Gutenberg::get_merged_data() in gutenberg_register_webfonts_from_theme_json removes the error. I don't know why 🤷

The WP_Theme_JSON constructor is called twice where static:ELEMENTS refers to the Core const, that is, without the new elements button et. al., from the WP_Theme_JSON_Gutenberg class.

This is why we see the error where theme.json contains unsupported elements.

If, in the constructor, we referred to self::ELEMENTS we'd avoid this error, but it'd also mean we'd have to overwrite the constructor every time ELEMENTS was updated.

Copy link
Contributor

@draganescu draganescu Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If, in the constructor, we referred to self::ELEMENTS we'd avoid this error, but it'd also mean we'd have to overwrite the constructor every time ELEMENTS was updated.

This looks like the way to go. Some copy pasta is a small price to pay for better consistency of how things work!

Also @ramonjd does #43622 solve @mcsf 's potential issue with recent themes/older core - starting WP 6.1?

);
}
}

// Blocks.
if ( ! isset( $theme_json['styles']['blocks'] ) ) {
return $nodes;
}

$nodes = array_merge( $nodes, static::get_block_nodes( $theme_json ) );

// This filter allows us to modify the output of WP_Theme_JSON so that we can do things like loading block CSS independently.
return apply_filters( 'gutenberg_get_style_nodes', $nodes );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is copied from the parent class except this line and the one above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we filter out filter_out_block_nodes later and not right here? Since we can override the entire function can't we just change it?

}

/**
* A public helper to get the block nodes from a theme.json file.
*
* @return array The block nodes in theme.json.
*/
public function get_styles_block_nodes() {
return static::get_block_nodes( $this->theme_json );
}

/**
* An internal method to get the block nodes from a theme.json file.
*
* @param array $theme_json The theme.json converted to an array.
*
* @return array The block nodes in theme.json.
*/
private static function get_block_nodes( $theme_json ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't new code, it's just extracted to a new function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is it extracted from? This PR doesn't delete any code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in the definition of get_style_nodes in the parent class.

$selectors = static::get_blocks_metadata();
$nodes = array();
if ( ! isset( $theme_json['styles'] ) ) {
return $nodes;
}

// Blocks.
if ( ! isset( $theme_json['styles']['blocks'] ) ) {
return $nodes;
}

foreach ( $theme_json['styles']['blocks'] as $name => $node ) {
$selector = null;
if ( isset( $selectors[ $name ]['selector'] ) ) {
$selector = $selectors[ $name ]['selector'];
}

$duotone_selector = null;
if ( isset( $selectors[ $name ]['duotone'] ) ) {
$duotone_selector = $selectors[ $name ]['duotone'];
}

$nodes[] = array(
'name' => $name,
'path' => array( 'styles', 'blocks', $name ),
'selector' => $selector,
'duotone' => $duotone_selector,
);

if ( isset( $theme_json['styles']['blocks'][ $name ]['elements'] ) ) {
foreach ( $theme_json['styles']['blocks'][ $name ]['elements'] as $element => $node ) {
$nodes[] = array(
'path' => array( 'styles', 'blocks', $name, 'elements', $element ),
'selector' => $selectors[ $name ]['elements'][ $element ],
);
}
}
}

return $nodes;
}

/**
* Gets the CSS rules for a particular block from theme.json.
*
* @param array $block_metadata Meta data about the block to get styles for.
*
* @return array Styles for the block.
*/
public function get_styles_for_block( $block_metadata ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new function gets styles for the given block.

$node = _wp_array_get( $this->theme_json, $block_metadata['path'], array() );
$selector = $block_metadata['selector'];
$settings = _wp_array_get( $this->theme_json, array( 'settings' ) );
$declarations = static::compute_style_properties( $node, $settings );
$block_rules = static::to_ruleset( $selector, $declarations );
return $block_rules;
}
}
23 changes: 23 additions & 0 deletions lib/compat/wordpress-6.1/get-global-styles-and-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
/**
* API to interact with global settings & styles.
*
* @package gutenberg
*/

/**
* Adds global style rules to the inline style for each block.
*/
function wp_add_global_styles_for_blocks() {
$tree = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data();
// TODO some nodes dont have a name...
$block_nodes = $tree->get_styles_block_nodes();
foreach ( $block_nodes as $metadata ) {
$block_css = $tree->get_styles_for_block( $metadata );
$block_name = str_replace( 'core/', '', $metadata['name'] );
// These block styles are added on block_render.
// This hooks inline CSS to them so that they are loaded conditionally
// based on whether or not the block is used on the page.
wp_add_inline_style( 'wp-block-' . $block_name, $block_css );
Copy link
Contributor

Choose a reason for hiding this comment

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

Took me a while to figure out that the inline style exists at this stage and this only adds to the 'wp-block-' . $block_name handler name :) These block inline styles are added on block_render and this is how we only get the ones that are actually on page, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment to that effect here as it took me a while to figure out as well.

}
}
83 changes: 83 additions & 0 deletions lib/compat/wordpress-6.1/script-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
<?php
/**
* Load global styles assets in the front-end.
*
* @package gutenberg
*/

/**
* This applies a filter to the list of style nodes that comes from `get_style_nodes` in WP_Theme_JSON.
* This particular filter removes all of the blocks from the array.
*
* We want WP_Theme_JSON to be ignorant of the implementation details of how the CSS is being used.
* This filter allows us to modify the output of WP_Theme_JSON depending on whether or not we are loading separate assets,
* without making the class aware of that detail.
*
* @since 6.1
*
* @param array $nodes The nodes to filter.
* @return array A filtered array of style nodes.
*/
function filter_out_block_nodes( $nodes ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is merged we won't need this anymore, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need it in a filter for the case where loading separate assets is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the script loader filters the output of the class that handles "processing of structures that adhere to the theme.json spec". Why not instantiate with a config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The want the WP_Theme_Json class to be ignorant of the implementation details of how the CSS is being used - so passing a config which determines whether or not to include blocks goes against that principle.

There's more context here: #40513 (review)

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave a note in the code here so that it's immediately clear to future readers.

return array_filter(
$nodes,
function( $node ) {
return ! in_array( 'blocks', $node['path'], true );
},
ARRAY_FILTER_USE_BOTH
);
}

/**
* Enqueues the global styles defined via theme.json.
* This should replace wp_enqueue_global_styles.
*
* @since 5.8.0
*/
function gutenberg_enqueue_global_styles() {
$separate_assets = wp_should_load_separate_core_block_assets();
$is_block_theme = wp_is_block_theme();
$is_classic_theme = ! $is_block_theme;

/*
* Global styles should be printed in the head when loading all styles combined.
* The footer should only be used to print global styles for classic themes with separate core assets enabled.
*
* See https://core.trac.wordpress.org/ticket/53494.
*/
if (
( $is_block_theme && doing_action( 'wp_footer' ) ) ||
( $is_classic_theme && doing_action( 'wp_footer' ) && ! $separate_assets ) ||
( $is_classic_theme && doing_action( 'wp_enqueue_scripts' ) && $separate_assets )
) {
return;
}

/**
* If we are loading CSS for each block separately, then we can load the theme.json CSS conditionally.
* This removes the CSS from the global-styles stylesheet and adds it to the inline CSS for each block.
*/
if ( $separate_assets ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside from this block, this whole function is copypasta.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment explaining this code branch.

add_filter( 'gutenberg_get_style_nodes', 'filter_out_block_nodes' );
// add each block as an inline css.
wp_add_global_styles_for_blocks();
}

$stylesheet = gutenberg_get_global_stylesheet();

if ( empty( $stylesheet ) ) {
return;
}

wp_register_style( 'global-styles', false, array(), true, true );
wp_add_inline_style( 'global-styles', $stylesheet );
wp_enqueue_style( 'global-styles' );
}

remove_action( 'wp_enqueue_scripts', 'wp_enqueue_global_styles' );
remove_action( 'wp_footer', 'wp_enqueue_global_styles' );
remove_action( 'wp_enqueue_scripts', 'gutenberg_enqueue_global_styles_assets' );
remove_action( 'wp_footer', 'gutenberg_enqueue_global_styles_assets' );

add_action( 'wp_enqueue_scripts', 'gutenberg_enqueue_global_styles' );
add_action( 'wp_footer', 'gutenberg_enqueue_global_styles', 1 );
2 changes: 2 additions & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ function gutenberg_is_experiment_enabled( $name ) {
// WordPress 6.1 compat.
require __DIR__ . '/compat/wordpress-6.1/blocks.php';
require __DIR__ . '/compat/wordpress-6.1/persisted-preferences.php';
require __DIR__ . '/compat/wordpress-6.1/get-global-styles-and-settings.php';
require __DIR__ . '/compat/wordpress-6.1/script-loader.php';
require __DIR__ . '/compat/wordpress-6.1/class-wp-theme-json-6-1.php';

// Experimental features.
Expand Down