Allow themes to provide empty values for `color.duotone` and `spacing.units` #33280

oandregal · 2021-07-08T09:39:49Z

This fixes an issue with the color.duotone & spacing.units in which empty values didn't override previous origins, resulting in that a theme couldn't provide an empty set for this via its theme.json.

How to test

Use the TT1-blocks theme.

Test empty units work as expected

Set to empty those presets in its theme.json:

{
  "settings": {
    "color": {
      "duotone": []
    },
    "spacing": {
       "units": []
    }
  }
}

Go to the editor and add an image. Select the duotone control. The expected result is that the duotone control doesn't show any color, but still shows the shadow and highlight options.

Add a cover block, select it and go to its inspector. The cover height and the padding should only show px units.

Test filtered units work as expected

Set to empty those presets in its theme.json:

{
  "settings": {
    "color": {
      "duotone": [
        {
          "name":  "Dark grayscale" ,
          "colors": [ "#000000", "#7f7f7f" ],
          "slug": "dark-grayscale"
          }
      ]
    },
    "spacing": {
       "units": [ "px", "em" ]
    }
  }
}

Go to the editor and add an image. Select the duotone control. The expected result is that the duotone control only has the one we added.
Add a cover block, select it and go to its inspector. The cover height and the padding should only show the two units added by the theme (px, em).

Test no units work as expected

Remove color.duotone and spacing.units from the theme.json of TT1-blocks.
Check that the controls show a list of duotone colors and units in the cases above.

I've also tested passing an empty array to the other presets and this is how it should work: colors and gradients should still work as before: hide the list of preset values, but users still can add custom values. Font sizes should still work as before: hide the control.

oandregal · 2021-07-08T09:46:40Z

I've got curious about how one can prevent the color panel in the inspector and the duotone control from showing. Apparently we can't, and this is an issue that should be discussed in a different PR.

I've tried this in the theme.json:

