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

Add blocks to the list of valid origins for theme.json #44363

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Sep 22, 2022

What?

This PR updates the name of the blocks origin from core to blocks and adds it to the list of valid origins for theme.json.

Why?

  • We were missing this new origin from the list.
  • The core name is not reflective of what it does (see existing comment), as this data origin is related to block styles, whether they come with WordPress/Gutenberg or third-party blocks.
  • The existing filter for this piece of data is called theme_json_blocks, to reflect it filters "block" data.
  • We used core in the past for default, and reverted because it was confusing and want to use names that communicate what part of the pipeline are processing (default > blocks > theme > custom).

How?

  • Rename the string, from core to blocks.
  • Add blocks to the list of valid origins.
  • Verify that the $theme_json->get_stylesheet call uses the proper $origins at all times.

Testing

Classic theme

Using a theme without theme.json (for example, Canape):

  • Verify that the styles for the pullquote and navigation blocks are still present in the global-styles-inline-css. These blocks are the only ones that use the __experimentalStyle API in its block.json. It should look like this:
.wp-block-pullquote{font-size: 1.5em;line-height: 1.6;}
.wp-block-navigation a:where(:not(.wp-element-button)){color: inherit;}

Block theme

Using a theme with theme.json (for example, TwentyTwentyTwo).

Test that the styles are enqueued when the blocks are in use:

  • Create a page that uses the pullquote and navigation blocks and publish.
  • Go to the front and
    • verify that the styles for the pullquote and navigation blocks are not part of the global-styles-inline-css.
    • verify that there's a wp-block-pullquote-inline-css stylesheet and contains .wp-block-pullquote{border-width: 1px 0;font-size: 1.5em;line-height: 1.6;}.
      • Note that the stylesheet contains a lot more CSS than expected and the rule generated by the __experimentalStyle API also content border-width, which is also unexpected. This is an issue in trunk that needs to be investigated separately. If you want to go the extra mile and verify that this works as expected, edit the values found in __experimentalStyle of the block.json of pullquote, recompile and reload the front. The values should be the ones you set.
    • verify that there's a wp-block-navigation-inline-css embedded stylesheet whose content is .wp-block-navigation a:where(:not(.wp-element-button)){color: inherit;}.

Test that the styles are not enqueued when the blocks are not in use:

  • Create a page that does not use the pullquote and navigation blocks and publish.
  • Go to the front and verify that the styles for the pullquote and navigation blocks are not part of the global-styles-inline-css and aren't loaded separately either.

Test that themes can opt-out from loading blocks separately even if the post uses the blocks:

  • Add add_filter( 'should_load_separate_core_block_assets', '__return_false' ); to the functions.php of the theme.
  • Go to the front (any post/page) and verify that styles for all blocks are part of the global-styles-inline-css stylesheet.

