`ColorPalette`: partial support of `color-mix()` CSS colors #64224

Rishit30G · 2024-08-03T14:56:59Z

Why?

Users were facing issue with ColorGradientsControl showing incorrect labels using color-mix

What ?

Related to #64195

How?

Solved this issue by adding const currentValueIsColorMix = /^color-mix$.*$$/.test( currentValue ); to check for color-mix and using it logically in normalizedCurrentValue instead of converting color-mix value to hex which was giving the same label as only the transparency was changing but the color was same i.e #000000

Testing Instructions

Copy the color palette snippet in this issue to the theme.json.
Go to block editor (e.g. edit a page)
Select a block and try to change its color.
Try switching between the new colors and the selected label will always show 10% of Current Text.

Screenshots or screencast

Screen.Recording.2024-08-03.at.10.46.07.AM.mov

github-actions · 2024-08-03T14:57:14Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Rishit30G <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-08-03T14:57:19Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Rishit30G! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

ciampo

Thank you for working on this!

Unfortunately, as explained in an inline comment, we're currently unable to fully support color-mix because of lack of support in parsing newer CSS color syntaxes from an internal dependency.

Having said that, we may also argue that this PR still improves the component with respect to what's on trunk, and therefore we may want to still merge such improvements.

@WordPress/gutenberg-components any thoughts?

If we were to continue work on this PR, it would be great also to add unit tests.

ciampo · 2024-08-15T10:50:49Z

packages/components/src/color-palette/utils.ts

-	const normalizedCurrentValue = currentValueIsCssVariable
-		? currentValue
-		: colord( currentValue ).toHex();
+	const currentValueIsColorMix = /^color-mix\(.*\)$/.test( currentValue );


I'd apply a less strict regex here for better matching:

Suggested change

const currentValueIsColorMix = /^color-mix$.*$$/.test( currentValue );

const currentValueIsColorMix = /color-mix\(/.test( currentValue );

And potentially consider removing the "start of line" caret (^) from currentValueIsCssVariable too

ciampo · 2024-08-15T11:12:06Z

packages/components/src/color-palette/utils.ts

I'm not sure that we can fully support color-mix CSS colors at the moment.

I took a look at the component, and noticed that this PR is missing changes in the normalizeColorValue function at the bottom of this file. The normalizeColorValue function takes care of converting the chosen color option to a CSS color that is passed to the ColorPicker.

I applied the same logic in that function too:

diff --git a/packages/components/src/color-palette/utils.ts b/packages/components/src/color-palette/utils.ts index 00438db528..43104d2084 100644 --- a/packages/components/src/color-palette/utils.ts +++ b/packages/components/src/color-palette/utils.ts @@ -82,9 +82,12 @@ export const normalizeColorValue = ( value: string | undefined, element: HTMLElement | null ) => { - const currentValueIsCssVariable = /^var\(/.test( value ?? '' ); + const valueIsCssVariable = /var\(/.test( value ?? '' ); + const valueIsColorMix = /color-mix\(/.test( value ?? '' ); - if ( ! currentValueIsCssVariable || element === null ) { + const valueIsSimpleColor = ! valueIsCssVariable && ! valueIsColorMix; + + if ( valueIsSimpleColor || element === null ) { return value; }

But the computed styles that the browser returns from applying a color-mix() color to an element use the CSS color() function, which is not a valid input to colord (the library that we use to manipulate and normalize color strings).

And therefore, when opening the ColorPicker, the color initially chosen in the picker is not the correct color, but black (which is the default color that colord outputs when the string that it parses is not valid).

Kapture.2024-08-15.at.13.11.48.mp4

But the computed styles that the browser returns from applying a color-mix() color to an element use the CSS color() function, which is not a valid input to colord (the library that we use to manipulate and normalize color strings).

What are the repro steps for this by the way? I'm just trying it in the browser and not on this PR branch, but when I try it I get a string for the respective color space. So if I getComputedStyle() on an element with background-color: color-mix(in oklab, #a71e14 75%, white) for example, I will get 'oklab(0.603981 0.113291 0.0640898)'.

If we can do that, we could possibly add support for it if it's a color space that has a colord plugin. Maybe not in this PR though.

And yes I'm all for merging changes as long as it's incrementally better than trunk 👍

What are the repro steps for this by the way?

For example, in latest chrome, if I apply background-color: color-mix(in srgb, indigo, white) to an element and then query its computed background-color style, I get color(srgb 0.647059 0.5 0.754902).

If we can do that, we could possibly add support for it if it's a color space that has a colord plugin. Maybe not in this PR though.

I had looked into it, and didn't spot any plugin for the color() syntax — and not even for newer color spaces like oklab and oklch

I guess it's up to the browser to decide the format of the computed style string, and since color-mix can target any color space, to support fully color-mix we'd need to be able to virtually support any color syntax.

Also, looking at colord repo, I think the project is not being maintained anymore 😱 Given that CSS continues to evolve, I think that the colord-based approach is showing its limitations. We should ideally find a browser-based way (either through CSS or native JS) that will guarantee us that whatever CSS color was passed to the component, we're able to parse it, show it, and render a working color picker.

And yes I'm all for merging changes as long as it's incrementally better than trunk 👍

👌

We should ideally find a browser-based way (either through CSS or native JS) that will guarantee us that whatever CSS color was passed to the component, we're able to parse it, show it, and render a working color picker.

Actually, hold up my canvas: https://codepen.io/ciampo/pen/QWXamNe

This approach could have potential. We'd need to make sure it can't be somehow blocked by the browser, and that there are no severe performance implications (if needed, we can explore using OffscreenCanvas in a separate worker thread).

ciampo · 2024-08-16T09:55:49Z

Hey @Rishit30G , @mirka and I think that the improvements in this PR are still worth pursuing, despite recognising that we still won't be able to fully support color-mix() syntaxes.

Here's a list of next steps for this PR:

Apply suggestions from ColorPalette: partial support of color-mix() CSS colors #64224 (comment)
Apply similar changes to the normalizeColorValue (see suggested diff in ColorPalette: partial support of color-mix() CSS colors #64224 (comment))
Consider extracting a reusing a isSimpleCSSColor utility that checks for CSS variable and color-mix()
Add unit tests if possible

Does it sound good ?

ciampo

Thank you!

I left some extra feedback on the current changes.

For the unit tests, we could add the color-mix tests to this file. Let me know if you need help in writing those tests.

packages/components/src/color-palette/utils.ts

ciampo

I actually looked into writing some unit tests, but it looks like they would be a bit pointless since JSDom has very poor support for CSS custom properties and CSS color syntax — so our best bet would be to use a real browser environment to test this component, and that's not in the scope of this PR.

@Rishit30G , the last thing left before merging would be to add a CHANGELOG entry in this file. I would probably add it under the Enhancements category.

Thank you!

… CSS colors

packages/components/CHANGELOG.md

ciampo

LGTM 🚀

…s#64224) * Fix incorrect dropdown label with selected color value * Fix normalizeColorValue function * Add isSimpleCSSColor function * Fix isSimpleCSSColor function, add early return * Update CHANGELOG.md with ColorPalette: partial support of color-mix() CSS colors * Update packages/components/CHANGELOG.md --------- Co-authored-by: Rishit30G <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: mirka <[email protected]> Co-authored-by: tyxla <[email protected]>

ramonjd · 2024-09-02T05:18:18Z

packages/components/src/color-palette/utils.ts

@@ -79,9 +92,9 @@ export const normalizeColorValue = (
 	value: string | undefined,
 	element: HTMLElement | null
 ) => {
-	const currentValueIsCssVariable = /^var\(/.test( value ?? '' );
+	const valueIsSimpleColor = value ? isSimpleCSSColor( value ) : false;


I believe this change has created a regression.

It's treating undefined values as false positives = non-simple values, which means it then returns defaultView?.getComputedStyle

With this changes

The default alpha value is 0, which means, by default, any custom color cannot be seen in the canvas.

Kapture.2024-09-02.at.14.46.23.mp4

With these changes reverted

The alpha is 100 so, by default, my custom color changes are visible on the canvas.

Kapture.2024-09-02.at.14.44.36.mp4

I have a fix here:

ColorPalette utils: do not normalize undefined color values #64969

Would be good to confirm that it doesn't affect what this PR intends to do. Thanks!

ciampo · 2024-10-11T14:44:24Z

@fabiankaegy I don't think this PR deserves a Dev Note, since it's mostly a best-effort bug fix that prevents the component from crashing. The color-mix syntax is not really supported, and as such I don't think we should advertise this to WordPress developers.

I removed it from the list int he 6.7 dev notes tracking issue.

Fix incorrect dropdown label with selected color value

250b9c7

Rishit30G requested a review from ajitbohra as a code owner August 3, 2024 14:56

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Aug 3, 2024

akasunil added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Aug 10, 2024

tyxla requested a review from a team August 12, 2024 18:51

ciampo assigned Rishit30G Aug 15, 2024

ciampo reviewed Aug 15, 2024

View reviewed changes

ciampo changed the title ~~Fix: ColorGradientsControl dropdown shows incorrect label when the selected color value is using color-mix CSS function~~ ColorPalette: partial support of color-mix() CSS colors Aug 15, 2024

Rishit30G added 2 commits August 18, 2024 19:07

Fix normalizeColorValue function

a7e789b

Add isSimpleCSSColor function

055a52e

ciampo reviewed Aug 19, 2024

View reviewed changes

packages/components/src/color-palette/utils.ts Outdated Show resolved Hide resolved

packages/components/src/color-palette/utils.ts Outdated Show resolved Hide resolved

tyxla reviewed Aug 19, 2024

View reviewed changes

packages/components/src/color-palette/utils.ts Outdated Show resolved Hide resolved

packages/components/src/color-palette/utils.ts Outdated Show resolved Hide resolved

Fix isSimpleCSSColor function, add early return

cb1bfef

ciampo reviewed Aug 19, 2024

View reviewed changes

Update CHANGELOG.md with ColorPalette: partial support of color-mix()…

9275984

… CSS colors

ciampo reviewed Aug 20, 2024

View reviewed changes

packages/components/CHANGELOG.md Outdated Show resolved Hide resolved

Update packages/components/CHANGELOG.md

e46f9a8

ciampo enabled auto-merge (squash) August 20, 2024 08:26

ciampo approved these changes Aug 20, 2024

View reviewed changes

ciampo merged commit 3ed1812 into WordPress:trunk Aug 20, 2024
59 checks passed

github-actions bot added this to the Gutenberg 19.1 milestone Aug 20, 2024

This was referenced Aug 20, 2024

ColorGradientsControl dropdown shows incorrect label when the selected color value is using color-mix CSS function #64195

Closed

Color components: support color-mix() and other color syntaxes #64638

Open

ramonjd reviewed Sep 2, 2024

View reviewed changes

ramonjd mentioned this pull request Sep 2, 2024

ColorPalette utils: do not normalize undefined color values #64969

Merged

fabiankaegy mentioned this pull request Oct 1, 2024

6.7 Dev Note Tracking Issue #65784

Closed

97 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ColorPalette`: partial support of `color-mix()` CSS colors #64224

`ColorPalette`: partial support of `color-mix()` CSS colors #64224

Rishit30G commented Aug 3, 2024 •

edited by ciampo

Loading

github-actions bot commented Aug 3, 2024 •

edited

Loading

github-actions bot commented Aug 3, 2024

ciampo left a comment •

edited

Loading

ciampo Aug 15, 2024

ciampo Aug 15, 2024

ciampo Aug 15, 2024

mirka Aug 15, 2024

ciampo Aug 16, 2024

ciampo Aug 16, 2024 •

edited

Loading

ciampo commented Aug 16, 2024

ciampo left a comment

ciampo left a comment

ciampo left a comment

ramonjd Sep 2, 2024 •

edited

Loading

ramonjd Sep 2, 2024

ciampo commented Oct 11, 2024 •

edited

Loading

	const currentValueIsColorMix = /^color-mix\(.*\)$/.test( currentValue );
	const currentValueIsColorMix = /color-mix\(/.test( currentValue );

ColorPalette: partial support of color-mix() CSS colors #64224

ColorPalette: partial support of color-mix() CSS colors #64224

Conversation

Rishit30G commented Aug 3, 2024 • edited by ciampo Loading

Why?

What ?

How?

Testing Instructions

Screenshots or screencast

github-actions bot commented Aug 3, 2024 • edited Loading

github-actions bot commented Aug 3, 2024

ciampo left a comment • edited Loading

Choose a reason for hiding this comment

ciampo Aug 15, 2024

Choose a reason for hiding this comment

ciampo Aug 15, 2024

Choose a reason for hiding this comment

ciampo Aug 15, 2024

Choose a reason for hiding this comment

mirka Aug 15, 2024

Choose a reason for hiding this comment

ciampo Aug 16, 2024

Choose a reason for hiding this comment

ciampo Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

ciampo commented Aug 16, 2024

ciampo left a comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

ramonjd Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

With this changes

With these changes reverted

ramonjd Sep 2, 2024

Choose a reason for hiding this comment

ciampo commented Oct 11, 2024 • edited Loading

`ColorPalette`: partial support of `color-mix()` CSS colors #64224

`ColorPalette`: partial support of `color-mix()` CSS colors #64224

Rishit30G commented Aug 3, 2024 •

edited by ciampo

Loading

github-actions bot commented Aug 3, 2024 •

edited

Loading

ciampo left a comment •

edited

Loading

ciampo Aug 16, 2024 •

edited

Loading

ramonjd Sep 2, 2024 •

edited

Loading

ciampo commented Oct 11, 2024 •

edited

Loading