ColorPicker: store internal HSLA state for better slider UX

packages/components/CHANGELOG.md

@@ -23,6 +23,7 @@
 
 ### Enhancements
 
+-   `ColorPicker`: improve the UX around HSL sliders ([#57555](https://github.com/WordPress/gutenberg/pull/57555)).
 -   Update `ariakit` to version `0.3.10` ([#57325](https://github.com/WordPress/gutenberg/pull/57325)).
 -   Update `@ariakit/react` to version `0.3.12` and @ariakit/test to version `0.3.7` ([#57547](https://github.com/WordPress/gutenberg/pull/57547)).
 -   `DropdownMenuV2`: do not collapse suffix width ([#57238](https://github.com/WordPress/gutenberg/pull/57238)).

packages/components/src/color-picker/hsl-input.tsx

@@ -3,14 +3,61 @@
  */
 import { colord } from 'colord';
 
+/**
+ * WordPress dependencies
+ */
+import { useState, useEffect, useMemo } from '@wordpress/element';
+
 /**
  * Internal dependencies
  */
 import { InputWithSlider } from './input-with-slider';
 import type { HslInputProps } from './types';
 
 export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => {
-	const { h, s, l, a } = color.toHsl();
+	const colorPropHSLA = useMemo( () => color.toHsl(), [ color ] );
+
+	const [ internalHSLA, setInternalHSLA ] = useState( { ...colorPropHSLA } );
+
+	const isInternalColorSameAsReceivedColor = color.isEqual(
+		colord( internalHSLA )
+	);
+
+	useEffect( () => {
+		if ( ! isInternalColorSameAsReceivedColor ) {
+			// Keep internal HSLA color up to date with the received color prop
+			setInternalHSLA( colorPropHSLA );
+		}
+	}, [ colorPropHSLA, isInternalColorSameAsReceivedColor ] );
+
+	// If the internal color is equal to the received color prop, we can use the
+	// HSLA values from the local state which, compared to the received color prop,
+	// retain more details about the actual H and S values that the user selected,
+	// and thus allow for better UX when interacting with the H and S sliders.
+	const colorValue = isInternalColorSameAsReceivedColor
+		? internalHSLA
+		: colorPropHSLA;
+
+	const updateHSLAValue = (
+		partialNewValue: Partial< typeof colorPropHSLA >
+	) => {
+		const nextOnChangeValue = colord( {
+			...colorValue,
+			...partialNewValue,
+		} );
+
+		// Fire `onChange` only if the resulting color is different from the
+		// current one.
+		// Otherwise, update the internal HSLA color to cause a re-render.
+		if ( ! color.isEqual( nextOnChangeValue ) ) {
+			onChange( nextOnChangeValue );
+		} else {
+			setInternalHSLA( ( prevHSLA ) => ( {
+				...prevHSLA,
+				...partialNewValue,
+			} ) );
+		}
+	};
 
 	return (
 		<>
@@ -19,43 +66,29 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => {
 				max={ 359 }
 				label="Hue"
 				abbreviation="H"
-				value={ h }
+				value={ colorValue.h }
 				onChange={ ( nextH: number ) => {
-					onChange( colord( { h: nextH, s, l, a } ) );
+					updateHSLAValue( { h: nextH } );
 				} }
 			/>
 			<InputWithSlider
 				min={ 0 }
 				max={ 100 }
 				label="Saturation"
 				abbreviation="S"
-				value={ s }
+				value={ colorValue.s }
 				onChange={ ( nextS: number ) => {
-					onChange(
-						colord( {
-							h,
-							s: nextS,
-							l,
-							a,
-						} )
-					);
+					updateHSLAValue( { s: nextS } );
 				} }
 			/>
 			<InputWithSlider
 				min={ 0 }
 				max={ 100 }
 				label="Lightness"
 				abbreviation="L"
-				value={ l }
+				value={ colorValue.l }
 				onChange={ ( nextL: number ) => {
-					onChange(
-						colord( {
-							h,
-							s,
-							l: nextL,
-							a,
-						} )
-					);
+					updateHSLAValue( { l: nextL } );
 				} }
 			/>
 			{ enableAlpha && (
@@ -64,16 +97,9 @@ export const HslInput = ( { color, onChange, enableAlpha }: HslInputProps ) => {
 					max={ 100 }
 					label="Alpha"
 					abbreviation="A"
-					value={ Math.trunc( 100 * a ) }
+					value={ Math.trunc( 100 * colorValue.a ) }
 					onChange={ ( nextA: number ) => {
-						onChange(
-							colord( {
-								h,
-								s,
-								l,
-								a: nextA / 100,
-							} )
-						);
+						updateHSLAValue( { a: nextA / 100 } );
 					} }
 				/>
 			) }

packages/components/src/color-picker/test/index.tsx

@@ -1,13 +1,19 @@
 /**
  * External dependencies
  */
-import { screen, render } from '@testing-library/react';
+import { fireEvent, screen, render, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 
+/**
+ * WordPress dependencies
+ */
+import { useState } from '@wordpress/element';
+
 /**
  * Internal dependencies
  */
 import { ColorPicker } from '..';
+import { click } from '@ariakit/test';
 
 const hslaMatcher = expect.objectContaining( {
 	h: expect.any( Number ),
@@ -133,20 +139,39 @@ describe( 'ColorPicker', () => {
 		} );
 	} );
 
-	describe.each( [
-		[ 'hue', 'Hue', '#aad52a' ],
-		[ 'saturation', 'Saturation', '#20dfdf' ],
-		[ 'lightness', 'Lightness', '#95eaea' ],
-	] )( 'HSL inputs', ( colorInput, inputLabel, expected ) => {
-		it( `should fire onChange with the correct value when the ${ colorInput } value is updated`, async () => {
+	describe( 'HSL inputs', () => {
+		it( 'sliders should use accurate H and S values based on user interaction when possible', async () => {
 			const user = userEvent.setup();
 			const onChange = jest.fn();
-			const color = '#2ad5d5';
+
+			const ControlledColorPicker = ( {
+				onChange: onChangeProp,
+				...restProps
+			}: React.ComponentProps< typeof ColorPicker > ) => {
+				const [ colorState, setColorState ] = useState( '#000000' );
+
+				const internalOnChange: typeof onChangeProp = ( newColor ) => {
+					onChangeProp?.( newColor );
+					setColorState( newColor );
+				};
+
+				return (
+					<>
+						<ColorPicker
+							{ ...restProps }
+							onChange={ internalOnChange }
+							color={ colorState }
+						/>
+						<button onClick={ () => setColorState( '#4d87ba' ) }>
+							Set color to #4d87ba
+						</button>
+					</>
+				);
+			};
 
 			render(
-				<ColorPicker
+				<ControlledColorPicker
 					onChange={ onChange }
-					color={ color }
 					enableAlpha={ false }
 				/>
 			);
@@ -156,16 +181,165 @@ describe( 'ColorPicker', () => {
 
 			await user.selectOptions( formatSelector, 'hsl' );
 
-			const inputElement = screen.getByRole( 'spinbutton', {
-				name: inputLabel,
+			const hueSliders = screen.getAllByRole( 'slider', {
+				name: 'Hue',
 			} );
-			expect( inputElement ).toBeVisible();
+			expect( hueSliders ).toHaveLength( 2 );
 
-			await user.clear( inputElement );
-			await user.type( inputElement, '75' );
+			// Reason for the `!` post-fix expression operator: if the check above
+			// doesn't fail, we're guaranteed that `hueSlider` is not undefined.
+			const hueSlider = hueSliders.at( -1 )!;
+			const saturationSlider = screen.getByRole( 'slider', {
+				name: 'Saturation',
+			} );
+			const lightnessSlider = screen.getByRole( 'slider', {
+				name: 'Lightness',
+			} );
+			const hueNumberInput = screen.getByRole( 'spinbutton', {
+				name: 'Hue',
+			} );
+			const saturationNumberInput = screen.getByRole( 'spinbutton', {
+				name: 'Saturation',
+			} );
+			const lightnessNumberInput = screen.getByRole( 'spinbutton', {
+				name: 'Lightness',
+			} );
+
+			// All initial inputs should have a value of `0` since the color is black.
+			expect( hueSlider ).toHaveValue( '0' );
+			expect( saturationSlider ).toHaveValue( '0' );
+			expect( lightnessSlider ).toHaveValue( '0' );
+			expect( hueNumberInput ).toHaveValue( 0 );
+			expect( saturationNumberInput ).toHaveValue( 0 );
+			expect( lightnessNumberInput ).toHaveValue( 0 );
+
+			// Interact with the Hue slider, it should change its value (and the
+			// value of the associated number input), but it shouldn't cause the
+			// `onChange` callback to fire, since the resulting color is still black.
+			fireEvent.change( hueSlider, { target: { value: 80 } } );
+
+			expect( hueSlider ).toHaveValue( '80' );
+			expect( hueNumberInput ).toHaveValue( 80 );
+			expect( onChange ).not.toHaveBeenCalled();
+
+			// Interact with the Saturation slider, it should change its value (and the
+			// value of the associated number input), but it shouldn't cause the
+			// `onChange` callback to fire, since the resulting color is still black.
+			fireEvent.change( saturationSlider, { target: { value: 50 } } );
+
+			expect( saturationSlider ).toHaveValue( '50' );
+			expect( saturationNumberInput ).toHaveValue( 50 );
+			expect( onChange ).not.toHaveBeenCalled();
+
+			// Interact with the Lightness slider, it should change its value (and the
+			// value of the associated number input). It should also cause the
+			// `onChange` callback to fire, since changing the lightness actually
+			// causes the color to change.
+			fireEvent.change( lightnessSlider, { target: { value: 30 } } );
 
+			await waitFor( () =>
+				expect( lightnessSlider ).toHaveValue( '30' )
+			);
+			expect( lightnessNumberInput ).toHaveValue( 30 );
+			expect( onChange ).toHaveBeenCalledTimes( 1 );
+			expect( onChange ).toHaveBeenLastCalledWith( '#597326' );
+
+			// Interact with the Lightness slider, setting to 100 (ie. white).
+			// It should also cause the `onChange` callback to fire, and reset the
+			// hue and saturation inputs to `0`.
+			fireEvent.change( lightnessSlider, { target: { value: 100 } } );
+
+			await waitFor( () =>
+				expect( lightnessSlider ).toHaveValue( '100' )
+			);
+			expect( lightnessNumberInput ).toHaveValue( 100 );
+			expect( hueSlider ).toHaveValue( '0' );
+			expect( saturationSlider ).toHaveValue( '0' );
+			expect( hueNumberInput ).toHaveValue( 0 );
+			expect( saturationNumberInput ).toHaveValue( 0 );
+			expect( onChange ).toHaveBeenCalledTimes( 2 );
+			expect( onChange ).toHaveBeenLastCalledWith( '#ffffff' );
+
+			// Interact with the Hue slider, it should change its value (and the
+			// value of the associated number input), but it shouldn't cause the
+			// `onChange` callback to fire, since the resulting color is still white.
+			fireEvent.change( hueSlider, { target: { value: 147 } } );
+
+			expect( hueSlider ).toHaveValue( '147' );
+			expect( hueNumberInput ).toHaveValue( 147 );
+			expect( onChange ).toHaveBeenCalledTimes( 2 );
+
+			// Interact with the Saturation slider, it should change its value (and the
+			// value of the associated number input), but it shouldn't cause the
+			// `onChange` callback to fire, since the resulting color is still white.
+			fireEvent.change( saturationSlider, { target: { value: 82 } } );
+
+			expect( saturationSlider ).toHaveValue( '82' );
+			expect( saturationNumberInput ).toHaveValue( 82 );
+			expect( onChange ).toHaveBeenCalledTimes( 2 );
+
+			// Interact with the Lightness slider, it should change its value (and the
+			// value of the associated number input). It should also cause the
+			// `onChange` callback to fire, since changing the lightness actually
+			// causes the color to change.
+			fireEvent.change( lightnessSlider, { target: { value: 14 } } );
+
+			await waitFor( () =>
+				expect( lightnessSlider ).toHaveValue( '14' )
+			);
+			expect( lightnessNumberInput ).toHaveValue( 14 );
 			expect( onChange ).toHaveBeenCalledTimes( 3 );
-			expect( onChange ).toHaveBeenLastCalledWith( expected );
+			expect( onChange ).toHaveBeenLastCalledWith( '#064121' );
+
+			// Set the color externally. All inputs should update to match the H/S/L
+			// value of the new color.
+			const setColorButton = screen.getByRole( 'button', {
+				name: /set color/i,
+			} );
+			await click( setColorButton );
+
+			expect( hueSlider ).toHaveValue( '208' );
+			expect( hueNumberInput ).toHaveValue( 208 );
+			expect( saturationSlider ).toHaveValue( '44' );
+			expect( saturationNumberInput ).toHaveValue( 44 );
+			expect( lightnessSlider ).toHaveValue( '52' );
+			expect( lightnessNumberInput ).toHaveValue( 52 );
+		} );
+
+		describe.each( [
+			[ 'hue', 'Hue', '#aad52a' ],
+			[ 'saturation', 'Saturation', '#20dfdf' ],
+			[ 'lightness', 'Lightness', '#95eaea' ],
+		] )( 'HSL inputs', ( colorInput, inputLabel, expected ) => {
+			it( `should fire onChange with the correct value when the ${ colorInput } value is updated`, async () => {
+				const user = userEvent.setup();
+				const onChange = jest.fn();
+				const color = '#2ad5d5';
+
+				render(
+					<ColorPicker
+						onChange={ onChange }
+						color={ color }
+						enableAlpha={ false }
+					/>
+				);
+
+				const formatSelector = screen.getByRole( 'combobox' );
+				expect( formatSelector ).toBeVisible();
+
+				await user.selectOptions( formatSelector, 'hsl' );
+
+				const inputElement = screen.getByRole( 'spinbutton', {
+					name: inputLabel,
+				} );
+				expect( inputElement ).toBeVisible();
+
+				await user.clear( inputElement );
+				await user.type( inputElement, '75' );
+
+				expect( onChange ).toHaveBeenCalledTimes( 3 );
+				expect( onChange ).toHaveBeenLastCalledWith( expected );
+			} );
 		} );
 	} );
 } );