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: Ensure text label contrast checking works with CSS variables #47373

Merged
merged 16 commits into from
Feb 7, 2023
Merged

ColorPalette: Ensure text label contrast checking works with CSS variables #47373

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0c5c13b
ColorPalette: Ensure text label contrast checking works with CSS vari…
t-hamano Jan 24, 2023
c4cd604
use `useEffect` to get normalized color value
t-hamano Jan 24, 2023
ebae2d7
Update changelog
t-hamano Jan 24, 2023
9327bc9
Try to detect actual color from rgba
t-hamano Jan 26, 2023
2eec7a6
Resolve conflict
t-hamano Jan 28, 2023
98bf1dc
Use function to get the composite background color
t-hamano Jan 28, 2023
75f9c62
Rewrite useEffect with useCallback
t-hamano Jan 29, 2023
0489602
Merge branch 'trunk' into fix/colorpalette-contrast-check
t-hamano Feb 4, 2023
30ae6bc
Don't consider transparent color
t-hamano Feb 4, 2023
011e099
Don't use ref, simplify normalizeColorValue() function
t-hamano Feb 4, 2023
dc62b0f
Add JSDoc
t-hamano Feb 4, 2023
2b845f7
Add unit tests
t-hamano Feb 4, 2023
b020ea2
Fix conflicts
t-hamano Feb 6, 2023
e225bf5
Refactor unit tests
t-hamano Feb 6, 2023
2945982
Refactor unit test
t-hamano Feb 7, 2023
969bf03
Merge branch 'trunk' into fix/colorpalette-contrast-check
t-hamano Feb 7, 2023
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- `Dropdown`: deprecate `position` prop, use `popoverProps` instead ([46865](https://github.com/WordPress/gutenberg/pull/46865)).
- `Button`: improve padding for buttons with icon and text. ([46764](https://github.com/WordPress/gutenberg/pull/46764)).
- `ColorPalette`: Use computed color when css variable is passed to `ColorPicker` ([47181](https://github.com/WordPress/gutenberg/pull/47181)).
- `ColorPalette`: ensure text label contrast checking works with CSS variables ([#47373](https://github.com/WordPress/gutenberg/pull/47373)).

### Internal

Expand Down
23 changes: 20 additions & 3 deletions packages/components/src/color-palette/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,14 @@ import a11yPlugin from 'colord/plugins/a11y';
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { useCallback, useRef, useMemo, forwardRef } from '@wordpress/element';
import {
useCallback,
useEffect,
useRef,
useMemo,
useState,
forwardRef,
} from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -250,8 +257,18 @@ function UnforwardedColorPalette(
__experimentalIsRenderedInSidebar = false,
...otherProps
} = props;
const [ normalizedColorValue, setNormalizedColorValue ] = useState( value );
const clearColor = useCallback( () => onChange( undefined ), [ onChange ] );

useEffect( () => {
if ( ! customColorPaletteRef.current ) {
return;
}
setNormalizedColorValue(
normalizeColorValue( value, customColorPaletteRef )
);
}, [ value ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. component renders with disableCustomColors prop set to true
    • this means that CustomColorPickerDropdown is not rendered, which means that customColorPaletteRef.current is null
    • this also means that setNormalizedColorValue doesn't get called, because of the early return on the ref check
  2. Later, disableCustomColors gets set to false
    • customColorPaletteRef.current gets updated to reference the HTML element, but that doesn't cause the component to re-render (that's how a React ref works)
  3. The normalizedColorValue at this point could be out of sync, because the useEffect hook that is in charge of calculating the normalizedColorValue won't be called until the value prop 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:

  1. Add disableCustomColors as a dependency of the useEffect hook (and use disableCustomColors in 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
  2. Store customColorPaletteRef in the component state, by using a ref callback + useState. This method would ensure that, when the ref updates, the component re-renders — thus forcing the useEffect hook to run and keep normalizeColorValue in sync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 useEffect with useCallback. 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 normalizeColorValue color value to accept directly an element (HTMLElement | null), instead of a ref
  • since we're using a callback ref, there's no need to keep the customColorPaletteRef variable
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.


const hasMultipleColorOrigins =
colors.length > 0 &&
( colors as PaletteObject[] )[ 0 ].colors !== undefined;
Expand Down Expand Up @@ -280,14 +297,14 @@ function UnforwardedColorPalette(
const renderCustomColorPicker = () => (
<DropdownContentWrapper paddingSize="none">
<ColorPicker
color={ normalizeColorValue( value, customColorPaletteRef ) }
color={ normalizedColorValue }
onChange={ ( color ) => onChange( color ) }
enableAlpha={ enableAlpha }
/>
</DropdownContentWrapper>
);

const colordColor = colord( value ?? '' );
const colordColor = colord( normalizedColorValue ?? '' );

const valueWithoutLeadingHash = value?.startsWith( '#' )
? value.substring( 1 )
Expand Down