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.

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

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented May 19, 2022

What?

This is an alternative to #40513. We should only load the Global Styles CSS for blocks that are used on the page, rather than loading it on every page as we do presently.

Why?

This means sending less data on each page load, so it should improve performance. This is is a pre-cursor to #34180, which will add a lot more CSS to each page load unless we improve the performance.

How?

The basic premise is that if we are loading separate block assets, we move the block specific Global Styles CSS from the global-styles inline CSS block to the inline CSS portion of the block's CSS. The loading of this inline CSS is handled by wp_common_block_scripts_and_styles which enqueues the inline CSS for each block, so this just builds on top of that existing logic.

The changes we need to make are:

  1. Don't add block specific CSS to the global styles inline CSS
  2. Add add the block specific CSS to the individual block styles inline CSS

This differs from the approach in #40513 by introducing a new filter for get_style_nodes which allows us to filter blocks from the style nodes used to generate the global styles, keeping WP_Theme_Json ignorant of the details of how assets are loaded.

Testing Instructions

  1. Switch to TT2
  2. Go to a page without a quote block on
  3. Check that this CSS isn't in either the global-styles-inline-css or wp-block-quote-css:
    .wp-block-quote {
    border-width: 1px;
    }
  4. Go to a page with a quote block on
    Check that this CSS is not in the global-styles-inline-css but is in wp-block-quote-css:
    .wp-block-quote {
    border-width: 1px;
    }

Note

The CSS for the blocks is doubled up - this is because its being enqueued in both core and by Gutenberg. This happens in trunk.

@WordPress/block-themers

@scruffian scruffian added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels May 19, 2022
@scruffian scruffian self-assigned this May 19, 2022

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

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?

*
* @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.

*
* @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.

return;
}

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.

@scruffian scruffian changed the title Add/conditional global styles loading Global Styles: Load block styles conditionally May 19, 2022
@scruffian scruffian changed the title Global Styles: Load block styles conditionally Global Styles: Load block CSS conditionally May 19, 2022
* @package gutenberg
*/

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.

foreach ( $block_nodes as $metadata ) {
$block_css = $tree->get_styles_for_block( $metadata );
$block_name = str_replace( 'core/', '', $metadata['name'] );
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.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I think this is good and makes a lot of sense to me. My only confusion is about the necessity for the gutenberg_get_style_nodes filter.

@adamziel
Copy link
Contributor

adamziel commented May 20, 2022

@scruffian quote/style.css seems to be getting enqueued twice:

CleanShot 2022-05-20 at 14 39 14

(Curl just for brevity, I checked in the actual HTML)

Also, when I set $separate_assets to false as below:

/lib/init.php
add_filter('should_load_separate_core_block_assets', function() { return false; }, 10000);

The only bit of CSS that's getting loaded for the quote block is .wp-block-quote{ border-width: 1px; }. On trunk, it loads all the styles (and appends them to the global styles). The same thing seems to be happening on trunk. Is that expected?

Copy link
Contributor

@adamziel adamziel left a comment

Choose a reason for hiding this comment

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

Both issues I mentioned above are present on trunk and therefore I don't think this PR introduces either of them.

The idea sounds brilliant to me: let's remove the block styles from global CSS and add them to their dedicated inline CSSes that only happen to be rendered when the actual blocks are used. The PR works and I don't see any major issues so I'm approving.

@scruffian scruffian force-pushed the add/conditional-global-styles-loading branch from 3c246b4 to 92ac01d Compare May 20, 2022 20:03
@scruffian scruffian merged commit 323174a into trunk May 20, 2022
@scruffian scruffian deleted the add/conditional-global-styles-loading branch May 20, 2022 22:19
@github-actions github-actions bot added this to the Gutenberg 13.4 milestone May 20, 2022
westonruter added a commit that referenced this pull request May 23, 2022
…p-tests-config

* 'trunk' of github.com:WordPress/gutenberg: (88 commits)
  Components: refactor `AlignmentMatrixControl` to pass `exhaustive-deps` (#41167)
  [RNMobile] Add 'Insert from URL' option to Image block (#40334)
  [RNMobile] Improvements to Getting Started Guides (#40964)
  Post Author Name: Add to and from Post Author transformations (#41151)
  CheckboxControl: Add unit tests (#41165)
  Improve inline documentation (#41209)
  Mobile Release v1.76.1 (#41196)
  Use explicit type definitions for entity configuration (#40995)
  Scripts: Convert file extension to js in `block.json` during build (#41068)
  Reflects revert in 6446878 (#41221)
  get_style_nodes should be compatible with parent method. (#41217)
  Gallery: Opt-in to axial (column/row) block spacing controls (#41175)
  Table of Contents block: convert line breaks to spaces in headings. (#41206)
  Add support for button elements to theme.json (#40260)
  Global Styles: Load block CSS conditionally (#41160)
  Update URL (#41188)
  Improve autocompleter performance (#41197)
  Site Editor: Set min-width for styles preview (#41198)
  Remove Navigation Editor screen from experiments page (#40878)
  Fix broken Page title for pages created inline within in Nav block (#41063)
  ...
ramonjd added a commit that referenced this pull request May 24, 2022
…uired. This commit migrates methods that call get_style_nodes() from WP_Theme_JSON_5_9 to WP_Theme_JSON_6_1, and removes the argument.
ramonjd added a commit that referenced this pull request May 30, 2022
…uired. This commit migrates methods that call get_style_nodes() from WP_Theme_JSON_5_9 to WP_Theme_JSON_6_1, and removes the argument.
@priethor priethor added the [Type] Performance Related to performance efforts label Jun 8, 2022
ramonjd added a commit that referenced this pull request Jul 5, 2022
…uired. This commit migrates methods that call get_style_nodes() from WP_Theme_JSON_5_9 to WP_Theme_JSON_6_1, and removes the argument.
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

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?

pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Sep 10, 2022
This commit backports the original PRs from Gutenberg repository:

* [WordPress/gutenberg#40260 #40260 Add support for button elements to theme.json]
* [WordPress/gutenberg#40889 #40889 Theme Json: Don't output double selectors for elements inside blocks]
* [WordPress/gutenberg#41140 #41140 Global Styles: Add support for caption elements]
* [WordPress/gutenberg#41160 #41160 Global Styles: Load block CSS conditionally]
* [WordPress/gutenberg#41240 #41240 Global Styles: Button Element: update button element selector]
* [WordPress/gutenberg#41335 #41335 Duotone: Fix CSS Selectors rendered by theme.json duotone/filter settings for blocks on public pages]
* [WordPress/gutenberg#41446 #41446 Block styles: Account for style block nodes that have no name]
* [WordPress/gutenberg#41696 #41696 Global Styles: Allow references to values in other locations in the tree]
* [WordPress/gutenberg#41753 #41753 Elements: Add an API make it easier to get class names]
* [WordPress/gutenberg#41786 #41786 Support pseudo selectors on elements in theme json]
* [WordPress/gutenberg#41822 #41822 Elements: Button - Fix element selectors]
* [WordPress/gutenberg#41981 #41981 Global Styles: Add support for heading elements]
* [WordPress/gutenberg#42072 #42072 Fix link element hover bleeding into button element default styles]
* [WordPress/gutenberg#42096 #42096 Add visited to link element allowed pseudo selector list]
* [WordPress/gutenberg#42669 #42669 Link elements: Add a :where selector to the :not to lower specificity]
* [WordPress/gutenberg#42776 #42776 Theme JSON: Add a static $blocks_metadata data definition to the Gutenberg instance of WP_Theme_JSON]
* [WordPress/gutenberg#43088 #43088 Pseudo elements supports on button elements]
* [WordPress/gutenberg#43167 #43167 Theme_JSON: Use existing append_to_selector for pseudo elements]
* [WordPress/gutenberg#43988 #43988 Styles API: Fixed selectors for nested elements]

Props onemaggie, bernhard-reiter, cbravobernal, mmaattiiaass, scruffian, andraganescu, dpcalhoun, get_dave, Mamaduka, SergeyBiryukov.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54118 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Sep 10, 2022
This commit backports the original PRs from Gutenberg repository:

* [WordPress/gutenberg#40260 #40260 Add support for button elements to theme.json]
* [WordPress/gutenberg#40889 #40889 Theme Json: Don't output double selectors for elements inside blocks]
* [WordPress/gutenberg#41140 #41140 Global Styles: Add support for caption elements]
* [WordPress/gutenberg#41160 #41160 Global Styles: Load block CSS conditionally]
* [WordPress/gutenberg#41240 #41240 Global Styles: Button Element: update button element selector]
* [WordPress/gutenberg#41335 #41335 Duotone: Fix CSS Selectors rendered by theme.json duotone/filter settings for blocks on public pages]
* [WordPress/gutenberg#41446 #41446 Block styles: Account for style block nodes that have no name]
* [WordPress/gutenberg#41696 #41696 Global Styles: Allow references to values in other locations in the tree]
* [WordPress/gutenberg#41753 #41753 Elements: Add an API make it easier to get class names]
* [WordPress/gutenberg#41786 #41786 Support pseudo selectors on elements in theme json]
* [WordPress/gutenberg#41822 #41822 Elements: Button - Fix element selectors]
* [WordPress/gutenberg#41981 #41981 Global Styles: Add support for heading elements]
* [WordPress/gutenberg#42072 #42072 Fix link element hover bleeding into button element default styles]
* [WordPress/gutenberg#42096 #42096 Add visited to link element allowed pseudo selector list]
* [WordPress/gutenberg#42669 #42669 Link elements: Add a :where selector to the :not to lower specificity]
* [WordPress/gutenberg#42776 #42776 Theme JSON: Add a static $blocks_metadata data definition to the Gutenberg instance of WP_Theme_JSON]
* [WordPress/gutenberg#43088 #43088 Pseudo elements supports on button elements]
* [WordPress/gutenberg#43167 #43167 Theme_JSON: Use existing append_to_selector for pseudo elements]
* [WordPress/gutenberg#43988 #43988 Styles API: Fixed selectors for nested elements]

Props onemaggie, bernhard-reiter, cbravobernal, mmaattiiaass, scruffian, andraganescu, dpcalhoun, get_dave, Mamaduka, SergeyBiryukov.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54118


git-svn-id: http://core.svn.wordpress.org/trunk@53677 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Sep 10, 2022
This commit backports the original PRs from Gutenberg repository:

* [WordPress/gutenberg#40260 #40260 Add support for button elements to theme.json]
* [WordPress/gutenberg#40889 #40889 Theme Json: Don't output double selectors for elements inside blocks]
* [WordPress/gutenberg#41140 #41140 Global Styles: Add support for caption elements]
* [WordPress/gutenberg#41160 #41160 Global Styles: Load block CSS conditionally]
* [WordPress/gutenberg#41240 #41240 Global Styles: Button Element: update button element selector]
* [WordPress/gutenberg#41335 #41335 Duotone: Fix CSS Selectors rendered by theme.json duotone/filter settings for blocks on public pages]
* [WordPress/gutenberg#41446 #41446 Block styles: Account for style block nodes that have no name]
* [WordPress/gutenberg#41696 #41696 Global Styles: Allow references to values in other locations in the tree]
* [WordPress/gutenberg#41753 #41753 Elements: Add an API make it easier to get class names]
* [WordPress/gutenberg#41786 #41786 Support pseudo selectors on elements in theme json]
* [WordPress/gutenberg#41822 #41822 Elements: Button - Fix element selectors]
* [WordPress/gutenberg#41981 #41981 Global Styles: Add support for heading elements]
* [WordPress/gutenberg#42072 #42072 Fix link element hover bleeding into button element default styles]
* [WordPress/gutenberg#42096 #42096 Add visited to link element allowed pseudo selector list]
* [WordPress/gutenberg#42669 #42669 Link elements: Add a :where selector to the :not to lower specificity]
* [WordPress/gutenberg#42776 #42776 Theme JSON: Add a static $blocks_metadata data definition to the Gutenberg instance of WP_Theme_JSON]
* [WordPress/gutenberg#43088 #43088 Pseudo elements supports on button elements]
* [WordPress/gutenberg#43167 #43167 Theme_JSON: Use existing append_to_selector for pseudo elements]
* [WordPress/gutenberg#43988 #43988 Styles API: Fixed selectors for nested elements]

Props onemaggie, bernhard-reiter, cbravobernal, mmaattiiaass, scruffian, andraganescu, dpcalhoun, get_dave, Mamaduka, SergeyBiryukov.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54118


git-svn-id: https://core.svn.wordpress.org/trunk@53677 1a063a9b-81f0-0310-95a4-ce76da25c4cd
whereiscodedude pushed a commit to whereiscodedude/wpss that referenced this pull request Sep 18, 2022
This commit backports the original PRs from Gutenberg repository:

* [WordPress/gutenberg#40260 #40260 Add support for button elements to theme.json]
* [WordPress/gutenberg#40889 #40889 Theme Json: Don't output double selectors for elements inside blocks]
* [WordPress/gutenberg#41140 #41140 Global Styles: Add support for caption elements]
* [WordPress/gutenberg#41160 #41160 Global Styles: Load block CSS conditionally]
* [WordPress/gutenberg#41240 #41240 Global Styles: Button Element: update button element selector]
* [WordPress/gutenberg#41335 #41335 Duotone: Fix CSS Selectors rendered by theme.json duotone/filter settings for blocks on public pages]
* [WordPress/gutenberg#41446 #41446 Block styles: Account for style block nodes that have no name]
* [WordPress/gutenberg#41696 #41696 Global Styles: Allow references to values in other locations in the tree]
* [WordPress/gutenberg#41753 #41753 Elements: Add an API make it easier to get class names]
* [WordPress/gutenberg#41786 #41786 Support pseudo selectors on elements in theme json]
* [WordPress/gutenberg#41822 #41822 Elements: Button - Fix element selectors]
* [WordPress/gutenberg#41981 #41981 Global Styles: Add support for heading elements]
* [WordPress/gutenberg#42072 #42072 Fix link element hover bleeding into button element default styles]
* [WordPress/gutenberg#42096 #42096 Add visited to link element allowed pseudo selector list]
* [WordPress/gutenberg#42669 #42669 Link elements: Add a :where selector to the :not to lower specificity]
* [WordPress/gutenberg#42776 #42776 Theme JSON: Add a static $blocks_metadata data definition to the Gutenberg instance of WP_Theme_JSON]
* [WordPress/gutenberg#43088 #43088 Pseudo elements supports on button elements]
* [WordPress/gutenberg#43167 #43167 Theme_JSON: Use existing append_to_selector for pseudo elements]
* [WordPress/gutenberg#43988 #43988 Styles API: Fixed selectors for nested elements]

Props onemaggie, bernhard-reiter, cbravobernal, mmaattiiaass, scruffian, andraganescu, dpcalhoun, get_dave, Mamaduka, SergeyBiryukov.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54118
ootwch pushed a commit to ootwch/wordpress-develop that referenced this pull request Nov 4, 2022
This commit backports the original PRs from Gutenberg repository:

* [WordPress/gutenberg#40260 #40260 Add support for button elements to theme.json]
* [WordPress/gutenberg#40889 #40889 Theme Json: Don't output double selectors for elements inside blocks]
* [WordPress/gutenberg#41140 #41140 Global Styles: Add support for caption elements]
* [WordPress/gutenberg#41160 #41160 Global Styles: Load block CSS conditionally]
* [WordPress/gutenberg#41240 #41240 Global Styles: Button Element: update button element selector]
* [WordPress/gutenberg#41335 #41335 Duotone: Fix CSS Selectors rendered by theme.json duotone/filter settings for blocks on public pages]
* [WordPress/gutenberg#41446 #41446 Block styles: Account for style block nodes that have no name]
* [WordPress/gutenberg#41696 #41696 Global Styles: Allow references to values in other locations in the tree]
* [WordPress/gutenberg#41753 #41753 Elements: Add an API make it easier to get class names]
* [WordPress/gutenberg#41786 #41786 Support pseudo selectors on elements in theme json]
* [WordPress/gutenberg#41822 #41822 Elements: Button - Fix element selectors]
* [WordPress/gutenberg#41981 #41981 Global Styles: Add support for heading elements]
* [WordPress/gutenberg#42072 #42072 Fix link element hover bleeding into button element default styles]
* [WordPress/gutenberg#42096 #42096 Add visited to link element allowed pseudo selector list]
* [WordPress/gutenberg#42669 #42669 Link elements: Add a :where selector to the :not to lower specificity]
* [WordPress/gutenberg#42776 #42776 Theme JSON: Add a static $blocks_metadata data definition to the Gutenberg instance of WP_Theme_JSON]
* [WordPress/gutenberg#43088 #43088 Pseudo elements supports on button elements]
* [WordPress/gutenberg#43167 #43167 Theme_JSON: Use existing append_to_selector for pseudo elements]
* [WordPress/gutenberg#43988 #43988 Styles API: Fixed selectors for nested elements]

Props onemaggie, bernhard-reiter, cbravobernal, mmaattiiaass, scruffian, andraganescu, dpcalhoun, get_dave, Mamaduka, SergeyBiryukov.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54118 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants