Make WordPress Core

Opened 8 months ago

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

Change History (38)

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


8 months 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
8 months 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
8 months 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
8 months 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
8 months 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.


8 months 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
8 months 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
8 months 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
8 months ago

Shadows applied in Twenty Ten

@poena
8 months ago

Twenty Eleven with shadows applied, front

@poena
8 months ago

Twenty Twelve, front

@poena
8 months ago

Twenty Thirteen, front

@poena
8 months ago

Twenty Fourteen, front

@poena
8 months ago

Twenty Fifteen

@poena
8 months ago

Twenty Sixteen, front

@poena
8 months ago

Twenty Seventeen, front

@poena
8 months ago

Twenty Nineteen, front

@poena
8 months ago

Twenty Twenty, front

@poena
8 months ago

Twenty Twenty-One, front

@poena
8 months ago

Twenty Twenty-Two, front

@poena
8 months ago

Twenty Twenty-Three, front

@poena
8 months ago

Twenty Twenty-Four, front

#9 @poena
8 months 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 8 months ago by poena (previous) (diff)

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


8 months 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:


8 months 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:


8 months 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
8 months 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 8 months ago by ajlende (previous) (diff)

#14 @swissspidy
8 months ago

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

@swissspidy commented on PR #6295:


8 months ago
#15

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

@swissspidy commented on PR #6303:


8 months 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.


8 months ago

#18 @swissspidy
8 months ago

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

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


8 months ago

#20 @swissspidy
8 months 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
8 months ago

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

#23 @audrasjb
8 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Approving this for 6.5 backport 👍

#24 @swissspidy
8 months 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.