Theme.json: Extend block style variations support #56540
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.5/blocks.php ❔ lib/block-supports/elements.php ❔ lib/class-wp-theme-json-gutenberg.php ❔ lib/class-wp-theme-json-resolver-gutenberg.php ❔ lib/load.php ❔ phpunit/block-supports/elements-test.php ❔ phpunit/class-wp-theme-json-resolver-test.php ❔ phpunit/class-wp-theme-json-test.php |
|
Size Change: +630 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
try/extending-block-style-variations
branch
from
34ec6a1
to
e10a879
Compare
|
Flaky tests detected in a88a18a488c06a98eb9540823348b660ee04239c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7471653408
|
aaronrobertshaw
force-pushed
the
try/extending-block-style-variations
branch
from
e10a879
to
c34ca0a
Compare
|
I think we can switch to using a custom select with render items for block style selection. Background and text colour for swatches. pattern-styles.mp4 |
Any objections to this UI enhancement being done separately or as a follow-up? I take it we want that change regardless of these enhancements to block style variations. Splitting that out might help prevent this PR from blocking it. |
None at all |
aaronrobertshaw
force-pushed
the
try/extending-block-style-variations
branch
from
efe74ee
to
390daca
Compare
aaronrobertshaw
requested review from
spacedmonkey,
ellatrix and
ajlende
as code owners
aaronrobertshaw
force-pushed
the
try/extending-block-style-variations
branch
from
390daca
to
c90e238
Compare
aaronrobertshaw
changed the title
| $schema_styles_variations = array(); | ||
| $block_style_variation_styles = static::VALID_STYLES; | ||
| $block_style_variation_styles['blocks'] = null; | ||
| $block_style_variation_styles['elements'] = null; | ||
|
|
||
| $schema_styles_variations = array(); | ||
| if ( ! empty( $style_variation_names ) ) { | ||
| $schema_styles_variations = array_fill_keys( $style_variation_names, $styles_non_top_level ); | ||
| $schema_styles_variations = array_fill_keys( $style_variation_names, $block_style_variation_styles ); |
There was a problem hiding this comment.
Block style variations are being extended to allow styles for inner block types and elements. The changes to the schema here prevent blocks and elements properties from being stripped out.
| $registered_styles = $style_registry->get_all_registered(); | ||
| foreach ( static::$blocks_metadata as $block_name => $block_metadata ) { | ||
| if ( ! empty( $registered_styles[ $block_name ] ) ) { | ||
| $style_selectors = $block_metadata['styleVariations'] ?? array(); | ||
|
|
||
| foreach ( $registered_styles[ $block_name ] as $block_style ) { | ||
| if ( ! isset( $style_selectors[ $block_style['name'] ] ) ) { | ||
| $style_selectors[ $block_style['name'] ] = static::append_to_selector( | ||
| '.is-style-' . $block_style['name'] . '.is-style-' . $block_style['name'], | ||
| $block_metadata['selector'] | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| static::$blocks_metadata[ $block_name ]['styleVariations'] = $style_selectors; | ||
| } | ||
| } |
There was a problem hiding this comment.
While testing, I found this was required to ensure new block style variations in the styles registry were detected properly so matching config in the theme.json wasn't stripped out during sanitization. See the constructor and remove_insecure_properties functions.
| // Block style variations can be registered through the WP_Block_Styles_Registry as well as block.json. | ||
| $registered_styles = $style_registry->get_registered_styles_for_block( $block_name ); | ||
| foreach ( $registered_styles as $style ) { | ||
| $style_selectors[ $style['name'] ] = static::append_to_selector( '.is-style-' . $style['name'] . '.is-style-' . $style['name'], static::$blocks_metadata[ $block_name ]['selector'] ); | ||
| } |
There was a problem hiding this comment.
As static::$blocks_metadata[ $block_name ]['styleVariations'] is used to sanitize available block style variations, the changes here make sure to include anything from the block style registry.
| if ( ! $selector || ! $scope ) { | ||
| return $selector; | ||
| } |
There was a problem hiding this comment.
This tweak just allowed more flexible use of the scope_selector util function, simplifying code elsewhere.
| foreach ( $variation_blocks as $variation_block => $variation_block_node ) { | ||
| $variation_block_selector = static::scope_selector( $variation_selector, $selectors[ $variation_block ]['selector'] ?? null ); | ||
| $variation_duotone_selector = static::scope_selector( $variation_selector, $selectors[ $variation_block ]['duotone'] ?? null ); | ||
| $variation_feature_selectors = $selectors[ $variation_block ]['selectors'] ?? null; | ||
|
|
||
| if ( $variation_feature_selectors ) { | ||
| foreach ( $variation_feature_selectors as $feature => $feature_selector ) { | ||
| if ( is_string( $feature_selector ) ) { | ||
| $variation_feature_selectors[ $feature ] = static::scope_selector( $variation_selector, $feature_selector ); | ||
| } | ||
|
|
||
| if ( is_array( $feature_selector ) ) { | ||
| foreach ( $feature_selector as $subfeature => $subfeature_selector ) { | ||
| $variation_feature_selectors[ $feature ][ $subfeature ] = static::scope_selector( $variation_selector, $subfeature_selector ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $variation_nodes[] = array( | ||
| 'name' => $variation_block, | ||
| 'path' => array( 'styles', 'blocks', $name, 'variations', $variation, 'blocks', $variation_block ), | ||
| 'selector' => $variation_block_selector, | ||
| 'selectors' => $variation_feature_selectors, | ||
| 'duotone' => $variation_duotone_selector, | ||
| ); | ||
| } |
There was a problem hiding this comment.
This code processes a block style variation's inner block type styles.
Blocks can define custom selectors for certain features, e.g., color, typography, etc., to be applied to inner markup. For this to work within a block style variation the block type's custom feature selectors need to be further scoped to the selector for the block style variation.
Once the feature selectors are correctly scoped, the inner block's node can be created as per a regular block node.
| * style_data (theme.json-like object to generate CSS from). | ||
| * @return bool True if all block styles were registered with success and false otherwise. | ||
| */ | ||
| function gutenberg_register_block_style( $block_name, $style_properties ) { |
There was a problem hiding this comment.
Initially, this extended block style registration also generated a stylesheet and enqueued it for block style variations that included a style object within its properties. This allowed only a single set of styles to be generated for a block style variation that was shared across multiple block types.
Future follow-ups to this PR will be to allow block style variations to be defined within theme.json outside of a block type. These definitions can then be referenced across multiple block types.
There was a problem hiding this comment.
Future follow-ups to this PR will be to allow block style variations to be defined within theme.json outside of a block type. These definitions can then be referenced across multiple block types.
That will be very handy! I came across that in testing with your provided test style variations and it was a bit confusing at first when tweaking styles in one of the Group variations had no effect in Columns with the same variation 😅
There was a problem hiding this comment.
it was a bit confusing
Apologies for the confusion. I'll add a warning to the PR description for that gotcha.
I'm inclined to see if we can refine this to a point we're happy to land, while concurrently working on allowing the referenced variations within theme.json.
What do you think?
| @@ -611,6 +611,33 @@ function pickStyleKeys( treeToPickFrom ) { | |||
| return Object.fromEntries( clonedEntries ); | |||
| } | |||
|
|
|||
| function scopeFeatureSelectors( scope, selectors ) { | |||
There was a problem hiding this comment.
Blocks can define their own custom selectors for features such as color, typography, border etc. When such a block is styled as an inner block to a block style variation, its custom feature selectors need to be scoped by the block style variations selector to still be correctly applied. This function helps with that.
| node.variations[ variation ] | ||
| ); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
The following changes mostly follow those in the PHP class for processing block style variations into nodes that can be later turned into actual styles.
| @@ -22,8 +32,10 @@ export function useBlockVariations( name ) { | |||
| }, | |||
| [ name ] | |||
| ); | |||
| const coreBlockStyles = getCoreBlockStyles( blockStyles ); | |||
| return coreBlockStyles; | |||
| const [ variations ] = useGlobalStyle( 'variations', name, 'base' ); | |||
There was a problem hiding this comment.
At this point, the theme.json resolver has absorbed block style variations registered through the block styles registry into the merged global styles data so it can be used as a source of validated variations.
| // Only core block styles (source === block) or block styles with | ||
| // a matching theme.json style variation will be configurable via | ||
| // Global Styles. | ||
| function getFilteredBlockStyles( blockStyles, variations ) { |
There was a problem hiding this comment.
This function now opens up Global Styles to allow for non-core block style variations to be configured through the site editor.
At this stage, inner block type and element styles for the block style variation are not editable however this could be worked on in a follow-up.
aaronrobertshaw
force-pushed
the
try/extending-block-style-variations
branch
from
c90e238
to
eadf74b
Compare
aaronrobertshaw
added
the
[Feature] Block Style Variations
|
A future follow-up for this will be to explore refining the theme.json API around registering block style variations across multiple block types. For example, perhaps a shared block style variation could be defined under |
There was a problem hiding this comment.
This is looking pretty solid! Some comments and questions below. Testing shows all working as expected.
Is there any plan to make style variations declarable directly through theme.json without the additional step of registering them in PHP? Asking because part of the idea in #44417 was to provide the ability to create new variations through the global styles interface. It wasn't implemented straightaway and I subsequently created #49602 for it.
I'm also wondering why WP_Block_Styles_Registry is final; perhaps @jorgefilipecosta has some idea? The ticket that added the class to core doesn't explain much.
| if ( isset( $selectors[ $name ]['selector'] ) ) { | ||
| $selector = $selectors[ $name ]['selector']; | ||
| } | ||
| $selector = $selectors[ $name ]['selector'] ?? null; |
There was a problem hiding this comment.
I'm afraid the null coalescing operator isn't officially supported in WP yet. There's a ticket for it here.
There was a problem hiding this comment.
Thanks for flagging that. I had a suspicion that was the case although I recalled some recent performance PRs swapping out _wp_array_get for it. It's used a fair bit now already within this class.
Do you think it is that much of a nuisance when it comes to backporting?
There was a problem hiding this comment.
Yeahhh those perf improvements were changed before landing in core - see the discussion here. I'd avoid it if you don't want the backport PR to turn into an argument 😂
|
|
||
| // Note that `get_feature_declarations_for_node` will also unset | ||
| // feature values so they aren't duplicated in declarations via | ||
| // the call below. |
There was a problem hiding this comment.
I'm not sure I understand this comment. Which feature values are unset by get_feature_declarations_for_node and why might they be duplicated below? Might be worth adding a bit more context. (Also is it correct to assume "features" here means the feature selectors optionally declared by blocks?)
There was a problem hiding this comment.
I'm not sure I understand this comment.
Nor do I 😅. I suspect I messed up while rebasing an earlier incarnation of this PR.
Which feature values are unset by get_feature_declarations_for_node and why might they be duplicated below? Might be worth adding a bit more context. (Also is it correct to assume "features" here means the feature selectors optionally declared by blocks?)
Correct.
If a block defines custom selectors for features, the get_feature_declarations_for_node function will create the appropriate declarations from the normal style object given to it. If the values used in the feature declarations weren't unset, when the style object is processed for the normal block declarations we'd get styles applying the value to both the custom feature selector and the block's wrapper.
I've removed the comment as I don't think it adds much value now the sands have shifted underneath it.
| * style_data (theme.json-like object to generate CSS from). | ||
| * @return bool True if all block styles were registered with success and false otherwise. | ||
| */ | ||
| function gutenberg_register_block_style( $block_name, $style_properties ) { |
There was a problem hiding this comment.
Future follow-ups to this PR will be to allow block style variations to be defined within theme.json outside of a block type. These definitions can then be referenced across multiple block types.
That will be very handy! I came across that in testing with your provided test style variations and it was a bit confusing at first when tweaking styles in one of the Group variations had no effect in Columns with the same variation 😅
|
Thanks for taking the time to review this one @tellthemachines 👍
Definitely! #49602 is already on my radar, especially with the API refinements I mentioned above. I'll actually be digging into this over the next few days.
It puzzled me as well. Unfortunately, it is what it is and made extending the block style registration as suggested in the issue more awkward. I'm certainly open to better approaches. In my view, it adds some extra justification to adding something like the |
aaronrobertshaw
force-pushed
the
try/extending-block-style-variations
branch
from
875938a
to
562a9fa
Compare
|
Just did another round of testing on this and everything is working as expected, taking into account the details @andrewserong outlined above. I can also confirm that nested block element styles (the fuchsia example) are now working correctly across editors and front end.
I agree with this. User-generated styles should override theme styles, and it's up to the user to add them to the correct container.
Yeah this is a weird one. The latest added style always overrides earlier, more targeted styles. It may be related to how we load each new update in its own style tag, because I notice that switching the order of style tags in the DOM changes which styles apply. I'd expect that, if all the declarations were in the same stylesheet, it wouldn't matter what order they're in because the classname closer to the element in the DOM hierarchy would take precedence. In any case, this should be looked at separately. This PR feels pretty ready to me. Is there anything still to do or is it final review time? |
All testing nicely for me, too. I haven't run into any other issues so far! |
|
Thanks for the patience and commitment to testing and reviewing this one @tellthemachines and @andrewserong 👍
🎉 I think we're ready for a final review! I'll also give it a further once over with fresh eyes on Monday. |
Sounds good! I'll give it another proper look on Monday with fresh eyes, too 🙂 |
Same 😄 |
|
Just looking into the e2e test failures, and it seems they're legitimate for the Navigation block, where it expects Navigation link colors to inherit from a wrapping Group block, but not to be applied to submenus or overlays: In manually testing locally, I can see that on
So, unfortunately it seems we have a case of the Navigation block expecting the block-level link color to have a lower priority. In this case, would it be better to try to fix the Navigation block to account for the higher specificity, or does this point to potential breaking changes in other blocks (i.e. 3rd party blocks) that could potentially be an issue? 🤔 |
Great catch @andrewserong ! From the comment above the line where submenu color is set it looks like specificity has been deliberately set to override element styles. I'm not sure what the best approach is tbh, there's no way of knowing how many custom blocks out there might be relying on the existing specificity. |
For all my attempts at creating a decent example of possible breakages, it was under my nose all along with the navigation block. It's rather obvious in hindsight!
This has been the question all along. Over recent discussions, the consensus was that this possible breakage would be very much an edge case. In the overwhelming majority of cases, a block should adopt the user's choice of element styles and it would be rare for a block to need differing element styles within itself. Then, should a user have applied element styles to a section or group and desire a different color for something within that, they'd need to manually apply it. Does the example of the navigation block change this thinking? Do you both now believe this use case is more likely than our previous understanding?
I'm struggling to find any clean options here but that too, has been the problem all along 🤔 Bumping the specificity for that block further feels wrong and a course of action that might lead to further issues. I imagine this is one of the more heavily styled blocks out there. Another option I've explored is tweaking the element styles selector such that it won't apply to navigation submenu links. While we might be able to get this work and even allow for reducing the selector @tellthemachines noted was deliberately aimed at overcoming element styles. The solution isn't exactly scalable to every possible 3rd party block that might suffer a similar fate. It also adds some block-specific code where it doesn't really belong, not that this would be the first time the nav block has forced that. 
|
|
I just tried something in #57841 that could potentially allow us to reduce specificity of the element selector. I don't think there's any solution here that won't involve breaking changes of some sort: any specificity change is going to be a breaking change. My reasoning is it's usually better to reduce than increase specificity, so it's worth giving it a try 😄 |
|
Revisiting the option to dial back block style variation functionality, such that a variation's block type styles cannot contain element styles, might be viable.
|
|
I've removed the ability to style elements for specific block types within a block style variation. While testing the latest iteration, I've encountered a few further issues relating to nested variations and the application of element styles. It might be worth holding off on further testing and review of this PR until I can find a solution there or at least document the problems in further detail. |
aaronrobertshaw
force-pushed
the
try/extending-block-style-variations
branch
from
2abb7fc
to
15270b6
Compare
…ent" This reverts commit 15270b6.
I don't know why the class was final I think it was probably already final on the Gutenberg code and during the backport, I kept it as it was. I could think that the API may not have been ready at the time and making it final avoids unexpected extensibility using that class. |
|
There's a start on an alternate approach in #57908 that seems to work for nesting application of block style variations. It isn't fully functional yet. It still needs the root styles of a block style variation to be generated properly in the editor, lots of clean up etc. The key to it though is reducing the specificity of global block styles so needs rebasing onto #57841 as that progresses. With that said, if #57841 proves not to be possible, then this alternate approach will also be dead in the water 😅 |
|
Thank you everyone for your help on this PR 🙇 I'm going to close it though in favour of pursuing the alternate approach in #57908. The reasoning is that even if we took the quick option to bump the block instance element styles so they correctly overrode block style variation element styles, a user still can't reliably apply variation to a block that is within another block that also has a variation applied. If you are still interested in following this feature, more updates will be made via the section styling tracking issue and PRs linked there. |
Related:
What?
Extends block style variations to include support for element and inner block type styles as well as registering across multiple block types.
Why?
Enhancing block style variations is an initial step to enabling section based styling.
What?
Extends block style variations to include support for element and inner block type styles as well as registering styles across multiple block types via
register_block_style.How?
gutenberg_register_block_styleglobal function.WP_Block_Styles_Registrywas madefinalso this was the only straightforward way to extend the block style registration there until it can be backported to core.WP_Theme_JSON_Resolver_Gutenbergclass will now absorb block style variations registered viaWP_Block_Styles_Registryinto the theme's theme.json data.WP_Theme_JSON_Gutenbergwas updated to:WP_Block_Styles_Registryinstead of stripping them out during sanitizationuseGlobalStylesOutputhook updated to generate the extended block style variation styles for the site editorTesting Instructions
The following snippets may be used or provide guidance for testing this PR. They were added to a copy of TT3 for local testing and the demo videos below.
Theme.json snippet for variations
PHP snippet to register some custom block style variations
WARNING: If editing theme.json variations that were registered via
gutenberg_register_block_styleacross multiple block types, keep in mind until the theme.json API is extended you might have to update multiple variations in the theme.jsonstyle_datain the style's propertiesnpm run test:unit:php:base -- --filter WP_Theme_JSON_Gutenberg_Testnpm run test:unit:php:base -- --filter WP_Theme_JSON_Resolver_Gutenberg_Testunfiltered_htmlcapabilities (or emulating via addingadd_action( 'init', 'kses_init_filters' );to your theme functions.php, confirm you can customize element styles for block style variations.Screenshots or screencast
Screen.Recording.2023-12-14.at.7.10.36.pm.mp4