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.

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: Color picker doesn't work correctly with variable colors #46492

Closed
sergiu-radu opened this issue Dec 13, 2022 · 9 comments · Fixed by #47181
Closed

ColorPalette: Color picker doesn't work correctly with variable colors #46492

sergiu-radu opened this issue Dec 13, 2022 · 9 comments · Fixed by #47181
Assignees
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Components /packages/components

Comments

@sergiu-radu
Copy link

Hello everyone 👋

If I set the value of a color in theme.json file to be a CSS custom property - the color picker is not showing the color correctly inside the color picker popover when you select a color from the theme color presets - it returns to #000000.

I am setting the theme palette colors in theme.json file like this because the theme has multiple color palettes and the user could change the palette from customizer.

"color": {
	"palette": [
		{
			"name": "Palette Color 1",
			"slug": "palette-color-1",
			"color": "var(--paletteColor1)"
		},

		{
			"name": "Palette Color 2",
			"slug": "palette-color-2",
			"color": "var(--paletteColor2)"
		}
	]
}

Here is a quick video that better explains the problem - https://d.pr/v/8tSl5E

The question is, if it is not possible to use variable colors inside color picker, could you please add support for this?

Thank you very much for your time!

@ndiego ndiego added Needs Testing Needs further testing to be confirmed. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Dec 13, 2022
@albanyacademy
Copy link

albanyacademy commented Dec 19, 2022

colord usage in https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/color-palette/index.tsx can be modified to check for variables. loose example:


// remove CSS variable formatting
const valueStripped = value ? value.substring( value.indexOf( 'var(' ) + 4, value.lastIndexOf( ')' ) ) : '';


const computed = getComputedStyle( document.body );
const computedColor = computed.getPropertyValue( valueStripped );
const colordColor = colord( computedColor || '#fff' );

this could very well be cleaned up. though isSelected logic would need to change:
https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/color-palette/index.tsx#L48

i'm not sure why value needs to be checked against colord and not just the color prop - but we pretty much use only css variables internally, and have a custom color palette picker to support them.

@t-hamano
Copy link
Contributor

@sergiu-radu
Thanks for the report.
My understanding is that only the 111 or 222 string should be supported for the color, as indicated by the schema in theme.json.

Also, the actual color value pointed to by the CSS variable is output as some kind of CSS file or inline style, so it may be difficult to reference that value from the color picker component.

I would be glad to know what exactly is your idea of CSS variable support, and what kind of solution you would like to see.

@t-hamano t-hamano added the [Package] Components /packages/components label Dec 25, 2022
@andreiglingeanu
Copy link
Contributor

andreiglingeanu commented Dec 26, 2022

@t-hamano yes, that's exactly right!

The actual color value is output as an inline style to the :root selector, but it's not hard at all to retrieve it.
We've done similar fixes for a big majority of block plugins out there, specifically:

If this solution looks okay to you, we can go ahead with an actual PR that would potentially fix this. Otherwise, if you have better ideas, please share and we'll work towards the implementation :)

@t-hamano
Copy link
Contributor

@andreiglingeanu
Thanks for the idea!
But that approach expects the CSS variable to be output inline in the :root. For classic themes (without theme.json), it is possible that the CSS variables are written to a CSS file, which is then enqueued into the editor. In that case, it may be difficult to read that CSS variable.

@t-hamano
Copy link
Contributor

@ciampo @mirka
We are looking for a way to show the value that the CSS variable points to on a custom color picker when a CSS variable is given for the value of a ColorPalette component.

Should we get the value of a CSS variable if it is output inline in :root? If you have a better approach, please advise 🙏

@t-hamano t-hamano removed the Needs Testing Needs further testing to be confirmed. label Dec 28, 2022
@ciampo ciampo changed the title Color picker doesn't work correctly with variable colors ColorPalette: Color picker doesn't work correctly with variable colors Jan 2, 2023
@ciampo
Copy link
Contributor

ciampo commented Jan 2, 2023

Thank you for flagging!

We're aware of the fact that ColorPalette may not play well with color variables (see #41459 and #44904).

By looking at the issue, it looks like it can be split into two sub-issues:

  1. ColorPalette (and ColorPicker) don't know how to handle CSS custom properties, and therefore they fallback to transparent colors and/or #000000
  2. The value of the CSS custom property may not be defined in the component's scope

@mirka and I can look further into addressing point 1 (ie. from the @wordpress/components package's point of view), but as stated before, there may also be a need to somehow define the value of the CSS property so that it can be read by the ColorPalette and ColorPicker components (point 2). On this specific aspect I'm probably not the best person to give advice, since I'm not super familiar with how theme.json's variables should be exposed from the settings. We should also keep in mind that sometimes iframes are involved, which may complicate CSS scope and cascade even further.

@mirka
Copy link
Member

mirka commented Jan 4, 2023

If anyone would like to submit a PR for the first sub-issue, the getComputedStyle().getPropertyValue() approach suggested is also where I would start. Except, I think it would be more straightforward to get the computed style for the .components-color-palette__custom-color element (instead of document.body, i.e. :root), and get the property value for background-color. This way the color value will always match what is being shown in the UI component.

(I hope this behavior is consistent across browsers 🤞)

@t-hamano
Copy link
Contributor

t-hamano commented Jan 5, 2023

I think it would be more straightforward to get the computed style for the .components-color-palette__custom-color element (instead of document.body, i.e. :root), and get the property value for background-color.

I'm trying this approach and I would like to submit a PR as it seems to work as expected 👍

@ciampo
Copy link
Contributor

ciampo commented Jan 12, 2023

I think it would be more straightforward to get the computed style for the .components-color-palette__custom-color element (instead of document.body, i.e. :root), and get the property value for background-color.

I'm trying this approach and I would like to submit a PR as it seems to work as expected 👍

Please do! You can ping @mirka and myself for the review, happy to help as always 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Components /packages/components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants