Skip to content

Fix regression with theme.json settings.shadow.defaultPresets #6295

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
+55 −9
Closed

Fix regression with theme.json settings.shadow.defaultPresets #6295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e1dd97f
opt-in to shadow presets only in classic themes
ajlende Mar 20, 2024
f838ba3
Fix WP_Test_REST_Themes_Controller schema test
ajlende Mar 20, 2024
4cc07b0
Fix PHP lint
ajlende Mar 20, 2024
43bddac
Fix PHP lint
ajlende Mar 20, 2024
9006ad2
Fix editor-shadow-presets
ajlende Mar 21, 2024
ce570a5
Rename shadow to shadow-presets
ajlende Mar 21, 2024
3845b40
Add inline docs for editor-shadow-presets
ajlende Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/wp-includes/block-editor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,5 +844,10 @@ function get_classic_theme_supports_block_editor_settings() {
$theme_settings['gradients'] = $gradient_presets;
}

$shadow_presets = current( (array) get_theme_support( 'editor-shadow-presets' ) );
if ( false !== $shadow_presets ) {
$theme_settings['shadow_presets'] = $shadow_presets;
}

return $theme_settings;
}
13 changes: 13 additions & 0 deletions src/wp-includes/class-wp-theme-json-resolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,19 @@ public static function get_theme_data( $deprecated = array(), $options = array()
}
$theme_support_data['settings']['color']['defaultGradients'] = $default_gradients;

if ( ! isset( $theme_support_data['settings']['shadow'] ) ) {
$theme_support_data['settings']['shadow'] = array();
}
$default_shadows = false;
if ( current_theme_supports( 'default-shadow-presets' ) ) {
$default_shadows = true;
}
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;
}
Comment on lines +322 to +325
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@ajlende ajlende Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@andrewserong andrewserong Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

Copy link
Author

@ajlende ajlende Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ajlende ajlende Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@ajlende ajlende Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know, thanks!

$theme_support_data['settings']['shadow']['defaultPresets'] = $default_shadows;

// Allow themes to enable link color setting via theme_support.
if ( current_theme_supports( 'link-color' ) ) {
$theme_support_data['settings']['color']['link'] = true;
Expand Down
8 changes: 7 additions & 1 deletion src/wp-includes/class-wp-theme-json.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,6 @@ public static function get_element_class_name( $element ) {
array( 'spacing', 'margin' ),
array( 'spacing', 'padding' ),
array( 'typography', 'lineHeight' ),
array( 'shadow', 'defaultPresets' ),
);

/**
Expand Down Expand Up @@ -3295,6 +3294,13 @@ public static function get_from_editor_settings( $settings ) {
$theme_settings['settings']['color']['gradients'] = $settings['gradients'];
}

if ( isset( $settings['shadow_presets'] ) ) {
if ( ! isset( $theme_settings['settings']['shadow'] ) ) {
$theme_settings['settings']['shadow'] = array();
}
$theme_settings['settings']['shadow']['presets'] = $settings['shadow_presets'];
}

if ( isset( $settings['fontSizes'] ) ) {
$font_sizes = $settings['fontSizes'];
// Back-compatibility for presets without units.
Expand Down
2 changes: 1 addition & 1 deletion src/wp-includes/theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@
"text": true
},
"shadow": {
"defaultPresets": false,
"defaultPresets": true,
"presets": [
{
"name": "Natural",
Expand Down
27 changes: 27 additions & 0 deletions src/wp-includes/theme.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2642,6 +2642,7 @@ function get_theme_starter_content() {
* @since 6.3.0 The `border` feature allows themes without theme.json to add border styles to blocks.
* @since 6.5.0 The `appearance-tools` feature enables a few design tools for blocks,
* see `WP_Theme_JSON::APPEARANCE_TOOLS_OPT_INS` for a complete list.
* @since 6.5.0 The `editor-shadow-presets` feature is added for adding custom shadow presets to the editor.
*
* @global array $_wp_theme_features
*
Expand All @@ -2668,6 +2669,7 @@ function get_theme_starter_content() {
* - 'disable-layout-styles'
* - 'editor-color-palette'
* - 'editor-gradient-presets'
* - 'editor-shadow-presets'
* - 'editor-font-sizes'
* - 'editor-styles'
* - 'featured-content'
Expand Down Expand Up @@ -4220,6 +4222,31 @@ function create_initial_theme_features() {
),
)
);
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',
),
),
),
),
),
)
);
Comment on lines +4225 to +4249
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ajlende ajlende Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

register_theme_feature(
'editor-styles',
array(
Expand Down
3 changes: 2 additions & 1 deletion tests/phpunit/tests/rest-api/rest-themes-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,14 +410,15 @@ public function test_get_item_schema() {
$this->assertArrayHasKey( 'editor-color-palette', $theme_supports );
$this->assertArrayHasKey( 'editor-font-sizes', $theme_supports );
$this->assertArrayHasKey( 'editor-gradient-presets', $theme_supports );
$this->assertArrayHasKey( 'editor-shadow-presets', $theme_supports );
$this->assertArrayHasKey( 'editor-styles', $theme_supports );
$this->assertArrayHasKey( 'formats', $theme_supports );
$this->assertArrayHasKey( 'html5', $theme_supports );
$this->assertArrayHasKey( 'post-thumbnails', $theme_supports );
$this->assertArrayHasKey( 'responsive-embeds', $theme_supports );
$this->assertArrayHasKey( 'title-tag', $theme_supports );
$this->assertArrayHasKey( 'wp-block-styles', $theme_supports );
$this->assertCount( 23, $theme_supports, 'There should be 23 theme supports' );
$this->assertCount( 24, $theme_supports, 'There should be 24 theme supports' );
}

/**
Expand Down
6 changes: 0 additions & 6 deletions tests/phpunit/tests/theme/wpThemeJson.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,6 @@ public function test_get_settings_appearance_true_opts_in() {
'typography' => array(
'lineHeight' => true,
),
'shadow' => array(
'defaultPresets' => true,
),
'blocks' => array(
'core/paragraph' => array(
'typography' => array(
Expand Down Expand Up @@ -334,9 +331,6 @@ public function test_get_settings_appearance_true_opts_in() {
'typography' => array(
'lineHeight' => false,
),
'shadow' => array(
'defaultPresets' => true,
),
),
),
);
Expand Down