-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
blocks
origin.theme.json
: renames the blocks
origin and add it to the list
theme.json
: renames the blocks
origin and add it to the listtheme.json
: add blocks
to the list of valid origins
theme.json
: add blocks
to the list of valid originsblocks
to the list of valid origins for theme.json
*/ | ||
const VALID_ORIGINS = array( | ||
'default', | ||
'blocks', |
There was a problem hiding this comment.
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' ) ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (namedcore
). - In the
WP_Theme_JSON
constructor, because it was not one of the origins registered it'll be changed totheme
. - The
WP_Theme_JSON_Resolved
loaded then thetheme
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.
There was a problem hiding this 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
I've found an issue that I'd like to investigate before merging this. |
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 |
Thank you @oandregal!
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 |
We've removed the "Backport to WP Beta/RC" label since it's not needed technically -- cherry-picking this PR to the |
What?
This PR updates the name of the
blocks
origin fromcore
toblocks
and adds it to the list of valid origins fortheme.json
.Why?
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.theme_json_blocks
, to reflect it filters "block" data.core
in the past fordefault
, 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?
core
toblocks
.blocks
to the list of valid origins.$theme_json->get_stylesheet
call uses the proper$origins
at all times.Testing
Classic theme
Using a theme without
theme.json
(for example, Canape):global-styles-inline-css
. These blocks are the only ones that use the__experimentalStyle
API in itsblock.json
. It should look like this:Block theme
Using a theme with
theme.json
(for example, TwentyTwentyTwo).Test that the styles are enqueued when the blocks are in use:
global-styles-inline-css
.wp-block-pullquote-inline-css
stylesheet and contains.wp-block-pullquote{border-width: 1px 0;font-size: 1.5em;line-height: 1.6;}
.__experimentalStyle
API also contentborder-width
, which is also unexpected. This is an issue intrunk
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 theblock.json
of pullquote, recompile and reload the front. The values should be the ones you set.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:
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_filter( 'should_load_separate_core_block_assets', '__return_false' );
to thefunctions.php
of the theme.global-styles-inline-css
stylesheet.