-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Comments
WP6.5 enables shadow UI at the block level. I think one of the reasons why 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. |
I'll defer to @ajlende and @madhusudhand. |
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. |
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 |
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 gutenberg/lib/class-wp-theme-json-resolver-gutenberg.php Lines 297 to 351 in 5f6488e
I can update things there, and I think that should help. |
So the default shadows show up if a theme has not provided their own shadows with @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 |
There looks like a lot of core changes that need to be backported to Gutenberg in order to add the EDIT: WordPress/wordpress-develop#6295 is now open with those changes. |
I have re-read this comment multiple times #59989 (comment) but I don't follow. I don't understand what this is
Or how it is related to gradients
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. |
OK; reading the linked core PR made more sense. I would expect it to work like in this example for the color palette: 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' ); |
I agree. We continue to have issues with this when new features are added. 😞
That was a typo. My bad. I intended to say
The PR I just opened should be more helpful to see the actual code changes. WordPress/wordpress-develop#6295 |
Good, you found it. 😄
That's what I have set up right now. Judging from the code, it looks like the default colors are opt-out by setting 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;
} |
I can go ahead and make shadows opt-in underneath the 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) |
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 LinksA 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 concernsThese are the concerns that I've seen raised. Expand the details for sources. Block themes regression.
Source #58766 (comment).
Source WordPress/wordpress-develop#6295. UI for classic themes.
Source #58908.
Source #59989 (comment).
Source Trac #60633 (comment). Shadow presets may not look good on existing themes.
Source Trac #60815 (comment). Source Trac #60815 Consistency with other presets
Source Slack #6-5-release-leads discussion
Source Slack #6-5-release-leads discussion
Source #58298 (comment).
Source #59989 (comment).
Source Theme support docs.
Source #59989 (comment).
Source #59989 (comment). Gutenberg UI panel title.
Source WordPress/wordpress-develop#6295 (review).
Source Trac #60815 (comment). Size of changes and new features in an RC or patch release.
Source Trac #60815 (comment). Existing defaultsThere 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.
This isn't quite the whole story because classic and block themes differ when custom presets are defined in a block theme's Default presets with custom presets defined are not very consistent.
There is an inconsistency in Consistency isn't everything. We introduced Proposed solutionsThere are three proposed changes to resolve the regression that affect the UI differently. WordPress/wordpress-develop#6295: Color/gradient consistency.
WordPress/wordpress-develop#6303: Minimal CSS regression fix.
NOTE: We don't have a good mechanism for changing defaults for classic themes like we have with theme.json WordPress/wordpress-develop#6309: Disabled in classic themes.
My suggestionWordPress/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.
|
Thank @ajlende for bringing all of the discussions together. |
I've been looking at this and this is what I see:
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. |
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. |
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. 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. |
Is there any chance we can add some automated tests to increase confidence? |
Contributors are looking into this now. |
I've added a couple of tests for the key points of the fix at WordPress/wordpress-develop@522ff61
|
@swissspidy This PR has now:
Can you re-review and (hopefully) commit? |
There are multiple open PRs. To clarify, you mean WordPress/wordpress-develop#6309? |
Yes. It appears I didn't created the link on the words |
Fixed in WordPress/wordpress-develop#6309 (comment). Closing |
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
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.
appearanceTools
is alsofalse
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 andsettings.shadow.defaultPresets
andsettings.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.
It's because to the
settings.shadow.defaultPresets
being used as the value forprevent_override
inPRESETS_METADATA
inWP_Theme_JSON
. The setting defines the behavior of matching slugs in the default and theme origins for presets in theme.json.gutenberg/lib/class-wp-theme-json-gutenberg.php
Line 181 in cdfd751
gutenberg/lib/class-wp-theme-json-gutenberg.php
Lines 2868 to 2876 in cdfd751
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)
The text was updated successfully, but these errors were encountered: