Directory

Backwards compatibility issue with shadow defaultPresets · Issue #59989 · WordPress/gutenberg · GitHub
Skip to content
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.

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

Backwards compatibility issue with shadow defaultPresets #59989

Closed
getdave opened this issue Mar 19, 2024 · 25 comments · Fixed by #60204
Closed

Backwards compatibility issue with shadow defaultPresets #59989

getdave opened this issue Mar 19, 2024 · 25 comments · Fixed by #60204
Assignees
Labels
[Type] Regression Related to a regression in the latest release

Comments

@getdave
Copy link
Contributor

getdave commented Mar 19, 2024

Trac ticket: https://core.trac.wordpress.org/ticket/60815

There may be an issue for themes that have shadow presets with the same slug as the core presets as it changes the CSS output for the theme.


I see that it may break a shadow when the following conditions are met

appearanceTools: false
theme defined custom shadows
and they are defined with same slug as defaultPresets

Also, prior to 6.5 shadows are enabled for button block and in global styles only.
It seems to be very unlikely edge case and shouldn't have big impact on themes.


Meaning that defaultPresets are off by default and when appearanceTools is set to true, they are enabled, which makes existing themes that are using default presets to work as before.

appearanceTools is also false by default, so it's still a problem for themes that haven't configured either.

Before this change, with a theme that has custom settings.shadow.presets defined with matching slugs to those in the defailt lib/theme.json and settings.shadow.defaultPresets and settings.appearanceTools both undefined, you'll end up with different CSS that gets output to the page.

In the first case the default styles will be output and in the second case, the theme styles will be output.

Here's an example of what I'm talking about.

// Relevant parts of theme.json
{
  "settings": {
    // `appearanceTools` is not set
    "shadow": {
      // `defaultPresets` is not set
      "presets": [
        {
          "name": "Natural",
          "slug": "natural",
          "shadow": "2px 2px 3px #000"
        }
      ]
    }
  }
}
/* Generated CSS before */
--wp--preset--shadow--natural: 6px 6px 9px rgba(0, 0, 0, 0.2);
/* Generated CSS after */
--wp--preset--shadow--natural: 2px 2px 3px #000;

It's because to the settings.shadow.defaultPresets being used as the value for prevent_override in PRESETS_METADATA in WP_Theme_JSON. The setting defines the behavior of matching slugs in the default and theme origins for presets in theme.json.

'prevent_override' => array( 'shadow', 'defaultPresets' ),

// Filter out default slugs from theme presets when defaults should not be overridden.
if ( 'theme' === $origin && $prevent_override ) {
$slugs_node = static::get_default_slugs( $this->theme_json, $node['path'] );
$preset_global = _wp_array_get( $slugs_global, $preset_metadata['path'], array() );
$preset_node = _wp_array_get( $slugs_node, $preset_metadata['path'], array() );
$preset_slugs = array_merge_recursive( $preset_global, $preset_node );
$content = static::filter_slugs( $content, $preset_slugs );
}

I'm suggesting that we revert this for 6.5 (at least this line since it is the one causing the problems). Then we can use the theme.json version bump to opt-in to breaking changes like this.

I'm adding the theme.json version bump in #58409. So you'd just need to update the migration from that PR to make sure that existing themes are unaffected.

Hopefully that all makes sense. 😅

Originally posted by @ajlende in #58766 (comment)

@t-hamano
Copy link
Contributor

WP6.5 enables shadow UI at the block level. I think one of the reasons why defaultPresets was changed to false is that there is no way to change default presets in classic themes that do not have theme.json. Therefore, I suspect that simply reverting #58766 or increasing the theme.json version will not be able to resolve the issue.

Is it possible to cover this backward compatibility issue in a Dev Note? I expect that the themes that will be affected will be very limited.

@getdave
Copy link
Contributor Author

getdave commented Mar 19, 2024

I'll defer to @ajlende and @madhusudhand.

@ajlende
Copy link
Contributor

ajlende commented Mar 19, 2024

