Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

#60815 closed defect (bug) (fixed)

Regression with theme.json `settings.shadow.defaultPresets`

Reported by: ajlende's profile ajlende Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.5
Component: Themes Keywords: has-patch needs-testing has-unit-tests fixed-major commit dev-reviewed
Focuses: Cc:

Description

#60633 introduced a regression in shadow defaultPresets. Details and discussion in WordPress/gutenberg#59989.

// 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;

Attachments (14)

6303-twenty-ten-front.png (218.1 KB) - added by poena 4 weeks ago.
Shadows applied in Twenty Ten
twenty-11-front-2024_03_22_shadow.png (2.6 MB) - added by poena 4 weeks ago.
Twenty Eleven with shadows applied, front
twenty-12-front-2024_03_22_shadow.png (590.6 KB) - added by poena 4 weeks ago.
Twenty Twelve, front
twenty-13-front-2024_03_22_shadow.png (1.3 MB) - added by poena 4 weeks ago.
Twenty Thirteen, front
twenty-14-front-2024_03_22_shadow.png (1.1 MB) - added by poena 4 weeks ago.
Twenty Fourteen, front
twenty-15-front-2024_03_22_shadow_.png (778.0 KB) - added by poena 4 weeks ago.
Twenty Fifteen
twenty-16-front-2024_03_22_shadow_.png (1.2 MB) - added by poena 4 weeks ago.
Twenty Sixteen, front
twenty-17-front-2024_03_22_shadow_.png (1.4 MB) - added by poena 4 weeks ago.
Twenty Seventeen, front
twenty-19-front-2024_03_22_shadow_.png (761.2 KB) - added by poena 4 weeks ago.
Twenty Nineteen, front
twenty-twenty-front-2024_03_22_shadow_.png (874.6 KB) - added by poena 4 weeks ago.
Twenty Twenty, front
twenty-twenty-one-front-2024_03_22_shadow_.png (892.9 KB) - added by poena 4 weeks ago.
Twenty Twenty-One, front
twenty-twenty-two-front-2024_03_22_shadow_.png (653.0 KB) - added by poena 4 weeks ago.
Twenty Twenty-Two, front
twenty-twenty-three-front-2024_03_22_shadow_.png (665.8 KB) - added by poena 4 weeks ago.
Twenty Twenty-Three, front
twenty-twenty-four-front-2024_03_22_shadow_.png (715.4 KB) - added by poena 4 weeks ago.
Twenty Twenty-Four, front

Change History (38)

This ticket was mentioned in PR #6295 on WordPress/wordpress-develop by @ajlende.


4 weeks ago
#1

  • Keywords has-patch has-unit-tests added
  • 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: https://github.com/WordPress/gutenberg/pull/60000
Gutenberg issue: https://github.com/WordPress/gutenberg/issues/59989
Trac ticket: https://core.trac.wordpress.org/ticket/60815

#2 @swissspidy
4 weeks ago

  • Keywords needs-testing needs-unit-tests added; has-unit-tests removed

Sounds like we've had nothing but problems with this whole change.

@get_dave @youknowriad would appreciate your thoughts as editor tech leads here.

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.

https://github.com/WordPress/gutenberg/issues/59989 indicates this is more an edge case, so I'd say this is a candidate for 6.5.1.

#3 @ajlende
4 weeks ago

I don't feel comfortable with making yet another change here that isn't a simple revert.

The one line change in theme.json is sufficient to fix the regression.

The intention of setting shadow.defaultPresets to false was to allow classic themes to opt-in to them, and the rest of the changes that are being discussed are to support that the correct way.

If we only include the one line change, the effect will be that classic themes don't have the option to configure shadows and the default shadows will appear in the block editor for classic themes.

#4 @swissspidy
4 weeks ago

In https://core.trac.wordpress.org/ticket/60633#comment:8 it was explicitly suggested to make it true, which then was changed in [57827]. This is the back and forth I mean.

#5 @jorbin
4 weeks ago

  • Milestone changed from Awaiting Review to 6.5

I'm putting this in 6.5 for visibility, but I share @swissspidy's concern about the back and forth. I think in the end this might be better placed in 6.5.1

This ticket was mentioned in PR #6303 on WordPress/wordpress-develop by @ajlende.


4 weeks ago
#6

  • Keywords has-unit-tests added; needs-unit-tests removed

Alternative to #6295 without the editor-shadow-presets theme support feature.

  • 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.)

The color/gradient support behavior that I think we should be matching enables support for the presets in classic themes by default and allows themes to opt-out. For example, gradients can be opted-out of by setting add_theme_support( 'editor-gradient-presets', array() ).

If we're okay with shadows being opt-out in the UI like colors/gradients, we can merge just this and add the ability to opt-out with add_theme_support( 'editor-shadow-presets', array() ) that's included in #6295 as a follow-up in 6.6.

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

#7 @poena
4 weeks ago

Is this summary accurate?

If we don't do anything, then:
1) Neither classic themes or block themes will have shadows enabled by default.

  • This is a change from 6.4.3 where block themes had shadows by default for buttons in the (global) Styles setting sidebar.
  • This is a problem because the defaults should not change without a theme.json version change(?)

2) There is a problem in any themes that has a theme.json, when a developer registers a custom shadow presets with the same slug as the core preset.
3) Classic themes without theme.json can not set their own presets.
4) Classic themes without theme.json can opt-in with the appearance-tools theme support.
5) No action is required today for the bundled themes.

If we merge 6303, then:
1) Both classic themes and block themes will have shadows enabled by default.
2) The problem with the duplicate slugs would be solved(?)
3) Any themes can opt out or set custom presets via theme.json.
4) Classic themes without theme.json can not set their own presets (this will be fixed in 6.6).
5) Classic themes without theme.json can not opt-out (this will be fixed in 6.6).
6) The shadows must be tested with the classic bundled themes. There should not be conflicts but this needs to be confirmed with testing.

If we merge 6295, then:
1) Both classic themes and block themes will have shadows enabled by default.
2) The problem with the duplicate slugs would be solved(?)
3) Any themes can opt out or set custom presets via theme.json.
4) Classic themes without theme.json can set their own presets.
5) Classic themes without theme.json can opt-out.
6) The shadows must be tested with the classic bundled themes. There should not be conflicts but this needs to be confirmed with testing.

#8 @poena
4 weeks ago

Classic themes only:
With both 6303 and 6295, there 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.

Is there a Gutenberg PR that addresses this and will it be ready in time?

@poena
4 weeks ago

Shadows applied in Twenty Ten

@poena
4 weeks ago

Twenty Eleven with shadows applied, front

@poena
4 weeks ago

Twenty Twelve, front

@poena
4 weeks ago

Twenty Thirteen, front

@poena
4 weeks ago

Twenty Fourteen, front

@poena
4 weeks ago

Twenty Fifteen

@poena
4 weeks ago

Twenty Sixteen, front

@poena
4 weeks ago

Twenty Seventeen, front

@poena
4 weeks ago

Twenty Nineteen, front

@poena
4 weeks ago

Twenty Twenty, front

@poena
4 weeks ago

Twenty Twenty-One, front

@poena
4 weeks ago

Twenty Twenty-Two, front

@poena
4 weeks ago

Twenty Twenty-Three, front

@poena
4 weeks ago

Twenty Twenty-Four, front

#9 @poena
4 weeks ago

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.

Last edited 4 weeks ago by poena (previous) (diff)

This ticket was mentioned in PR #6309 on WordPress/wordpress-develop by @ajlende.


4 weeks ago
#10

Alternative to https://github.com/WordPress/wordpress-develop/pull/6295

This does not enable default shadow presets when the theme does not provide their own shadow presets.

Instead default shadow presets can be explicitly opted into with the default-shadow-presets or appearance-tools theme supports in classic themes.

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

@get_dave commented on PR #6295:


4 weeks ago
#11

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

@swissspidy commented on PR #6295:


4 weeks ago
#12

@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.

#13 @ajlende
4 weeks ago

But let's discuss on the ticket please.

The Github issue predates the trac ticket, it has more context, and I asked for discussion in the GH issue at the top of the trac ticket when I opened it. I think it makes more sense as the place for discussion.

https://github.com/WordPress/gutenberg/issues/59989

Last edited 4 weeks ago by ajlende (previous) (diff)

#14 @swissspidy
3 weeks ago

@ajlende GH issue is fine too, but not a PR.

@swissspidy commented on PR #6295:


3 weeks ago
#15

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

@swissspidy commented on PR #6303:


3 weeks ago
#16

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

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 weeks ago

#18 @swissspidy
3 weeks ago

  • Owner set to swissspidy
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 weeks ago

#20 @swissspidy
3 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 57885:

Editor: disable shadow.defaultPresets for classic themes.

With this change default shadow presets are never shown for classic themes, and classic themes have no options for adding custom ones.
This essentially reverts [57717] and [57827] / [57828], which had unintended consequences.

Props ajlende, oandregal, madhudollu, swissspidy, get_dave, andrewserong, desrosj.
Fixes #60815.

#22 @swissspidy
3 weeks ago

  • Keywords fixed-major commit dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#23 @audrasjb
3 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

Approving this for 6.5 backport 👍

#24 @swissspidy
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57889:

Editor: disable shadow.defaultPresets for classic themes.

With this change default shadow presets are never shown for classic themes, and classic themes have no options for adding custom ones.
This essentially reverts [57717] and [57827] / [57828], which had unintended consequences.

Reviewed by audrasjb.
Merges [57885] to the 6.5 branch.

Props ajlende, oandregal, madhudollu, swissspidy, get_dave, andrewserong, desrosj.
Fixes #60815.

Note: See TracTickets for help on using tickets.