@oandregal oandregal self-assigned this Sep 22, 2022
@oandregal oandregal added [Type] Code Quality Issues or PRs that relate to code quality Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Sep 22, 2022
@oandregal oandregal changed the title Updates the name assigned to the blocks origin. Valid origins for theme.json: renames the blocks origin and add it to the list Sep 22, 2022
@oandregal oandregal changed the title Valid origins for theme.json: renames the blocks origin and add it to the list Valid origins for theme.json: add blocks to the list of valid origins Sep 22, 2022
@oandregal oandregal changed the title Valid origins for theme.json: add blocks to the list of valid origins Add blocks to the list of valid origins for theme.json Sep 22, 2022
*/
const VALID_ORIGINS = array(
'default',
'blocks',
Copy link
Member Author

Choose a reason for hiding this comment

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

blocks was missing

@@ -141,12 +141,10 @@ public static function get_block_data() {
*
* @param WP_Theme_JSON_Data_Gutenberg Class to access and update the underlying data.
*/
$theme_json = apply_filters( 'theme_json_blocks', new WP_Theme_JSON_Data_Gutenberg( $config, 'core' ) );
Copy link
Member Author

Choose a reason for hiding this comment

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

Changing this name here does not have any effect because it was not being used anywhere. It has started to be used when it was added to the VALID_ORIGINS list.

Copy link
Member Author

@oandregal oandregal Sep 22, 2022

Choose a reason for hiding this comment

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

By using an unregistered origin this is what was happening:

  • The WP_Theme_JSON_Resolver loaded the blocks origin (named core).
  • In the WP_Theme_JSON constructor, because it was not one of the origins registered it'll be changed to theme.
  • The WP_Theme_JSON_Resolved loaded then the theme origin, updating any instance.

This was working as expected. But only by coincidence. If the blocks origin was able to define presets this won't work. The reason is it'll work this way:

  • the presets of the blocks origin would be stored in the theme key
  • when the actual theme origin was loaded, its presets would be also loaded in the theme key, hence, overriding the previous blocks' presets that should have been stored elsewhere.

Essentially, this is the sort of tricky dormant bug waiting to happen.

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

I tested in a classic and block themes, and also applied the filter and everything works as expected. Thanks for handling this.

/*
* We only use the default, theme, and custom origins.
* This is because styles for blocks origin are added
* at a later phase (render cycle) so we only render the ones in use.
Copy link
Member

Choose a reason for hiding this comment

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

Given that the styles are output in other parts of code, isn't there the risk that priority 'default', 'block, 'theme', and 'custom' is not respected? How do we ensure this order?

Copy link
Member Author

@oandregal oandregal Sep 22, 2022

Choose a reason for hiding this comment

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

I've looked for every usage of this function (including core) and all instances have been updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me expand this.

The $origins only affect presets, they do not affect the styles in the styles key. Because the blocks origin cannot declare any preset, not providing the origins (hence using all of them, including blocks) would not matter in practice. Though I still think it is good practice to be explicit. Maybe, in the future, blocks can declare presets by themselves.

Note that a filter gutenberg_theme_json_get_style_nodes was introduced to remove the styles entirely from the global styles stylesheet and process them separately.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

The change look good, but I left a question of do ensure the expected styles order of default, blocks, theme, and user.

@oandregal
Copy link
Member Author

I've found an issue that I'd like to investigate before merging this.

@oandregal
Copy link
Member Author

Done. This is ready to land, was investigating this and this.

@oandregal
Copy link
Member Author

I'm backporting this in WordPress/wordpress-develop#3313

Not sure how this needs to be dealt with in Gutenberg land at this point (we've already released 14.1, the last Gutenberg version to be bundled with WordPress?). Happy to help if someone provides direction. cc @ockham @michalczaplinski

@ockham
Copy link
Contributor

ockham commented Sep 22, 2022

I'm backporting this in WordPress/wordpress-develop#3313

Thank you @oandregal!

Not sure how this needs to be dealt with in Gutenberg land at this point (we've already released 14.1, the last Gutenberg version to be bundled with WordPress?). Happy to help if someone provides direction. cc @ockham @michalczaplinski

So if it's a bug, we can still merge the fix. As for the GB PR, adding the "Backport to WP Beta/RC" label is enough; this brings it on our radar when we triage (planned for Monday) and cherry-pick fixes for a new round of npm releases prior to the next Beta (Tuesday).

As for the backport PR, we'll need to get that approved and find a Core committer to commit it -- ideally also before Tuesday 😊

Edit: I just noticed that this PR doesn't touch any JS at all -- it's all PHP code in lib/compat/wordpress-6.1/ and lib/experimental/. In this case, the npm publishing is of course irrelevant, and WordPress/wordpress-develop#3313 is all that matters for Core 😄

@ndiego ndiego removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 26, 2022
@ockham
Copy link
Contributor

ockham commented Sep 26, 2022

We've removed the "Backport to WP Beta/RC" label since it's not needed technically -- cherry-picking this PR to the wp/6.1 branch and publishing npms from there won't include those files. Instead, they're covered by WordPress/wordpress-develop#3313.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants