Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Aug 20, 2024
Merged

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

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
250b9c7
Fix incorrect dropdown label with selected color value
Rishit30G Aug 3, 2024
a7e789b
Fix normalizeColorValue function
Rishit30G Aug 18, 2024
055a52e
Add isSimpleCSSColor function
Rishit30G Aug 18, 2024
cb1bfef
Fix isSimpleCSSColor function, add early return
Rishit30G Aug 19, 2024
9275984
Update CHANGELOG.md with ColorPalette: partial support of color-mix()…
Rishit30G Aug 19, 2024
e46f9a8
Update packages/components/CHANGELOG.md
ciampo Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions packages/components/src/color-palette/utils.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

👌

Copy link
Contributor

@ciampo ciampo Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2024-08-16 at 15 29 50 Screenshot 2024-08-16 at 15 27 28

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

Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ export const extractColorNameFromCurrentValue = (
}

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

// Normalize format of `colors` to simplify the following loop
type normalizedPaletteObject = { colors: ColorObject[] };
Expand All @@ -38,9 +40,10 @@ export const extractColorNameFromCurrentValue = (
: [ { colors: colors as ColorObject[] } ];
for ( const { colors: paletteColors } of colorPalettes ) {
for ( const { name: colorName, color: colorValue } of paletteColors ) {
const normalizedColorValue = currentValueIsCssVariable
? colorValue
: colord( colorValue ).toHex();
const normalizedColorValue =
currentValueIsCssVariable || currentValueIsColorMix
? colorValue
: colord( colorValue ).toHex();
ciampo marked this conversation as resolved.
Show resolved Hide resolved

if ( normalizedCurrentValue === normalizedColorValue ) {
return colorName;
Expand Down Expand Up @@ -68,6 +71,18 @@ export const isMultiplePaletteArray = (
);
};

/**
* Checks if a color value is a simple CSS color.
*
* @param value The color value to check.
* @return A boolean indicating whether the color value is a simple CSS color.
*/
const isSimpleCSSColor = ( value: string | undefined ): boolean => {
Rishit30G marked this conversation as resolved.
Show resolved Hide resolved
const valueIsCssVariable = /var\(/.test( value ?? '' );
const valueIsColorMix = /color-mix\(/.test( value ?? '' );
return ! valueIsCssVariable && ! valueIsColorMix;
Rishit30G marked this conversation as resolved.
Show resolved Hide resolved
};

/**
* Transform a CSS variable used as background color into the color value itself.
*
Expand All @@ -79,9 +94,9 @@ export const normalizeColorValue = (
value: string | undefined,
element: HTMLElement | null
) => {
const currentValueIsCssVariable = /^var\(/.test( value ?? '' );
const valueIsSimpleColor = isSimpleCSSColor( value );

if ( ! currentValueIsCssVariable || element === null ) {
if ( valueIsSimpleColor || element === null ) {
ciampo marked this conversation as resolved.
Show resolved Hide resolved
return value;
}

Expand Down
Loading