{
"settings": {
  "color": {
    "link": false,
    "custom": false,
    "duotone": [],
    "palette": [],
    "gradients": []
  }
}

Resulting in (notice how the color panel and the duotone control are still accessible but are empty):

oandregal · 2021-07-08T10:09:52Z

Updated the issue description with data about how this affects the other presets. TLDR: all work well except for units. The issue with units is that UI controls don't respect the empty array passed by the theme and should be fixed in a different PR.

oandregal · 2021-07-08T10:26:47Z

lib/class-wp-theme-json-gutenberg.php

@@ -1073,6 +1073,7 @@ public function merge( $incoming ) {
 		// For leaf values that are arrays it will use the numeric indexes for replacement.
 		// In those cases, we want to replace the existing with the incoming value, if it exists.
 		$to_replace   = array();
+		$to_replace[] = array( 'layout', 'units' );


@youknowriad would like confirmation about how layout.units should work. My understanding is that it should work this way:

if a theme doesn't provide any layout.units defaults are listed

themes can override these values via theme.json

if a theme provides an empty array in layout.units, the control only shows px

if a theme provides a limited set of units, such as [ 'px', 'em' ], those are the only units shown in the control

Is this behavior correct?

If so, we need to add layout.units to the list of things we merge ourselves.

@aristath would know more here but it does sound right.

Though that makes me wonder, is layout.units a thing? Why "units" are in "layout" I feel like that's an error?

isn't it spacing.units ?

Both exist and are used in different places:

layout.units: layout hook, letter-spacing component, border width hook, column width.

spacing.units: padding hook, margin hook, cover height, unit control.

I don't know if this is proper or we should only have one of them.

I think replacing layout.units with spacing.units would be OK... Both implementations are sub-optimal, but until we figure out a proper way to differentiate these things we should probably keep things as they were 👍
If and when the time comes to differentiate them, a separate ticket should be opened and we can investigate the implementation separately. For now, let's replace things with spacing.units and keep it consistent ❤️

spacing.units can be overridden by block, so I wonder if that's enough for now.

Another thing I've noticed is that the UnitControl uses spacing.units for availableUnits: it looks like these are the "allowed units" the control can use unless a consumer filters them via units. This means that what you pass as units can only be a subset of spacing.units: you can't have in layout.units things that are not part of spacing.units as well.

I'm also in favor of starting small and add things if we need them later. Remove things is hard/impossible.

I haven't seen this in use by any theme and doesn't have documentation either, so I presume layout.units wasn't widely known/used in practice.

For example, if you set spacing.units to an empty array, any content in layout.units would've been ignored. I think I'm going to commit this as it is. It's code, so we can always add more, should it be necessary.

The other side effect here is that we shouldn't add any config to layout that is not about the "layout config" itself (content width, wide width, type of layout), because that config has a meaning holistically and it's not a discrete list of settings.

Prepared this #33303 to specify what settings can be part of layout, so we don't allow random things there.

oandregal · 2021-07-08T10:47:09Z

Ok, units seem to work as expected as well, except for the border-radius that doesn't allow filtering the units by the theme, so I've asked https://github.com/WordPress/gutenberg/pull/31585/files#r666080677 This can be done later as it's unrelated and doesn't affect WordPress 5.8.

youknowriad · 2021-07-08T11:05:09Z

"duotone": [],

I think you could say "duotone": false to hide the button but maybe "duotone": [] should work similarly. @ajlende

oandregal · 2021-07-08T11:10:56Z

I think you could say "duotone": false to hide the button but maybe "duotone": [] should work similarly.

That raises an error in the translation code because it's expecting an array. We can update that code to allow booleans so this works, but I'm reluctant because it's very specific to duotone. Any other preset (colors, gradients) don't work that way.

youknowriad · 2021-07-08T11:15:29Z

That raises an error in the translation code because it's expecting an array. We can update that code to allow booleans

Maybe it's null and not false but there's definitely something in the code to support that #32002 maybe it regressed with theme.json translation.

Mamaduka · 2021-07-08T11:34:21Z

We can update ! duotonePalette check to ! duotonePalette?.length and this should handle an empty array.

gutenberg/packages/block-editor/src/components/duotone-control/index.js

Lines 23 to 25 in a6908dc

    
           if ( ! duotonePalette ) { 
        
           	return null; 
        
           }

oandregal · 2021-07-08T11:44:01Z

Maybe it's null and not false but there's definitely something in the code to support that #32002 maybe it regressed with theme.json translation.

No, it didn't. Tested that PR and it also fails with false, I guess that's why the workaround to use null. I've also noticed that setting the palette to null as suggested by that PR raised an error at the time and it gets a different one today :(

Ok, let's move forward: this PR should focus on fixing the ability to filter and pass empty units to color.duotone, spacing.units, layout.units.

In a different PR, we should address how panels are shown/hidden. I'd argue we should not use nulls and do this instead:

duotone is hidden when color.duotone is empty && color.palette is empty && color.custom is false
the color panel is hidden when color.palette is empty && color.gradients is empty && color.custom is false
etc

Essentially, the panels should be hidden when the presets they are empty && the other boolean style properties are false.

jorgefilipecosta · 2021-07-08T11:52:32Z

lib/class-wp-theme-json-gutenberg.php

@@ -1086,8 +1087,8 @@ public function merge( $incoming ) {
 		foreach ( $nodes as $metadata ) {
 			foreach ( $to_replace as $property_path ) {
 				$path = array_merge( $metadata['path'], $property_path );
-				$node = _wp_array_get( $incoming_data, $path, array() );
-				if ( ! empty( $node ) ) {


We already a similar bug before with using empty. I grepped for empty on this file to make sure we don't have additional issues. I found other suspicious case:

if ( ! empty( $escaped_preset ) ) { gutenberg_experimental_set( $output, $preset_metadata['path'], $escaped_preset ); }

It is related to user escaping does not affect 5.8 I will confirm if this is indeed another bug and if yes fix it in another PR.

jorgefilipecosta · 2021-07-08T11:54:56Z

the color panel is hidden when color.palette is empty && color.gradients is empty && color.custom is false

I think for the color panels that's already the case unless we had some regression.

jorgefilipecosta · 2021-07-08T11:59:56Z

That raises an error in the translation code because it's expecting an array. We can update that code to allow booleans

Maybe it's null and not false but there's definitely something in the code to support that #32002 maybe it regressed with theme.json translation.

I think duotone should be consistent with the other presets we can have duotone presets that are always an array, and a customDuotone flag. Duotone is not available at all if the presets are empty and customDuotone flag is false, exactly as it happens on color, grandients, font sizes.

youknowriad · 2021-07-08T12:20:10Z

Ok, let's move forward: this PR should focus on fixing the ability to filter and pass empty units to color.duotone, spacing.units, layout.units.

I'd love @aristath's input but I think layout.units was just a typo and this PR should focus on using spacing.units consistently (this is the documented config anyway)

nevermind @nosolosw just saw that you did already :)

youknowriad · 2021-07-08T12:21:21Z

phpunit/class-wp-theme-json-test.php

@@ -899,10 +884,6 @@ public function test_merge_incoming_data_null_presets() {
 						),
 					),
 				),
-				'layout'     => array(
-					'contentSize' => '610px',


not sure what this test is about but this config is valid (contentSize) and maybe we could keep it.

This commit is removing a previous commit in this PR: I had added this when I thought layout.units was a valid input and wanted to make sure the merge algorithm respected pre-existing data (layout.contentSize). If we want a test for this it should live in another method of this file. Can be done in a follow-up PR if necessary.

youknowriad

LGTM 👍

aristath · 2021-07-08T12:25:20Z

I'd love @aristath's input but I think layout.units was just a typo and this PR should focus on using spacing.units consistently (this is the documented config anyway)

Thank you for the ping! Added comment on #33280 (comment) 👍

oandregal · 2021-07-08T12:30:00Z

This is ready in my testing as well. I'm waiting for the conversation at #33280 (comment) to settle before merging this.

oandregal · 2021-07-08T13:49:13Z

WordPress/wordpress-develop#1489 ports the PHP changes to core. The JS changes are related but don't need to land at the same time: I presume they are going to be part of the next package release for core?

oandregal · 2021-07-08T13:51:38Z

The JS changes are related but don't need to land at the same time: I presume they are going to be part of the next package release for core?

Or perhaps is best if I push the JS changes to the wp/5.8 branch?

youknowriad · 2021-07-08T14:05:01Z

Yeah, next package update but we can include WordPress/wordpress-develop#1489 in the same commit if needed.

oandregal · 2021-07-08T15:53:02Z

#33295 updates the conditions under which the duotone control and its subcomponents are visible.

oandregal · 2021-07-08T17:59:55Z

Follow-up from #33280 (comment) I could reproduce the color panel issue: it seems that it requires all things to be disabled/empty, even if the block itself supports only some of them: #33304

….units` and remove `layout.units` (#33280)

oandregal added 3 commits July 8, 2021 11:27

Add tests for empty incoming presets

22b3bd3

Add tests for null incoming presets

456a7be

Empty arrays are valid input data that need to be merged as well

f62c32a

oandregal requested a review from spacedmonkey as a code owner July 8, 2021 09:39

oandregal requested review from youknowriad and jorgefilipecosta July 8, 2021 09:39

oandregal self-assigned this Jul 8, 2021

oandregal requested a review from ajlende July 8, 2021 09:41

oandregal changed the title ~~Incoming presets in theme.json can be empty~~ Duotone colors should not be present if the theme provides an empty set via theme.json Jul 8, 2021

oandregal added 2 commits July 8, 2021 12:19

Layout.units were not being considered for merging

05fc7b2

Add tests for layout.units

fd1fc57

oandregal commented Jul 8, 2021

View reviewed changes

jorgefilipecosta reviewed Jul 8, 2021

View reviewed changes

Substitute layout.units per spacing.units

2d19440

oandregal requested review from ajitbohra and ellatrix as code owners July 8, 2021 12:07

youknowriad reviewed Jul 8, 2021

View reviewed changes

youknowriad approved these changes Jul 8, 2021

View reviewed changes

oandregal changed the title ~~Duotone colors should not be present if the theme provides an empty set via theme.json~~ Allow themes to provide empty values for color.duotone and spacing.units Jul 8, 2021

aristath approved these changes Jul 8, 2021

View reviewed changes

oandregal merged commit 8d5a38a into trunk Jul 8, 2021

oandregal deleted the fix/incoming-presets-can-be-empty branch July 8, 2021 13:17

github-actions bot added this to the Gutenberg 11.1 milestone Jul 8, 2021

oandregal mentioned this pull request Jul 8, 2021

Fix for theme.json: color.duotone and spacing.units should allow empty sets WordPress/wordpress-develop#1489

Closed

oandregal mentioned this pull request Jul 8, 2021

Update conditions to hide duotone panel #33295

Merged

This was referenced Jul 8, 2021

Add option to disable duotone #32002

Merged

Option to disable duotone #31764

Closed

Specify what settings can be part of settings.layout #33303

Merged

Color panel should be hidden when the block's color are disabled #33304

Closed

oandregal mentioned this pull request Jul 9, 2021

Border Support: Add support for custom border units #33315

Merged

7 tasks

youknowriad pushed a commit that referenced this pull request Jul 12, 2021

Allow themes to provide empty values for color.duotone and `spacing…

d6cbaca

….units` and remove `layout.units` (#33280)

youknowriad mentioned this pull request Jul 12, 2021

Fixes targeted for WordPress 5.8 RC3 #33354

Merged

youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 12, 2021

youknowriad pushed a commit that referenced this pull request Jul 13, 2021

Allow themes to provide empty values for color.duotone and `spacing…

a042157

….units` and remove `layout.units` (#33280)

sarayourfriend pushed a commit that referenced this pull request Jul 15, 2021

Allow themes to provide empty values for color.duotone and `spacing…

52d1dda

….units` and remove `layout.units` (#33280)

priethor mentioned this pull request Jul 19, 2021

Automate grouping of release Changelog PRs by feature #33229

Merged

12 tasks

carolinan mentioned this pull request Sep 21, 2021

Site Editor breaks when no color, gradient or font-size is configured #31986

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow themes to provide empty values for `color.duotone` and `spacing.units` #33280

Allow themes to provide empty values for `color.duotone` and `spacing.units` #33280

oandregal commented Jul 8, 2021 •

edited

Loading

oandregal commented Jul 8, 2021

oandregal commented Jul 8, 2021

oandregal Jul 8, 2021

youknowriad Jul 8, 2021

youknowriad Jul 8, 2021 •

edited

Loading

youknowriad Jul 8, 2021

oandregal Jul 8, 2021 •

edited

Loading

aristath Jul 8, 2021

oandregal Jul 8, 2021

oandregal Jul 8, 2021

oandregal Jul 8, 2021

oandregal Jul 8, 2021

oandregal commented Jul 8, 2021

youknowriad commented Jul 8, 2021 •

edited

Loading

oandregal commented Jul 8, 2021

youknowriad commented Jul 8, 2021 •

edited

Loading

Mamaduka commented Jul 8, 2021

oandregal commented Jul 8, 2021

jorgefilipecosta Jul 8, 2021

jorgefilipecosta commented Jul 8, 2021

jorgefilipecosta commented Jul 8, 2021

youknowriad commented Jul 8, 2021 •

edited

Loading

youknowriad Jul 8, 2021

oandregal Jul 8, 2021

youknowriad left a comment

aristath commented Jul 8, 2021 •

edited

Loading

oandregal commented Jul 8, 2021

oandregal commented Jul 8, 2021

oandregal commented Jul 8, 2021

youknowriad commented Jul 8, 2021

oandregal commented Jul 8, 2021 •

edited

Loading

oandregal commented Jul 8, 2021

Allow themes to provide empty values for color.duotone and spacing.units #33280

Allow themes to provide empty values for color.duotone and spacing.units #33280

Conversation

oandregal commented Jul 8, 2021 • edited Loading

How to test

Test empty units work as expected

Test filtered units work as expected

Test no units work as expected

oandregal commented Jul 8, 2021

oandregal commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal commented Jul 8, 2021

youknowriad commented Jul 8, 2021 • edited Loading

oandregal commented Jul 8, 2021

youknowriad commented Jul 8, 2021 • edited Loading

Mamaduka commented Jul 8, 2021

oandregal commented Jul 8, 2021

Choose a reason for hiding this comment

jorgefilipecosta commented Jul 8, 2021

jorgefilipecosta commented Jul 8, 2021

youknowriad commented Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

aristath commented Jul 8, 2021 • edited Loading

oandregal commented Jul 8, 2021

oandregal commented Jul 8, 2021

oandregal commented Jul 8, 2021

youknowriad commented Jul 8, 2021

oandregal commented Jul 8, 2021 • edited Loading

oandregal commented Jul 8, 2021

Allow themes to provide empty values for `color.duotone` and `spacing.units` #33280

Allow themes to provide empty values for `color.duotone` and `spacing.units` #33280

oandregal commented Jul 8, 2021 •

edited

Loading

youknowriad Jul 8, 2021 •

edited

Loading

oandregal Jul 8, 2021 •

edited

Loading

youknowriad commented Jul 8, 2021 •

edited

Loading

youknowriad commented Jul 8, 2021 •

edited

Loading

youknowriad commented Jul 8, 2021 •

edited

Loading

aristath commented Jul 8, 2021 •

edited

Loading

oandregal commented Jul 8, 2021 •

edited

Loading