Fix regression with theme.json `settings.shadow.defaultPresets` by ajlende · Pull Request #6295 · WordPress/wordpress-develop

ajlende · 2024-03-20T17:25:10Z

All discussion for this issue should be done in WordPress/gutenberg#59989

Description

Reverts core theme.json settings.shadow.defaultPresets to true to fix the regression. (Back to working the same as color/gradient preset configuration for block themes.)
Adds editor-shadow-presets and default-shadow-presets for classic themes to configure shadows. (Works the same way as color/gradient preset configuration in classic themes.)

Backport to Gutenberg: WordPress/gutenberg#60000
Gutenberg issue: WordPress/gutenberg#59989
Trac ticket: https://core.trac.wordpress.org/ticket/60815

Testing

Here is an example theme.json for testing that would override the default natural shadow with a red shadow to make the difference obvious.

// theme.json
{
	"$schema": "https://schemas.wp.org/wp/6.4/theme.json",
	"version": 2,
	"settings": {
		"shadow": {
			"presets": [
				{
					"name": "Natural",
					"slug": "natural",
					"shadow": "6px 6px 9px #F00"
				}
			]
		},
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		}
	},
	"styles": {
		"blocks": {
			"core/button": {
				"shadow": "var(--wp--preset--shadow--natural)"
			}
		}
	}
}

Use the above theme.json as an example theme.
Insert a button in a post.
Preview the post in wp/6.4
Preview the post in trunk
Preview the post in this PR
See that wp/6.4 and this PR match.

Regression screenshots

WP 6.4	Current 6.5	This PR

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2024-03-20T17:25:26Z

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.

Core Committers: Use this line as a base for the props when committing in SVN:

Props ajlende, andrewserong, madhudollu, get_dave, swissspidy.

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

github-actions · 2024-03-20T17:44:15Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

andrewserong · 2024-03-20T22:20:15Z

src/wp-includes/class-wp-theme-json-resolver.php

+			if ( ! isset( $theme_support_data['settings']['shadow']['presets'] ) ) {
+				// If the theme does not have any shadows, we still want to show the core ones.
+				$default_shadows = true;
+			}


Just a quick comment, as I haven't looked at this change in detail, but this line stood out to me while passing by. Does this check mean that if the theme doesn't add current_theme_supports( 'default-shadow-presets' ) it'll still wind up getting the default shadow presets?

Yes, that's correct.

I think from all of the discussions that I read, the desire was to go for consistency with other settings, so I copied the behavior from colors and gradients just above. However, I know that behavior is different from what you had talked about in WordPress/gutenberg#58908.

If we want to match the appearance-tools behavior that you were going for there, this can be moved just a few lines lower into the appearance-tools section.

I don't feel too strongly about it, but mostly trying to follow the general discussion from WordPress/gutenberg#58908. My impression was that shadows / effects are a bit of a special case, so guarding behind appearance-tools is likely a better fit so that we're not outputting anything for themes that don't explicitly opt-in either to the presets or to appearance-tools, but @carolinan might have more insight for the Classic themes use case than me here 🙂

And fair warning, I haven't really tested any of this add_theme_support code yet. I wanted to get something pushed up as a starting point that folks could look at. But it's all copied from colors/gradients configuration, so I'm relatively confident that it should be working.

so that we're not outputting anything for themes that don't explicitly opt-in either to the presets

This is often a point of confusion for global styles, but setting default* options to false does not prevent the styles from being output. It just allows for themes to override the default values. What I've been told is that the default styles always need to exist in order to support patterns between themes. The only preset that handles this differently is duotone.

Ah, gotcha.

The only preset that handles this differently is duotone.

I suppose that's the question then: whether shadows are like colors or font sizes, or whether they're more like duotone in that respect. It might be worth getting more opinions on this? I don't feel strongly either way, my main hope is that whichever way it works feels natural and not unexpected to folks. Maybe @madhusudhand might have some thoughts, too?

I suppose that's the question then: whether shadows are like colors or font sizes, or whether they're more like duotone in that respect.

The only reason duotone is different is because of the large SVGs that were also being generated WordPress/gutenberg#38299. Changing it was not an easy feat—we had to essentially rewrite duotone from scratch to do all the things that the global styles classes give you for free. And there are still unresolved issues from making that change WordPress/gutenberg#53662 and WordPress/gutenberg#49293.

For something small like a few lines of shadow presets, I promise it isn't worth it.

Good to know, thanks!

ajlende · 2024-03-20T23:05:36Z

src/wp-includes/theme.php

+	register_theme_feature(
+		'editor-shadow-presets',
+		array(
+			'type'         => 'array',
+			'description'  => __( 'Custom shadow presets if defined by the theme.' ),
+			'show_in_rest' => array(
+				'schema' => array(
+					'items' => array(
+						'type'       => 'object',
+						'properties' => array(
+							'name'   => array(
+								'type' => 'string',
+							),
+							'shadow' => array(
+								'type' => 'string',
+							),
+							'slug'   => array(
+								'type' => 'string',
+							),
+						),
+					),
+				),
+			),
+		)
+	);


Bonus support for custom shadows in classic themes to match other presets.

This is great 🌟
I think it is best to split this change into separate PR (as it would be a new feature), in case if it gets landed in 6.5 or in a patch version.

This was required in order to support these lines and match the behavior of colors/gradients.