I think one of the reasons why defaultPresets was changed to false is that there is no way to change default presets in classic themes that do not have theme.json

What was the status of shadows in classic themes prior to this change?

This is what I see now on Twenty Twenty.

image
body {
	--wp--preset--shadow--natural: 6px 6px 9px rgba(0, 0, 0, 0.2);
	--wp--preset--shadow--deep: 12px 12px 50px rgba(0, 0, 0, 0.4);
	--wp--preset--shadow--sharp: 6px 6px 0px rgba(0, 0, 0, 0.2);
	--wp--preset--shadow--outlined: 6px 6px 0px -3px rgba(255, 255, 255, 1),
		6px 6px rgba(0, 0, 0, 1);
	--wp--preset--shadow--crisp: 6px 6px 0px rgba(0, 0, 0, 1);
}

The only difference I see is that buttons can have shadows in classic themes when defaultPresets is true by default. I guess I don't understand why that is a problem?

image
body {
	--wp--preset--shadow--natural: 6px 6px 9px rgba(0, 0, 0, 0.2);
	--wp--preset--shadow--deep: 12px 12px 50px rgba(0, 0, 0, 0.4);
	--wp--preset--shadow--sharp: 6px 6px 0px rgba(0, 0, 0, 0.2);
	--wp--preset--shadow--outlined: 6px 6px 0px -3px rgba(255, 255, 255, 1),
		6px 6px rgba(0, 0, 0, 1);
	--wp--preset--shadow--crisp: 6px 6px 0px rgba(0, 0, 0, 1);
}

If there's an issue with showing the shadow presets in classic themes, that can be handled in the UI instead of the theme.json for now.

@ajlende ajlende changed the title Backwards compatibility issue with defaultPresets Backwards compatibility issue with shadow defaultPresets Mar 19, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Mar 19, 2024
@ajlende
Copy link
Contributor

ajlende commented Mar 19, 2024

I opened #60000 with the change so that folks can play around with it rather than just seeing on my screenshots. Feel free to commit directly to that branch or make modifications to it.

With the impact being probably quite low, I don't know how important this is for getting into an RC, but at the very least I think it should make it into a point release (6.5.1) if it isn't deemed important enough for now.

@andrewserong
Copy link
Contributor

The only difference I see is that buttons can have shadows in classic themes when defaultPresets is true by default. I guess I don't understand why that is a problem?

This is where it was flagged as an issue: #58908 TL;DR (if I'm getting it right) is that it was unexpected for classic themes to receive new visual controls or preset rules without having to opt in to anything, and the appearance tools flag seems to be a consistent way to expose new controls. Now that classic themes can opt in to appearance-tools (as of #56131 being resolved) it'd be better for the shadow controls to be exposed when that's set, rather than by default.

@ajlende
Copy link
Contributor

ajlende commented Mar 20, 2024

This is where it was flagged as an issue: #58908

Thanks for the link!

I understand the problem better now. The lib/theme.json isn't the place to add those changes for classic themes. It looks like they're handled in the resolver in a ! wp_theme_has_theme_json() block.

