Shadows: disable shadow in theme.json block settings and in classic themes

[madhusudhand]

What?

This PR addresses the following

1. Allow the shadow control to be removed based on the block settings in theme.json

{
  "settings": {
     "blocks": {
        "core/button": {
           "border": {
		"radius": false
           },
           "shadow": false
        }
     }
  }
}

2. Remove shadow control in classic themes as the default presets are disabled by default and classic themes cannot opt-in (for now)

WORK IN PROGRESS

Fixes: #58908

[github-actions]

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/class-wp-theme-json-gutenberg.php
❔ lib/class-wp-theme-json-resolver-gutenberg.php

[madhusudhand]

Without it shadow setting override in lib/class-wp-theme-json-resolver-gutenberg.php is removed by the sanitize call.

[andrewserong]

I think it'd be better if we can to avoid overriding a property that's already set by static::VALID_SETTINGS. I have an idea, I'll put it in the main review comment 🙂

[andrewserong]

Nice work @madhusudhand, I think the overall direction is pretty close!

I have an idea: what if instead of setting settings.shadow to false, we set settings.shadow.defaultPresets to false. That way we wouldn't need the change to the sanitize method in the main theme JSON class.

Trying that locally, it reveals that I think the checks for displaying the Shadow controls have another bug in them, which is that the controls are displayed even when the presets are empty:

So, unless I'm missing something, I think a path forward could be:

• In Classic themes or themes without their own theme.json file, set settings.shadow.defaultPresets to false.
• Update the checks that determine whether to show the Shadow controls to check if any presets exist, rather than just whether settings.shadow is truthy.

Would that work?

[andrewserong]

Rather than setting shadow to false, could we set settings.shadow.defaultPresets to false?

[andrewserong]

I think it'd be better if we can to avoid overriding a property that's already set by static::VALID_SETTINGS. I have an idea, I'll put it in the main review comment 🙂

[madhusudhand]

In Classic themes or themes without their own theme.json file, set settings.shadow.defaultPresets to false.
Update the checks that determine whether to show the Shadow controls to check if any presets exist, rather than just whether settings.shadow is truthy.

@andrewserong Since shadow dropdown shows both default presets (5 presets that comes with gutenberg) and theme presets (provided via theme.json) together, dropdown cannot be hidden in the UI only based on settings.shadow.defaultPresets

Red: default presets, Blue: theme presets

I am not sure if its a good idea to consider hiding the shadow panel based on total number of presets available, instead of a setting.

[madhusudhand]

@andrewserong @aaronrobertshaw Similar to the shadow support key (it can either be boolean or object), block settings also needs to support the similar structure in order to disable to control from theme.json.

example: theme.json

{
  "settings": {
     "blocks": {
        "core/button": {
           "border": {
		"radius": false
           },
           "shadow": false
        }
     }
  }
}

Also, since this issue #58766 disables defaultPresets by default, in case of classic themes having an empty dropdown doesn't make sense. Setting the shadow setting to false

This isn't just enough as the sanitize function call removes the key as it always expects it to be array.

} elseif ( is_array( $schema[ $key ] ) && ! is_array( $tree[ $key ] ) ) {
	unset( $tree[ $key ] );
}

Skipping the unset conditionally (by checking if the key is shadow) fixes the issue, but hard coding condition doesn't seem to be nice.
Please share your thoughts.

[github-actions]

Flaky tests detected in 239e9e8.
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/7899379632
📝 Reported issues:

• [Flaky Test] default to my most recently created menu #58976 in /test/e2e/specs/editor/blocks/navigation.spec.js

[scruffian]

I am not sure if its a good idea to consider hiding the shadow panel based on total number of presets available, instead of a setting.

This seems like a good idea to me.

[andrewserong]

Thanks for the discussion, folks! I haven't spent as much time with the shadow or effects behaviour, so there could very well be some context I'm missing.

I am not sure if its a good idea to consider hiding the shadow panel based on total number of presets available, instead of a setting. [....]
Also, since this issue #58766 disables defaultPresets by default, in case of classic themes having an empty dropdown doesn't make sense.

What did you have in mind with that issue? It looks like there's also some feedback on #58766 (comment) about hiding the controls if no presets are available. I think the lack of any presets being available should cause the control not to be displayed, but I'll try to explain my thinking a little more below.

Since shadow dropdown shows both default presets (5 presets that comes with gutenberg) and theme presets (provided via theme.json) together, dropdown cannot be hidden in the UI only based on settings.shadow.defaultPresets

What I had in mind is a little more subtle. If we set classic themes to have settings.shadow.defaultPreset be false, then the presets will be empty because the theme doesn't have a theme.json file to provide its own shadows.

Then, the logic for the control would check if any presets are available before deciding whether or not to render it (so not checking settings.shadow.defaultPreset, but rather if there are any presets available). At the moment, with a blocks theme, the control looks a bit broken when there are no presets available, so I thought it could be a neater approach overall if the control was hidden when there are no presets. Here's how TT4 looks if we set settings.shadow.defaultPresets to false:

Overall, I know this is potentially a bit nitpicky, but where we can, I think it's good if we can simplify the logic between classic and block themes so that things can happen a little more implicitly. The sanitize function in the Theme JSON class is already pretty complex, so if we can avoid adding more hard-coded rules in that function for particular properties, I think that would be better.

[aaronrobertshaw]

Thanks for all the effort on shadows @madhusudhand, it's appreciated 👍

Unfortunately, I have a few concerns around the current approach. There shouldn't be any need to add special case handling here to the theme.json sanitization or resolver.

I might be missing some context but is there a reason that shadow support was opt-out by default?

My understanding was that all new block supports really should be opt-in by default. If they applied in the majority of cases or themes, they could be enabled via appearanceTools.

That way, themes that were in a position to capitalize on the latest and greatest block supports could simply turn on appearanceTools and automatically profit. In this case, it would almost be like shadow support being opt-out by default but only for the themes that could handle it.

Given this support hasn't yet been in a major release and only recently landed in the Gutenberg plugin, that is to a degree experimental, I think we still have an opportunity to fix up shadow support so that not only does it function better, not require special handling within the code, but it would be consistent with other supports.

The unlikely edge case of a site using a classic theme with the version of Gutenberg containing shadow support active and having applied it to buttons already, is not one I think we need to cater for. In that rare case, there would be a technical workaround to clear a shadow via the code editor (not ideal for non-power users but still the option is there).

Also, we don't currently display controls for reset purposes when a theme sets a style via theme.json or global styles when the block doesn't have support etc.

So ultimately, my proposal would be:

1. Make shadow support opt-in by default (matches other block supports e.g border)
2. Enable shadow support via appearanceTools
3. Only display shadow control if the control is enabled in settings, the block has shadow support, and there are available presets.
4. Do not display control for reset purposes due to edge cases.

I would class the current state of shadow support being opt-out by default a bug given its impact. That might still leave the door open to fixing this during the 6.5 beta period.

What do you think?

[mikachan]

I believe this can be closed in favour of #58766, please re-open if necessary.