ColorPalette: make sure "key" is unique when iterating over color entries with same value #43096
Conversation
ciampo
added
[Type] Bug
|
Size Change: +9 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
Hey @marceloaof, what you're flagging is definitely related to the same issue that this PR is trying to mitigate, and it actually highlighted another edge case that I didn't consider (duplicated colors across separate palettes)
The solution currently proposed in this PR currently doesn't fix this scenario, which makes think that we should be probably looking at a more systematic solution. Here's some reflections:
Personally, I believe that the "safest" way to handle this issue would be to add a unique Cc'ing also few folks who may help with this (@mtias @jorgefilipecosta @ntsekouras @andrewserong @aaronrobertshaw @jasmussen ) |
|
Great ticket. I actually ran into this issue a while back, but ended up not ticketing since my specific use case is a bit of a lazy hack. This is a much older GIF, but it shows the issue: Here's what's going on:
Like so: Yes, it's a bit of a hack to transform the colors like that. But the net result is that I can selectively apply colors on my site that I know will invert in dark mode, as I traverse the site editor. While a hack, it does illustrate one opportunity for theme developers to be creative. For that reason, I would personally compare active colors only on their slug, and not their value. What do you think? |
Thank you for sharing additional perspective — your point of view makes a lot of sense!
If by That's why I was suggesting that we consider adding a new unique If we follow your suggestion and rely on color's |
|
Ah, I meant that "Interface mode black" gives me |
|
When I refer to "color entries" having a You can see it in action by visiting the Storybook example and clicking on the "Docs" tab in the top of the screen. There, you should see code snippet for different examples where the list of colors is an array of objects containing |
There was a problem hiding this comment.
I agree that this behavior is probably the lesser of all evils, in that the UI is honest about its current limitations. It's a bit confusing when you click Blue2 and Blue1 gets selected, but ultimately that is going to be the color slug you see when you reload the page.
So until we get backend changes that allow more than a raw color value to be persisted, I think this is a compromise we have to live with. (And the backend part should be discussed as a separate issue.)
| @@ -2,7 +2,6 @@ | |||
| /** | |||
| * External dependencies | |||
| */ | |||
| import { map } from 'lodash'; | |||
There was a problem hiding this comment.
To avoid two instances of the same color appearing as selected, I've arbitrarily decided that only the first matching color will appear as selected. This is not ideal, but IMO it's better than the current behavior.
I just realized we can't do this because it doesn't make sense when you're using a screen reader. When you select the second instance of the color, it sounds like the UI is unresponsive. Maybe we should keep this PR to the key fix only?
There was a problem hiding this comment.
Thanks for getting the discussion going on solutions to this issue @ciampo 👍
@mirka makes a great point regarding the accessibility problem with the PR's current approach. +1 to keeping this PR limited to the key fix. Tackling the bigger problem should probably be explored alongside what changes might need to be made to the editors, block supports, theme.json, and global styles.
(note that currently we're not keeping track of the name associated to the
colorthat was clicked by the user)
I believe our editors actually attempt to map the color value back to a preset and store the slug in block attributes etc. For example, textColor or backgroundColor. This slug (name) is used to apply appropriate classnames, CSS var strings in the form of var:preset|color|${ slug }, and create the actual CSS vars themselves. Given that, duplicate slugs or names are likely also a problem in their own right.
Any solution to uniquely identifying color presets in the ColorPalette component will, in all likelihood, be required by our editors.
@andrewserong and @ramonjd will have more insight into how potentially storing additional color data might impact, or be impacted by, the style engine.
Thanks for the ping! It depends on whether any additional data appears in a block's attributes and that data has any relationship to the block's actual text/background/gradient value. For presets, the style engine does expect a value format of Is there anywhere suggesting that block attribute values such as For all other purposes, the style engine mainly cares about the block attribute's If there are multiple properties with the same value we'd dedupe them anyway. Not sure if that even helps, but 🍺 |
+1 thanks for the discussion here, it's a pretty nuanced issue! The only other thing I'd add is to this comment:
I'm probably just echoing what's already been discussed, but I think ideally these wouldn't be duplicates, so that someone selecting "Foreground" in a theme color can then switch to a different theme variation where that color is then swapped out for a different color value, versus selecting a color that shouldn't (or probably won't) change on a theme switch, like the named "Black" from the default palette. TwentyTwentyTwo's variations are probably a good example as "Foreground" is set to black in the "Default" variation but takes on a different color in each of the others. |
|
Thank you all for the replies! I'll go ahead and make this PR only focused on solving the I will also open a new issue where the matter with duplicated color entries can be discussed separately |
ciampo
force-pushed
the
fix/color-palette-resilient-against-duplicate-colors
branch
from
7f94b11
to
ccb9bf9
Compare
ciampo
force-pushed
the
fix/color-palette-resilient-against-duplicate-colors
branch
from
ccb9bf9
to
7ff5559
Compare
ciampo
changed the title
Opened #43197 |
ciampo
deleted the
fix/color-palette-resilient-against-duplicate-colors
branch
What?
While testing a feature in the editor, I noticed that there was an error in the console while opening my personal site's color palette — I realised that the issue was related to the fact that my specific color palette for that specific site has a duplicated color (ie. two separate entries with the same
valueandname).When that happen, it causes 2 issues:
keyattributesvalueWhy?
This is an edge case that was never considered before.
How?
keyissue has been solved by using the arrayindexto compute thekeyAn alternative idea that came to mind is to filter the list of
colorsand remove duplicates, although that also poses a few challenges:valuebut a differencename? (note that currently we're not keeping track of the name associated to thecolorthat was clicked by the user)Testing Instructions
I've added a new Storybook example to showcase the situation described above — you can test the example with and without the changes introduced in this PR to confirm that the fix is working as expected.