if ( ! wp_theme_has_theme_json() ) {
if ( ! isset( $theme_support_data['settings']['color'] ) ) {
$theme_support_data['settings']['color'] = array();
}
$default_palette = false;
if ( current_theme_supports( 'default-color-palette' ) ) {
$default_palette = true;
}
if ( ! isset( $theme_support_data['settings']['color']['palette'] ) ) {
// If the theme does not have any palette, we still want to show the core one.
$default_palette = true;
}
$theme_support_data['settings']['color']['defaultPalette'] = $default_palette;
$default_gradients = false;
if ( current_theme_supports( 'default-gradient-presets' ) ) {
$default_gradients = true;
}
if ( ! isset( $theme_support_data['settings']['color']['gradients'] ) ) {
// If the theme does not have any gradients, we still want to show the core ones.
$default_gradients = true;
}
$theme_support_data['settings']['color']['defaultGradients'] = $default_gradients;
// Allow themes to enable all border settings via theme_support.
if ( current_theme_supports( 'border' ) ) {
$theme_support_data['settings']['border']['color'] = true;
$theme_support_data['settings']['border']['radius'] = true;
$theme_support_data['settings']['border']['style'] = true;
$theme_support_data['settings']['border']['width'] = true;
}
// Allow themes to enable link colors via theme_support.
if ( current_theme_supports( 'link-color' ) ) {
$theme_support_data['settings']['color']['link'] = true;
}
if ( current_theme_supports( 'experimental-link-color' ) ) {
_doing_it_wrong(
current_theme_supports( 'experimental-link-color' ),
__( '`experimental-link-color` is no longer supported. Use `link-color` instead.', 'gutenberg' ),
'6.3.0'
);
}
// BEGIN EXPERIMENTAL.
// Allow themes to enable appearance tools via theme_support.
// This feature was backported for WordPress 6.2 as of https://core.trac.wordpress.org/ticket/56487
// and then reverted as of https://core.trac.wordpress.org/ticket/57649
// Not to backport until the issues are resolved.
if ( current_theme_supports( 'appearance-tools' ) ) {
$theme_support_data['settings']['appearanceTools'] = true;
}
// END EXPERIMENTAL.
}

I can update things there, and I think that should help.

@ajlende
Copy link
Contributor

ajlende commented Mar 20, 2024

