Global Styles: Fix handling of booleans when stabilizing block supports

[aaronrobertshaw]

Related:

• Block Supports: Extend stabilization to common experimental block support flags #67018

What?

Fixes issue introduced by #67018 resulting in mishandling of boolean config values for experimental block supports that are being stabilized e.g. __experimentalBorder.

This problem was raised on the backport PR for block support stabilization.

Why?

Without a fix, this bug will throw a fatal error when experimental block supports with boolean config values are stabilized.

How?

• Early return has been added to the util function stabilizing experimental configs if we're working with a boolean value.
• When both experimental and stable keys are present, there's now a safeguard preventing the attempted merge of a boolean with an array that previously resulted in the fatal error

Testing Instructions

1. Pick a block that adopts border support e.g. Group
2. Add a register_block_type_args filter that replaces the supports.__experimentalBorder config with a boolean true or false
3. Check that there are no errors and the block support functions or doesn't function as you configured
4. Update the filter to replace the previous tweak with the addition of a stable border key that uses a boolean value.
5. Confirm again there are no errors and its working as expected.
6. Unit tests have also been added to the border support tests. Make sure they are passing.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: ramonjd <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

Flaky tests detected in 92e48c3.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12155128978
📝 Reported issues:

• [Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

[ramonjd]

Thanks for the follow up!

To test I updated the group block's border support to stable, e.g.,

		"border": {
            ...
		},

Then added a filter that sets __experimentalBorder to false:

function disable_group_borders( $args ) {
	if ( 'core/group' === $args['name'] ) {
		_wp_array_set( $args, array( 'supports', '__experimentalBorder' ), false );
	}

	return $args;
}
add_filter( 'register_block_type_args', 'disable_group_borders', 20 );

In trunk I get the fatal error:

Fatal error: Uncaught TypeError: array_merge(): Argument #2 must be of type array, bool given in /var/www/html/wp-content/plugins/gutenberg/lib/compat/wordpress-6.8/blocks.php:128

In this PR, the border support is disabled as expected.

[ramonjd]

One thing I noticed while testing is that setting border supports to true disables the support completely.

function disable_group_borders( $args ) {
	if ( 'core/group' === $args['name'] ) {
		_wp_array_set( $args, array( 'supports', '__experimentalBorder' ), true ); // or stable 'border' as well.
	}

	return $args;
}
add_filter( 'register_block_type_args', 'disable_group_borders', 20 );

Is that expected?

This is the state in trunk too, so nothing to fix in this PR as far as I'm aware.

[aaronrobertshaw]

Is that expected?

It's not what I'd expect 😕

Both the JS and PHP checks for border support appear to take the boolean true into account.

I'll see what I can find out on this front shortly. If, for the most part, this fix is working, we could merge it while I iterate on fixing the other issue.

[aaronrobertshaw]

I've tracked this down to a regression introduced when the border panel was combined for the block inspector and global styles #48636.

The combined border panel assumed it only had to check the supports.border.{feature} property instead of seeing how the previous hasBorderSupport function worked.

So the good news is that the recent block support stabilization didn't break it. The bad news is we broke that a long time ago!

I'll get this PR merged and spin up something fresh to fix that regression. I have a suspicion the merging of panels for all the other block supports, e.g. typography, will have broken the same thing 😅

[ramonjd]

I've tracked this down to a regression introduced when the border panel was combined for the block inspector and global styles #48636.

Thanks for taking the time to investigate. And excellent detective work.

I'll get this PR merged and spin up something fresh to fix that regression.

If you have time, thanks! Let me know if I can help.

[aaronrobertshaw]

If you have time, thanks! Let me know if I can help.

Thanks @ramonjd, might need it! To make all the block supports consistent is looking like a bigger job than I hoped.

• Typography: Doesn't appear to have ever supported the boolean value for the entire support config.
• Colors: PHP handling supports the boolean value but only enables text and background features when the config is true. In JS though it doesn't look like it ever did support it and would have thrown a runtime error if colorSupport was a boolean.
• Dimensions: Regressed in #48070. PHP supports boolean config still, JS regressed and doesn't anymore.
• Borders: Regressed in #48636. PHP supports boolean config still, JS regressed and doesn't anymore.
• Spacing: This regressed in #48070 as well. PHP supports boolean config still, JS regressed and doesn't anymore.

This gets murkier though when we look at the theme.json schema, particularly given how this schema has been regularly out of date and doesn't contain experimental supports e.g. border.

• Border: not in schema as it was experimental
• Color: object, no mention of boolean type
• Dimensions: object, no mention of boolean type
• Spacing: object, no mention of boolean type

I had a quick look and didn't spot any submitted issues around using boolean values to toggle on all features for block supports. It might not be something that is currently leaned on by extenders. They might have also just been forced into object-based configs because of bugs 🤷

@youknowriad If you have a moment, it'd be great to tap your wisdom on what the desired outcome here should be. Do we want to support boolean opt-in/out of block support features or should we remove the fragments of that support that remain?

If there's no obvious answer, I can spin up a dedicated issue for broader discussion.

[youknowriad]

If you have a moment, it'd be great to tap your wisdom on what the desired outcome here should be. Do we want to support boolean opt-in/out of block support features or should we remove the fragments of that support that remain?

I think it's a nice little touch, but yeah maybe a hard sell for "color" because of the backwards compatibility unless we do an apiV4 at some point.

[ramonjd]

To make all the block supports consistent is looking like a bigger job than I hoped.

For the supports that never worked that way, I suppose we can categorize it as a logical inconsistency and not a blocker for anything. I really appreciate the investigation and reporting here!

We could also be techocratic about it and say, even though Dimensions, Borders and Spacing regressed more than a year ago, there was nothing in the spec to say they should have worked that way.

Given the time span since the "regression" and today, and the lack of complaints I think it'd be possible re-introduce as an enhancement in a non-urgent fashion.

At least, that's my take away. You did all the hard work 😅 - what you think?

[aaronrobertshaw]

Quick update:

As discussed in #67729 (comment), the approach to block support stabilization needs more time so will likely be punted from 6.8. There is a revert PR available (#68163) and anyone interested in stabilized block supports is advised to follow that for the latest 🙏

[karthick-murugan]

@aaronrobertshaw - As mentioned in this PR #68163 do we need to revert all the changes in this PR? After reverting the changes, how do we test it? Please advice in this regards. Thanks in advance.

[aaronrobertshaw]

do we need to revert all the changes in this PR?

Yes, this PR only relates to fixing the use of boolean values for entire block supports while stabilizing them. Reverting it completely forms part of taking things back to where they were before recent stabilization efforts.

After reverting the changes, how do we test it?

Test instructions are already on the revert PR. They effectively consist of making sure that block supports function as expected, this includes skipping serialization and default controls config.