ColorPalette: Ensure text label contrast checking works with CSS variables #47373
Conversation
t-hamano
changed the title
t-hamano
added
[Type] Bug
|
cc'ing @brookewp as she's been also looking at how to best take transparent values into account when computing accessible contrast with |
There was a problem hiding this comment.
This is testing well for me! I just left a comment about a potential edge case.
Noted re. the transparency issue — as pointed out in the previous comment, it was also noticed by Brooke a few days ago. I highlighted what I believe are the challenges in doing this properly in this comment in the original colord issue — TL;DR: we need to find a way to "merge" 2 or more colors into a solid color to be used for colord accessibility calculations, since when the color is transparent, the color underneath affects the contrast too.
| setNormalizedColorValue( | ||
| normalizeColorValue( value, customColorPaletteRef ) | ||
| ); | ||
| }, [ value ] ); |
There was a problem hiding this comment.
Since this effect runs only when value changes and has an early return when customColorPaletteRef.current, I think that there could be an issue with this scenario:
- component renders with
disableCustomColorsprop set totrue- this means that
CustomColorPickerDropdownis not rendered, which means thatcustomColorPaletteRef.currentisnull - this also means that
setNormalizedColorValuedoesn't get called, because of the early return on the ref check
- this means that
- Later,
disableCustomColorsgets set tofalsecustomColorPaletteRef.currentgets updated to reference the HTML element, but that doesn't cause the component to re-render (that's how a React ref works)
- The
normalizedColorValueat this point could be out of sync, because theuseEffecthook that is in charge of calculating thenormalizedColorValuewon't be called until thevalueprop changes
We could do some manual testing (or unit testing) to check against this behaviour. A couple of approaches to fix this potential issue could be to:
- Add
disableCustomColorsas a dependency of theuseEffecthook (and usedisableCustomColorsin the hook internal check) — this is more of an indirect check, and it's not my favourite option because it's not clear why we'd need to check for it - Store
customColorPaletteRefin the component state, by using a ref callback +useState. This method would ensure that, when the ref updates, the component re-renders — thus forcing theuseEffecthook to run and keepnormalizeColorValuein sync
There was a problem hiding this comment.
Thank you, I have confirmed that the re-rendering will not take place even if disableCustomColors is changed. Below is an example where the text labels remain in an unintended color because normalizedColorValue is not executed when disableCustomColors is switched:
I may not have understood your advice correctly, but in 75f9c62, it appears to have worked correctly by rewriting useEffect with useCallback. Is this implementation appropriate?
There was a problem hiding this comment.
Below is an example
I can't see any example, maybe the attachment didn't upload correctly?
I may not have understood your advice correctly, but in 75f9c62, it appears to have worked correctly by rewriting
useEffectwithuseCallback. Is this implementation appropriate?
Excuses if I wasn't clear enough with my advice.
Your implementation looks good and works well in my tests — but I think we can clean it up further:
- we can change the
normalizeColorValuecolor value to accept directly an element (HTMLElement | null), instead of aref - since we're using a callback ref, there's no need to keep the
customColorPaletteRefvariable
diff --git a/packages/components/src/color-palette/index.tsx b/packages/components/src/color-palette/index.tsx
index 48fca6bc44..abba5855b1 100644
--- a/packages/components/src/color-palette/index.tsx
+++ b/packages/components/src/color-palette/index.tsx
@@ -182,7 +182,6 @@ function UnforwardedColorPalette(
props: WordPressComponentProps< ColorPaletteProps, 'div' >,
forwardedRef: ForwardedRef< any >
) {
- const customColorPaletteRef = useRef< HTMLElement | null >( null );
const {
clearable = true,
colors = [],
@@ -197,13 +196,8 @@ function UnforwardedColorPalette(
const clearColor = useCallback( () => onChange( undefined ), [ onChange ] );
const customColorPaletteCallbackRef = useCallback(
- ( ref: HTMLElement | null ) => {
- if ( ref ) {
- customColorPaletteRef.current = ref;
- setNormalizedColorValue(
- normalizeColorValue( value, customColorPaletteRef )
- );
- }
+ ( node: HTMLElement | null ) => {
+ setNormalizedColorValue( normalizeColorValue( value, node ) );
},
[ value ]
);
diff --git a/packages/components/src/color-palette/utils.ts b/packages/components/src/color-palette/utils.ts
index 0e988e0fb0..38b2722912 100644
--- a/packages/components/src/color-palette/utils.ts
+++ b/packages/components/src/color-palette/utils.ts
@@ -80,19 +80,18 @@ export const isMultiplePaletteArray = (
export const normalizeColorValue = (
value: string | undefined,
- ref: RefObject< HTMLElement > | null
+ element: HTMLElement | null
) => {
const currentValueIsCssVariable = /^var\(/.test( value ?? '' );
- if ( ! currentValueIsCssVariable || ! ref?.current ) {
+ if ( ! currentValueIsCssVariable || element === null ) {
return value;
}
- const { ownerDocument } = ref.current;
+ const { ownerDocument } = element;
const { defaultView } = ownerDocument;
- const computedBackgroundColor = defaultView?.getComputedStyle(
- ref.current
- ).backgroundColor;
+ const computedBackgroundColor =
+ defaultView?.getComputedStyle( element ).backgroundColor;
return computedBackgroundColor
? colord( computedBackgroundColor ).toHex()What do you think?
It would also be great if we added a JSDoc comment to the normalizeColorValue utility function, to specify that currently its purpose is to "transform" a CSS variable used as background color into the color value itself.
I tried the The logic is as follows:
From what I have tested, it seems to be working well, but what about this approach? ee2ef025c761114ad0967eb11b83ecd9.mp4 |
|
It seems to be working well! I think we can refine this approach in a few ways:
What do you think? Curious to hear @mirka 's thoughts too |
|
This is great! I tried the 'alpha blending' formula for #47476 (I quickly put it up as a draft to share) in relation to this issue #42715 which yielded nearly identical results. However, your solution is much cleaner. :)
Since you're discussing how to approach the background color, I wanted to share the above PR/issue as it's been requested to show the checkered background behind semi-transparent color values. I believe we should use grey rather than white for the background, as I think there will be more readability issues with the text over the grey. What do you all think? |
|
Thank you for chiming in, Brooke!
Awesome! The timing of the two of you working on these related issues couldn't have been better :)
Thank you for sharing this and giving more perspective on the requirements for this component!
I'm not 100% sure I understand your suggestion here — I can see 2 ways of interpret it:
The challenge here is that we can't assume that the background would always be white, especially since soon we may expose the ability for consumers of the package to change the background color to any color), and therefore we can't assume in advance what will the resulting "average" color be. I also though about whether we could instead just assume a "darkened" version of the background, but that also wouldn't work well in case we ever switched to a "dark" theme. I think it's probably ok to "ignore" the impact of the checkered color for now — we can take a look at the results and iterate if necessary? |
|
Flaky tests detected in 2945982. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4109786938
|
|
Thanks for the review, @ciampo!
I have responded at 98bf1dc. If you find anything inappropriate, please feel free to commit 🙏
Does this variable |
|
My suggestion is to keep the scope of this PR to the CSs variable issue only, and deal with alphas as a separate issue. This is because I think the alpha issue may require some design changes that should get proper input from the design team. We probably do in fact need to keep the checkered background showing through to signal that the color is semi-transparent. In this case we will always (I think?) run into readability issues at some point in the gradient scale if we put text on top of that checkered pattern. I think it's inevitable that we modify the design so it accommodates this case — for example, placing the text outside of the swatch, or placing the text on a pill-style solid background. The current design really does not take into account things like CSS variables (i.e. longer strings) and transparency. Overcomplicating the algorithm may not be necessary once the design issue is addressed! |
There was a problem hiding this comment.
Great job so far, @t-hamano !
Could you also add some unit tests around CSS variables, so that we can prevent regressions in the future?
The unit tests CI issue should hopefully go away after rebasing on latest trunk.
My suggestion is to keep the scope of this PR to the CSs variable issue only, and deal with alphas as a separate issue.
@mirka makes a good point here — I agree that we should keep this PR focused on the CSS variable issue. We can tackle the transparency problem separately (or even, as Lena points out, solve it at the design level).
| setNormalizedColorValue( | ||
| normalizeColorValue( value, customColorPaletteRef ) | ||
| ); | ||
| }, [ value ] ); |
There was a problem hiding this comment.
Below is an example
I can't see any example, maybe the attachment didn't upload correctly?
I may not have understood your advice correctly, but in 75f9c62, it appears to have worked correctly by rewriting
useEffectwithuseCallback. Is this implementation appropriate?
Excuses if I wasn't clear enough with my advice.
Your implementation looks good and works well in my tests — but I think we can clean it up further:
- we can change the
normalizeColorValuecolor value to accept directly an element (HTMLElement | null), instead of aref - since we're using a callback ref, there's no need to keep the
customColorPaletteRefvariable
diff --git a/packages/components/src/color-palette/index.tsx b/packages/components/src/color-palette/index.tsx
index 48fca6bc44..abba5855b1 100644
--- a/packages/components/src/color-palette/index.tsx
+++ b/packages/components/src/color-palette/index.tsx
@@ -182,7 +182,6 @@ function UnforwardedColorPalette(
props: WordPressComponentProps< ColorPaletteProps, 'div' >,
forwardedRef: ForwardedRef< any >
) {
- const customColorPaletteRef = useRef< HTMLElement | null >( null );
const {
clearable = true,
colors = [],
@@ -197,13 +196,8 @@ function UnforwardedColorPalette(
const clearColor = useCallback( () => onChange( undefined ), [ onChange ] );
const customColorPaletteCallbackRef = useCallback(
- ( ref: HTMLElement | null ) => {
- if ( ref ) {
- customColorPaletteRef.current = ref;
- setNormalizedColorValue(
- normalizeColorValue( value, customColorPaletteRef )
- );
- }
+ ( node: HTMLElement | null ) => {
+ setNormalizedColorValue( normalizeColorValue( value, node ) );
},
[ value ]
);
diff --git a/packages/components/src/color-palette/utils.ts b/packages/components/src/color-palette/utils.ts
index 0e988e0fb0..38b2722912 100644
--- a/packages/components/src/color-palette/utils.ts
+++ b/packages/components/src/color-palette/utils.ts
@@ -80,19 +80,18 @@ export const isMultiplePaletteArray = (
export const normalizeColorValue = (
value: string | undefined,
- ref: RefObject< HTMLElement > | null
+ element: HTMLElement | null
) => {
const currentValueIsCssVariable = /^var\(/.test( value ?? '' );
- if ( ! currentValueIsCssVariable || ! ref?.current ) {
+ if ( ! currentValueIsCssVariable || element === null ) {
return value;
}
- const { ownerDocument } = ref.current;
+ const { ownerDocument } = element;
const { defaultView } = ownerDocument;
- const computedBackgroundColor = defaultView?.getComputedStyle(
- ref.current
- ).backgroundColor;
+ const computedBackgroundColor =
+ defaultView?.getComputedStyle( element ).backgroundColor;
return computedBackgroundColor
? colord( computedBackgroundColor ).toHex()What do you think?
It would also be great if we added a JSDoc comment to the normalizeColorValue utility function, to specify that currently its purpose is to "transform" a CSS variable used as background color into the color value itself.
|
Update:
In my unit tests, I expected the following, but both tests failed.
It appears that the If there is a way to resolve this, I would love to hear about it 🙏 |
|
Mhh, it looks like CSS custom properties are not fully supported in jsdom. In case we can't write tests at the component level, we could at least write unit test for the |
|
I have changed to a unit test for the |
| // @ts-ignore | ||
| defaultView.getComputedStyle = () => ( { | ||
| backgroundColor: '#ff0000', | ||
| } ); |
There was a problem hiding this comment.
Not ideal, of course, but better than nothing. I'm not sure if @mirka has any idea to who we could get getComputedStyle to work our tests (afterall, it seems to work as expected in Theme's tests).
There was a problem hiding this comment.
Or even @jsnajdr , given his recent work on getComputedStyle
There was a problem hiding this comment.
JSDOM has a very limited support for CSS, and it doesn't expand CSS variables to their real values. That means that normalizeValue is not possible to unit test with Jest and JSDOM. I would consider if I want to keep the unit test at all.
A less harsh way to "fake" the CSS value is to set the style on the tested element itself:
element.style = { backgroundColor: '#ff0000' };Then getComputedStyle( element ) will find this style and will return it.
By the way, the normalizeValue function would be better off if its second param was just the element, not a ref:
normalizeColorValue( value: string | undefined, element: HTMLElement | undefined )There was a problem hiding this comment.
Thank you, Jarda!
A less harsh way to "fake" the CSS value is to set the style on the tested element itself:
Good point. @t-hamano , would you mind trying out this approach?
By the way, the normalizeValue function would be better off if its second param was just the element, not a ref:
Agreed! Although this change should have been applied in a previous commit
There was a problem hiding this comment.
Thank you for the advice! Perhaps I was thinking too hard. I think we handled it well with 2945982.
|
A rebase/merge with |
ciampo
added
the
Backport to WP 6.7 Beta/RC
|
Thank you, @ciampo! |
…ables (#47373) * ColorPalette: Ensure text label contrast checking works with CSS variables * use `useEffect` to get normalized color value * Update changelog * Try to detect actual color from rgba * Use function to get the composite background color * Rewrite useEffect with useCallback * Don't consider transparent color * Don't use ref, simplify normalizeColorValue() function * Add JSDoc * Add unit tests * Refactor unit tests * Refactor unit test
|
Cherry-picked this PR to the wp/6.2 branch. |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
Closes: #41459
What?
This PR performs a contrast check for text labels displayed in the custom color palette, taking into account CSS variables, and displays them in the correct color.
Why?
Passing a CSS variable to
contrast()function of thecolordreturns an incorrect contrast value because it cannot interpret the value correctly. As a result, the text labels in the custom color palette will always be white (#fff).How?
As was done in #47181, I used computed colors from the custom color palette for the contrast check.
Issues about transparency
I have discovered that the
contrast()function doesn't take into account the alpha value, as reported in this issue.For example, the following two return the same value:
I think that we need to use a library that converts rgba to rgb, or similar logic, and would like to discuss the appropriate approach.
Testing Instructions
In the block editor
Enable Empty Theme and update theme.json as follows:
/test/emptytheme/theme.json
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 2, "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" }, "color": { "palette": [ { "name": "Hex Red", "slug": "hex-red", "color": "var(--wp--custom--hex-red)" }, { "name": "RGB Red", "slug": "rgb-red", "color": "var(--wp--custom--rgb-red)" }, { "name": "RGBA Red", "slug": "rgba-red", "color": "var(--wp--custom--rgba-red)" }, { "name": "Hex Green", "slug": "hex-green", "color": "var(--wp--custom--hex-green)" }, { "name": "RGB Green", "slug": "rgb-green", "color": "var(--wp--custom--rgb-green)" }, { "name": "RGBA Green", "slug": "rgba-green", "color": "var(--wp--custom--rgba-green)" }, { "name": "Hex Blue", "slug": "hex-blue", "color": "var(--wp--custom--hex-blue)" }, { "name": "RGB Blue", "slug": "rgb-blue", "color": "var(--wp--custom--rgb-blue)" }, { "name": "RGBA Blue", "slug": "rgba-blue", "color": "var(--wp--custom--rgba-blue)" }, { "name": "Hex Yellow", "slug": "hex-yellow", "color": "var(--wp--custom--hex-yellow)" }, { "name": "RGB Yellow", "slug": "rgb-yellow", "color": "var(--wp--custom--rgb-yellow)" }, { "name": "RGBA Yellow", "slug": "rgba-yellow", "color": "var(--wp--custom--rgba-yellow)" } ] }, "custom": { "hex-red": "#E10800", "rgb-red": "rgb(225,8,0)", "rgba-red": "rgba(225,8,0,0.3)", "hex-green": "#3EC300", "rgb-green": "rgb(62,195,0)", "rgba-green": "rgba(62,195,0,0.3)", "hex-blue": "#337CA0", "rgb-blue": "rgb(51,124,160)", "rgba-blue": "rgba(51,124,160,0.3)", "hex-yellow": "#FFFC31", "rgb-yellow": "rgb(255,252,49)", "rgba-yellow": "rgba(255,252,49,0.3)" } } }Note: Palettes with
rgbavalues may result in unintended colors, as mentioned above.0be534e8c412514e7d53af84bb3349bf.mp4
In the storybook
storybook.mp4