Now that classic themes can opt in to appearance-tools (as of #56131 being resolved) it'd be better for the shadow controls to be exposed when that's set, rather than by default.

I noticed that the add_theme_support( 'appearance-tools' ) is still experimental, so I don't think they can use that to opt-in. I can add a new default-shadow-presets theme support instead. It can have an accompanying editor-shadow-presets theme support for adding shadow presets. The combination of these follows the same the behavior as the colors and gradients.

So the default shadows show up if a theme has not provided their own shadows with editor-shadow-presets or they have used default-shadow-presets to opt-in to showing the default presets alongside the theme-provided ones.

@carolinan, maybe you can share more insights for how you think default shadows can be handled in classic themes since the implementation of colors and gradients is slightly different from your description in #58908.

EDIT: I see that the experimental appearance-tools part marked for not backporting was actually backported to 6.5, but the comment just wasn't removed in the commit that I was referencing. We could use that instead for shadows, but I'd still like to hear from @carolinan.

@ajlende
Copy link
Contributor

ajlende commented Mar 20, 2024

There looks like a lot of core changes that need to be backported to Gutenberg in order to add the editor-shadow-presets, so I'll open another PR in wordpress/wordpress-develop with those. The changes will need to be there eventually anyway. #60000 can become the backport to Gutenberg PR.

EDIT: WordPress/wordpress-develop#6295 is now open with those changes.

@carolinan
Copy link
Contributor

carolinan commented Mar 20, 2024

I have re-read this comment multiple times #59989 (comment) but I don't follow.

I don't understand what this is

I can add a new default-shadow-presets instead.

Or how it is related to gradients

an accompanying editor-gradient-presets

I know this does not help now, but when new features are added, classic themes need to be considered and part of the planning of the new feature, from the start. It becomes a problem when we need to try to fix it later.

@carolinan
Copy link
Contributor

carolinan commented Mar 20, 2024

OK; reading the linked core PR made more sense.

I would expect it to work like in this example for the color palette:
https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-support/

function  mytheme_setup_theme_supported_features() {
    add_theme_support( 'editor-color-palette', array(
        array(
            'name'  => esc_attr__( 'strong magenta', 'themeLangDomain' ),
            'slug'  => 'strong-magenta',
            'color' => '#a156b4',
        ),
        array(
            'name'  => esc_attr__( 'light grayish magenta', 'themeLangDomain' ),
            'slug'  => 'light-grayish-magenta',
            'color' => '#d0a5db',
        ),
        array(
            'name'  => esc_attr__( 'very light gray', 'themeLangDomain' ),
            'slug'  => 'very-light-gray',
            'color' => '#eee',
        ),
        array(
            'name'  => esc_attr__( 'very dark gray', 'themeLangDomain' ),
            'slug'  => 'very-dark-gray',
            'color' => '#444',
        ),
    ) );
}

add_action( 'after_setup_theme', 'mytheme_setup_theme_supported_features' );

@ajlende
Copy link
Contributor

ajlende commented Mar 20, 2024

I know this does not help now, but when new features are added, classic themes need to be considered and part of the planning of the new feature, from the start. It becomes a problem when we need to try to fix it later.

I agree. We continue to have issues with this when new features are added. 😞

Or how it is related to gradients

an accompanying editor-gradient-presets

That was a typo. My bad. I intended to say editor-shadow-presets. 😅

I don't understand what this is

I can add a new default-shadow-presets instead.

The PR I just opened should be more helpful to see the actual code changes. WordPress/wordpress-develop#6295

@ajlende
Copy link
Contributor

ajlende commented Mar 20, 2024

OK; reading the linked core PR made more sense.

Good, you found it. 😄

I would expect it to work like in this example for the color palette:

That's what I have set up right now. Judging from the code, it looks like the default colors are opt-out by setting add_theme_support( 'editor-color-palette', array() ) based on this line:

https://github.com/ajlende/wordpress-develop/blob/e1dd97f94fc65eac12e2d6921fe237957b194488/src/wp-includes/class-wp-theme-json-resolver.php#L299-L302

if ( ! isset( $theme_support_data['settings']['color']['palette'] ) ) {
	// If the theme does not have any palette, we still want to show the core one.
	$default_palette = true;
}

@ajlende
Copy link
Contributor

ajlende commented Mar 20, 2024

default colors are opt-out by setting add_theme_support( 'editor-color-palette', array() )

I can go ahead and make shadows opt-in underneath the appearance-tools instead if you'd prefer that (as discussed in #58908).

There isn't a whole lot of consistency in how these settings work in classic themes, likely because they are often an afterthought.

I like the consistency with color/gradient presets, but I'll defer to your judgement on what makes the most sense. I feel like you're more connected to the going-ons of classic themes than I am. 🙇

EDIT: More discussion on this happening in WordPress/wordpress-develop#6295 (review)

@ajlende
Copy link
Contributor

ajlende commented Mar 25, 2024

I talked with @getdave in a DM to get him up to speed. He asked that I provide a breakdown of the discussion that has happened so far as well as explain my proposed solution more clearly.

There are a lot of things here, so I've collapsed sections into <details> to make things easier to scan.

Links

A lot of discussion has been spread out across various places as folks have joined the conversation. To my knowledge, this is the definitive list.

Primary concerns

These are the concerns that I've seen raised. Expand the details for sources.

Block themes regression.

This is a breaking change for themes that have shadow presets with the same slug as the core presets as it changes the CSS output for the theme.

Source #58766 (comment).

// 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)"
			}
		}
	}
}
WP 6.4 Current 6.5
Screen Shot 2024-03-19 at 12 56 11 Screen Shot 2024-03-19 at 12 55 36

Source WordPress/wordpress-develop#6295.

UI for classic themes.

I found it surprising that for a classic theme, I did not have to opt-in for the new shadows controls. They are enabled by default on the supported blocks.

Source #58908.

This is where it was flagged as an issue: #58908 TL;DR (if I'm getting it right) is that it was unexpected for classic themes to receive new visual controls or preset rules without having to opt in to anything, and the appearance tools flag seems to be a consistent way to expose new controls. Now that classic themes can opt in to appearance-tools (as of #56131 being resolved) it'd be better for the shadow controls to be exposed when that's set, rather than by default.

Source #59989 (comment).

All new customization tools that are added to blocks are meant to be opt-in behind the appearanceTools flag which ensures that themes that use this flag will get these tools but the ones that choose to control manually all the available tools won't.

Source Trac #60633 (comment).