https://github.com/WordPress/wordpress-develop/pull/6295/files#diff-d6b86476eed058e7cf8b6e57fa52c4fd75b1f0907e1a9ccb0149528a24f7578bR322-R325

if ( ! isset( $theme_support_data['settings']['shadow']['presets'] ) ) { // If the theme does not have any shadows, we still want to show the core ones. $default_shadows = true; }

If we decide to diverge from the colors/gradients behavior like we're discussing in https://github.com/WordPress/wordpress-develop/pull/6295/files#r1533015533 and in WordPress/gutenberg#59989 (comment), we could remove it from here and add it to a follow-up.

I opened an alternative PR (#6303) with shadows enabled by default in classic themes (like colors/gradients) but without the option to opt-out with add_theme_support( 'editor-shadow-presets', array() ).

Both PRs fix the regression that I'm concerned with.

madhusudhand

Thanks @ajlende for bring this alternate approach for discussion.
It feels right to me and also exposes shadows to classic themes without creating any impact.

I gave it a test in both block and classic themes. It tests well with addition of small code. Also it uncovers a small guternberg UI issue where the label is set as Shadow instead of Border & Shadow, which I guess not because of the current changes. I am going to make a separate fix.

src/wp-includes/class-wp-theme-json-resolver.php

src/wp-includes/block-editor.php

madhusudhand · 2024-03-21T10:32:40Z

src/wp-includes/theme.php

+	register_theme_feature(
+		'editor-shadow-presets',
+		array(
+			'type'         => 'array',
+			'description'  => __( 'Custom shadow presets if defined by the theme.' ),
+			'show_in_rest' => array(
+				'schema' => array(
+					'items' => array(
+						'type'       => 'object',
+						'properties' => array(
+							'name'   => array(
+								'type' => 'string',
+							),
+							'shadow' => array(
+								'type' => 'string',
+							),
+							'slug'   => array(
+								'type' => 'string',
+							),
+						),
+					),
+				),
+			),
+		)
+	);


This is great 🌟
I think it is best to split this change into separate PR (as it would be a new feature), in case if it gets landed in 6.5 or in a patch version.

getdave · 2024-03-25T10:32:57Z

Flagging this to @swissspidy and @dream-encode as a PR that may needing a commit prior to WP 6.5 release.

swissspidy · 2024-03-25T11:31:14Z

@getdave see the discussion on the trac ticket. To me there's too much back and forth on this issue and expected behavior seems unclear at this point. That's why I suggested 6.5.1 for this change. But let's discuss on the ticket please.

swissspidy · 2024-03-27T12:56:25Z

Looks like we're converging towards focusing on #6309, so closing this one.

opt-in to shadow presets only in classic themes

e1dd97f

ajlende mentioned this pull request Mar 20, 2024

Backwards compatibility issue with shadow defaultPresets WordPress/gutenberg#59989

Closed

Fix WP_Test_REST_Themes_Controller schema test

f838ba3

ajlende changed the title ~~opt-in to default shadow presets in classic themes~~ Fix regression with theme.json settings.shadow.defaultPresets Mar 20, 2024

ajlende added 2 commits March 20, 2024 13:33

Fix PHP lint

4cc07b0

Fix PHP lint

43bddac

ajlende mentioned this pull request Mar 20, 2024

Revert shadow.defaultPresets default to true WordPress/gutenberg#60000

Closed

andrewserong reviewed Mar 20, 2024

View reviewed changes

ajlende commented Mar 20, 2024

View reviewed changes

madhusudhand reviewed Mar 21, 2024

View reviewed changes

ajlende added 3 commits March 21, 2024 17:35

Fix editor-shadow-presets

9006ad2

Rename shadow to shadow-presets

ce570a5

Add inline docs for editor-shadow-presets

3845b40

This was referenced Mar 21, 2024

Fix regression with theme.json settings.shadow.defaultPresets (minimal) #6303

Closed

Disable settings.shadow.defaultPresets for classic themes #6309

Closed

madhusudhand mentioned this pull request Mar 26, 2024

Shadow: tools panel label is set to Shadow instead of Border & Shadow WordPress/gutenberg#60192

Open

swissspidy closed this Mar 27, 2024

Conversation

ajlende commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Regression screenshots

Uh oh!

github-actions bot commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 20, 2024

Test using WordPress Playground

Some things to be aware of

Uh oh!

andrewserong Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

ajlende Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewserong Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajlende Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajlende Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewserong Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

ajlende Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewserong Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

ajlende Mar 20, 2024

Choose a reason for hiding this comment

Uh oh!

madhusudhand Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

ajlende Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajlende Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

madhusudhand left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

madhusudhand Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

getdave commented Mar 25, 2024

Uh oh!

swissspidy commented Mar 25, 2024

Uh oh!

swissspidy commented Mar 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ajlende commented Mar 20, 2024 •

edited

Loading

github-actions bot commented Mar 20, 2024 •

edited

Loading

ajlende Mar 20, 2024 •

edited

Loading

andrewserong Mar 20, 2024 •

edited

Loading

ajlende Mar 20, 2024 •

edited

Loading

ajlende Mar 20, 2024 •

edited

Loading

ajlende Mar 21, 2024 •

edited

Loading

ajlende Mar 21, 2024 •

edited

Loading

madhusudhand left a comment •

edited

Loading