Shadow presets may not look good on existing themes.

Because of the spacing issues, I wouldn't say that it was good experience using the shadows. They even make some of the blocks overlap.

But, the styling can be improved in any release, I don't consider it a blocker for 6.5.

Source Trac #60815 (comment).

bad looking shadows in the twenty-ten theme

Source Trac #60815

Consistency with other presets

I think for the future, whether that requires a new version or not, we should ideally aim for consistency with other presets (so it's clear for themers who all presets work). Now for this one, given the late stage of the release, we have to take into consideration the impact of the bug/change to make the right decision.

Source Slack #6-5-release-leads discussion

I don’t think we should index on limiting the out-of-the-box core WP functionality, but rather provide it like any other preset.

The alternative to this scenario is much more likely; that people on older themes updating to WordPress get confused because they can’t find the presets.

Source Slack #6-5-release-leads discussion

I would expect shadow presets to work like other default presets—defaultDuotone, defaultPalette, defaultGradients, etc.

Source #58298 (comment).

I would expect it to work like in this example for the color palette: https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-support/

Source #59989 (comment).

Editor Color Palette – A default set of colors is provided, but themes can register their own and optionally lock users into picking from the defined palette.

. . .

Some advanced block features require opt-in support in the theme itself as it’s difficult for the block to provide these styles, they may require some architecting of the theme itself, in order to work well.

To opt-in for one of these features, call add_theme_support in the functions.php file of the theme.

Source Theme support docs.

I can add a new default-shadow-presets theme support instead. It can have an accompanying editor-shadow-presets theme support for adding shadow presets. The combination of these follows the same the behavior as the colors and gradients.

So the default shadows show up if a theme has not provided their own shadows with editor-shadow-presets or they have used default-shadow-presets to opt-in to showing the default presets alongside the theme-provided ones.

@carolinan, maybe you can share more insights for how you think default shadows can be handled in classic themes since the implementation of colors and gradients is slightly different from your description in #58908.

Source #59989 (comment).

Judging from the code, it looks like the default colors are opt-out by setting add_theme_support( 'editor-color-palette', array() ) based on this line:

https://github.com/wordpress/wordpress-develop/blob/e1dd97f94fc65eac12e2d6921fe237957b194488/src/wp-includes/class-wp-theme-json-resolver.php#L299-L302

if ( ! isset( $theme_support_data['settings']['color']['palette'] ) ) {
	// If the theme does not have any palette, we still want to show the core one.
	$default_palette = true;
}

Source #59989 (comment).

Gutenberg UI panel title.

[I]t uncovers a small guternberg UI issue where the label is set as Shadow instead of Border & Shadow . . .

image

Source WordPress/wordpress-develop#6295 (review).

[T]here is a problem with the label of the border and shadow control for the image block and button block. These two blocks have a border radius control that is not dependant on border theme support.

The label says "Shadow" when it also contains the border radius.
This is also confusing because the default control is the radius, the shadow control has to be toggled on in the panel settings.

Source Trac #60815 (comment).

Size of changes and new features in an RC or patch release.

To me there's too much last minute back and forth here and I don't feel comfortable with making yet another change here that isn't a simple revert. Also, there definitely need more unit tests here, or even an e2e test.

Source Trac #60815 (comment).

Existing defaults

There seems to have been some confusion on what existing preset settings are like in block and classic themes.

All presets available are listed in https://github.com/WordPress/wordpress-develop/blob/84c21abf36dc2d8cb2b58c03e0e1f237c0a6b18d/src/wp-includes/class-wp-theme-json.php#L120

Default presets on empty themes are remarkably consistent.
Preset Block theme Classic theme
color.palette shown shown
color.gradients shown shown
color.duotone shown shown
typography.fontSizes shown shown
typography.fontFamilies not provided N/A
spacing.spacingSizes not provided N/A

This isn't quite the whole story because classic and block themes differ when custom presets are defined in a block theme's theme.json or classic theme's functions.php with add_theme_support.

Default presets with custom presets defined are not very consistent.
Preset Block theme Classic theme
color.palette shown hidden
color.gradients shown hidden
color.duotone shown N/A
typography.fontSizes hidden hidden
typography.fontFamilies not provided N/A
spacing.spacingSizes not provided N/A

There is an inconsistency in typography.fontSizes is being fixed in #58409.

Consistency isn't everything. We introduced settings.appearanceTools to allow themes to opt-in to new features. So it was suggested to do this for shadows in #58967 (review).

Proposed solutions

There are three proposed changes to resolve the regression that affect the UI differently.

WordPress/wordpress-develop#6295: Color/gradient consistency.
  • Enables shadow presets UI by default in classic themes and block themes.
  • Allows classic themes to opt out the same way you would opt out of color/gradient presets with add_theme_support( 'editor-shadow-presets', array() ).
  • Introduces editor-shadow-presets and default-shadow-presets for add_theme_support.
  • No follow-ups needed in 6.6.
  • Largest PR.
  • ** For 6.5 or 6.5.1**: Fix UI bug for shadow title in classic themes Fix regression with theme.json settings.shadow.defaultPresets wordpress-develop#6295 (review).
WordPress/wordpress-develop#6303: Minimal CSS regression fix.
  • Enables shadow presets UI by default in classic themes and block themes.
  • Classic themes cannot opt out of shadow presets UI.
  • Block themes can opt out of shadow presets UI with settings.shadow.defaultPresets = false.
  • 6.6 follow-up for consistency with color/gradient presets.
    • add_theme_support( 'editor-shadow-presets', array(...) ) to allow classic themes to define custom shadow presets or opt-out of default presets in the UI by setting an empty array().
    • add_theme_support( 'default-shadow-presets' ) to allow default presets to be opted-in when editor-shadow-presets are defined.
  • Smallest PR.
  • ** For 6.5 or 6.5.1**: Fix UI bug for shadow title in classic themes Fix regression with theme.json settings.shadow.defaultPresets wordpress-develop#6295 (review).

NOTE: We don't have a good mechanism for changing defaults for classic themes like we have with theme.json version for block themes (as far as I know). So we would probably want to keep the shadow presets enabled by default for classic themes going forward and add the option to opt-out if we go this route.

WordPress/wordpress-develop#6309: Disabled in classic themes.
  • Disables shadow presets UI in classic themes. Enables it by default in block themes.
  • Classic themes cannot opt-in to the shadow presets UI.
  • Follow-up for 6.6.
    • add_theme_support( 'appearance-tools' ) to enable shadow support in classic themes.
    • add_theme_support( 'default-shadow-presets' ) to allow opting-in to default shadow presets without the rest of appearance tools.
    • add_theme_support( 'editor-shadow-presets', array(...) ) to allow classic themes to define custom shadow presets.
    • Migrate block themes to disable shadow preset UI by default with theme.json v3.
      • By updating to theme.json v3, themes can opt-in to hiding shadow preset UI by default.
      • Themes with theme.json v2 remain unaffected and still have shadows enabled by default.

My suggestion

WordPress/wordpress-develop#6309 (disabled in classic themes) seems like the best option to me. It's hard to take back settings by default once they are available to users.

  1. It fixes the regression.
  2. Defers the decision for enabling default shadow presets in classic themes until 6.6. (No change from 6.4 to 6.5 for classic themes.)
  3. Maintains consistency with other presets for block themes.
  4. Does not require changes to the JS in a Gutenberg PR as it sidesteps the border/shadow title bug in classic themes.
  5. Is a relatively small PR that doesn't add any new features.

@madhusudhand
Copy link
Contributor

Thank @ajlende for bringing all of the discussions together.
I agree that WordPress/wordpress-develop#6309 is the best possible solution, also fetches time until 6.6 to decide on a solution for classic themes.

@oandregal
Copy link
Member

oandregal commented Mar 26, 2024

I've been looking at this and this is what I see:

WordPress 6.4 WordPress 6.5 (current) Needs action
Block supports button button, column, columns, image, featured image -
UI Global Styles For blocks that support it. For blocks that support it if the block theme opts-in. Regression. We shouldn't require the block themes to opt-in, as the feature was available for them.
UI Block Inspector None For blocks that support it if the block theme opts-in. Disabled for classic themes We shouldn't require the block themes to opt-in given how they work in the Global Styles UI.
theme.json Shadow can be used it in blocks that support it. Same. -
theme.json: CSS for the shadow The shadow uses the raw value. The shadow uses the CSS Custom Property. -
theme.json: CSS for the preset The theme shadow doesn't override default shadows. The theme shadow overrides default shadows. Regression. We should prevent the theme shadow from overriding the default ones.

WordPress/wordpress-develop#6309 by Alex addresses the regressions I see while maintaining all the requisites outlined for shadows in WordPress 6.5. My recommendation is to merge it as it is.

@swissspidy
Copy link
Member

We're extremely late in the release cycle, so another option is to simply revert https://core.trac.wordpress.org/changeset/57717 and https://core.trac.wordpress.org/changeset/57827 for this release and then revisit this in 6.6. WordPress/wordpress-develop#6309 does a little more than revert. Just wanted to mention it here.

If you all think WordPress/wordpress-develop#6309 resolves the issue and does not cause yet another regression, that's great.

Alternatively, as I mentioned on the ticket, we should also consider punting to 6.5.1 because of all the back and forth.

@madhusudhand
Copy link
Contributor

so another option is to simply revert https://core.trac.wordpress.org/changeset/57717 and https://core.trac.wordpress.org/changeset/57827 for this release and then revisit this in 6.6

The changes 57717 and 57827 were introduced to not enable shadows by default in classic themes. By reverting only those we will hit the same problem again.

6309 introduces following additional change to set defaultPresets to false for classic themes.
Since shadows were never enabled for classic themes, it will not cause any issues.

https://github.com/WordPress/wordpress-develop/pull/6309/files#diff-d6b86476eed058e7cf8b6e57fa52c4fd75b1f0907e1a9ccb0149528a24f7578bR315-R325

I feel confident on 6309, and it will be fine to punt it to 6.5.1 which also gives more time for further testing and feedback.

@swissspidy
Copy link
Member

Is there any chance we can add some automated tests to increase confidence?

@getdave
Copy link
Contributor Author

getdave commented Mar 27, 2024

Contributors are looking into this now.

@oandregal
Copy link
Member

I've added a couple of tests for the key points of the fix at WordPress/wordpress-develop@522ff61

  • test_shadow_default_presets_value_for_block_and_classic_themes: removing the changes to WP_Theme_JSON_Resolver in the PR will make this test fail.
  • test_theme_shadow_presets_do_not_override_default_shadow_presets: removing the changes to theme.json in the PR will make this fail.

@getdave
Copy link
Contributor Author

getdave commented Mar 27, 2024

Is there any chance we can add some automated tests to increase confidence?

@swissspidy This PR has now:

  • been tested by multiple contributors familiar with the concepts involved.
  • triaged by the Editor release leads.
  • had tests added to provide additional confidence.

Can you re-review and (hopefully) commit?

@swissspidy
Copy link
Member

swissspidy commented Mar 27, 2024

There are multiple open PRs. To clarify, you mean WordPress/wordpress-develop#6309?

@getdave
Copy link
Contributor Author

getdave commented Mar 27, 2024

here are multiple open PRs. To clarify, you mean WordPress/wordpress-develop#6309?

Yes. It appears I didn't created the link on the words this PR as I had intended 🤦 Updated above.

@getdave
Copy link
Contributor Author

getdave commented Mar 27, 2024

Fixed in WordPress/wordpress-develop#6309 (comment).

Closing

@getdave getdave closed this as completed Mar 27, 2024
@getdave getdave removed the [Status] In Progress Tracking issues with work in progress label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
Status: Done
8 participants