From 0a1b080a38d73d3ca72901fa41a97677f508ff46 Mon Sep 17 00:00:00 2001 From: Pareder Date: Fri, 5 Jun 2026 15:34:08 +0300 Subject: [PATCH 1/3] feat: Improve keyboard & screen-reader accessibility --- assets/index.less | 5 + src/ColorPicker.tsx | 18 ++ src/components/ColorBlock.tsx | 14 +- src/components/Handler.tsx | 96 ++++++++++- src/components/Picker.tsx | 38 +++- src/components/Slider.tsx | 14 ++ src/interface.ts | 8 + tests/__snapshots__/index.test.tsx.snap | 220 +++++++++++++++++++++--- tests/index.test.tsx | 166 ++++++++++++++++-- 9 files changed, 523 insertions(+), 56 deletions(-) diff --git a/assets/index.less b/assets/index.less index 5fcda105..4306e65b 100644 --- a/assets/index.less +++ b/assets/index.less @@ -55,6 +55,11 @@ border: @handler-border-size solid #fff; border-radius: 50%; box-shadow: 0 0 1px 1px rgba(0, 0, 0, 0.06); + &:focus-visible, + &:focus-within { + outline: 2px solid #1677ff; + outline-offset: 1px; + } } &-handler-sm { width: @sm-handler-size; diff --git a/src/ColorPicker.tsx b/src/ColorPicker.tsx index 64d52e76..aaf26d61 100644 --- a/src/ColorPicker.tsx +++ b/src/ColorPicker.tsx @@ -10,6 +10,15 @@ import useColorState from './hooks/useColorState'; import useComponent, { type Components } from './hooks/useComponent'; import type { BaseColorPickerProps, ColorGenInput } from './interface'; +const defaultLocale: Required['locale'] = { + picker: 'Color picker', + pickerDescription: '2D slider', + hue: 'Hue', + alpha: 'Alpha', + saturation: 'Saturation', + brightness: 'Brightness', +}; + const HUE_COLORS = [ { color: 'rgb(255, 0, 0)', @@ -67,8 +76,14 @@ const ColorPicker = forwardRef( disabledAlpha = false, disabled = false, components, + locale, } = props; + const mergedLocale = useMemo( + () => ({ ...defaultLocale, ...locale }), + [locale], + ); + // ========================== Components ========================== const [Slider] = useComponent(components); @@ -134,6 +149,7 @@ const ColorPicker = forwardRef(
@@ -151,6 +167,7 @@ const ColorPicker = forwardRef( value={colorValue.getHue()} onChange={onHueChange} onChangeComplete={onHueChangeComplete} + aria-label={mergedLocale.hue} /> {!disabledAlpha && ( ( value={colorValue.a * 100} onChange={onAlphaChange} onChangeComplete={onAlphaChangeComplete} + aria-label={mergedLocale.alpha} /> )}
diff --git a/src/components/ColorBlock.tsx b/src/components/ColorBlock.tsx index e78b9f3e..77e0a51f 100644 --- a/src/components/ColorBlock.tsx +++ b/src/components/ColorBlock.tsx @@ -1,34 +1,26 @@ import { clsx } from 'clsx'; import React from 'react'; -export type ColorBlockProps = { +export type ColorBlockProps = React.HTMLAttributes & { color: string; prefixCls?: string; - className?: string; - style?: React.CSSProperties; /** Internal usage. Only used in antd ColorPicker semantic structure only */ innerClassName?: string; /** Internal usage. Only used in antd ColorPicker semantic structure only */ innerStyle?: React.CSSProperties; - onClick?: React.MouseEventHandler; }; const ColorBlock: React.FC = ({ color, prefixCls, className, - style, innerClassName, innerStyle, - onClick, + ...props }) => { const colorBlockCls = `${prefixCls}-color-block`; return ( -
+
`) that mutate the value. */ +const VALUE_KEYS = [ + 'ArrowLeft', + 'ArrowRight', + 'ArrowUp', + 'ArrowDown', + 'Home', + 'End', + 'PageUp', + 'PageDown', +]; + +const isVerticalKey = (key: string) => key === 'ArrowUp' || key === 'ArrowDown'; + +// The range input is a keyboard / screen-reader proxy only — the visible thumb +// is the wrapping
. It must stay focusable and in the a11y tree, so it is +// hidden with `opacity` (not `display`/`visibility`). These styles are inlined +// rather than left to the stylesheet so consumers that don't ship our CSS +// (e.g. antd's own styling) still get a hidden input out of the box. +const RANGE_INPUT_STYLE: React.CSSProperties = { + position: 'absolute', + inset: 0, + width: '100%', + height: '100%', + margin: 0, + padding: 0, + opacity: 0, + pointerEvents: 'none', +}; + +interface HandlerAxis + extends Omit< + React.InputHTMLAttributes, + 'size' | 'value' | 'onChange' + > { + value: number; + onChange: (value: number) => void; + onChangeComplete: (value: number) => void; +} + +export interface HandlerProps { size?: HandlerSize; color?: string; prefixCls?: string; -}> = ({ size = 'default', color, prefixCls }) => { + disabled?: boolean; + x: HandlerAxis; + y?: HandlerAxis; +} + +const Handler: React.FC = ({ + size = 'default', + color, + prefixCls, + disabled, + x, + y, +}) => { + // The browser ignores Up/Down on a horizontal range, so the vertical axis is + // handled here: clamp to its own [min, max] and emit through its callbacks. + const handleKeyDown = (event: React.KeyboardEvent) => { + if (!y || !isVerticalKey(event.key)) { + return; + } + event.preventDefault(); + const step = Number(y.step ?? 1) || 1; + const min = Number(y.min ?? 0); + const max = Number(y.max ?? 100); + const delta = event.key === 'ArrowUp' ? step : -step; + y.onChange(Math.min(max, Math.max(min, y.value + delta))); + }; + return (
+ style={{ position: 'relative', backgroundColor: color }} + > + x.onChange(Number(event.target.value))} + onKeyDown={y ? handleKeyDown : undefined} + onKeyUp={event => { + if (!VALUE_KEYS.includes(event.key)) { + return; + } + if (y && isVerticalKey(event.key)) { + y.onChangeComplete(y.value); + } else { + x.onChangeComplete(Number(event.currentTarget.value)); + } + }} + /> +
); }; diff --git a/src/components/Picker.tsx b/src/components/Picker.tsx index 93a85eb0..8b0155d7 100644 --- a/src/components/Picker.tsx +++ b/src/components/Picker.tsx @@ -2,7 +2,7 @@ import type { FC } from 'react'; import React, { useRef } from 'react'; import useColorDrag from '../hooks/useColorDrag'; import type { BaseColorPickerProps, TransformOffset } from '../interface'; -import { calcOffset, calculateColor } from '../util'; +import { calcOffset, calculateColor, generateColor } from '../util'; import { useEvent } from '@rc-component/util'; import Handler from './Handler'; @@ -17,6 +17,7 @@ const Picker: FC = ({ prefixCls, onChangeComplete, disabled, + locale, }) => { const pickerRef = useRef(); const transformRef = useRef(); @@ -43,6 +44,16 @@ const Picker: FC = ({ disabledDrag: disabled, }); + // ===================== Keyboard (2-D handler) ===================== + const hsb = color.toHsb(); + + // Build a new color from the current one with a single HSB channel changed. + const changeColor = (channel: 's' | 'b', percent: number) => { + const next = generateColor({ ...hsb, [channel]: percent / 100 }); + colorRef.current = next; + onChange(next); + }; + return (
= ({ > - + changeColor('s', percent), + onChangeComplete: () => onChangeComplete?.(colorRef.current), + }} + y={{ + min: 0, + max: 100, + value: hsb.b * 100, + onChange: percent => changeColor('b', percent), + onChangeComplete: () => onChangeComplete?.(colorRef.current), + }} + />
void; type: HsbaColorType; color: Color; + 'aria-label'?: string; } const Slider: React.FC = props => { @@ -33,6 +34,10 @@ const Slider: React.FC = props => { onChangeComplete, color, type, + min, + max, + value, + 'aria-label': ariaLabel, } = props; const sliderRef = useRef(null); @@ -103,6 +108,15 @@ const Slider: React.FC = props => { size="small" color={handleColor.toHexString()} prefixCls={prefixCls} + disabled={disabled} + x={{ + 'aria-label': ariaLabel, + min, + max, + value, + onChange, + onChangeComplete, + }} /> diff --git a/src/interface.ts b/src/interface.ts index 4fd9077c..ecc29844 100644 --- a/src/interface.ts +++ b/src/interface.ts @@ -40,6 +40,14 @@ export interface BaseColorPickerProps { color?: Color; prefixCls?: string; disabled?: boolean; + locale?: { + picker?: string; + pickerDescription?: string; + hue?: string; + alpha?: string; + saturation?: string; + brightness?: string; + }; onChange?: ( color: Color, info?: { type?: HsbaColorType; value?: number }, diff --git a/tests/__snapshots__/index.test.tsx.snap b/tests/__snapshots__/index.test.tsx.snap index 1b28a513..d7753f98 100644 --- a/tests/__snapshots__/index.test.tsx.snap +++ b/tests/__snapshots__/index.test.tsx.snap @@ -17,8 +17,21 @@ exports[`ColorPicker > Should component onChange work on no control mode 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should component onChange work on no control mode 1`] = ` >
+ style="position: relative; background-color: rgb(0, 106, 255);" + > + +
Should component onChange work on no control mode 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should component render correct 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should component render correct 1`] = ` >
+ style="position: relative; background-color: rgb(0, 106, 255);" + > + +
Should component render correct 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should custom panel work 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should custom panel work 1`] = ` >
+ style="position: relative; background-color: rgb(0, 106, 255);" + > + +
Should custom panel work 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should disabled alpha work 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should disabled alpha work 1`] = ` >
+ style="position: relative; background-color: rgb(0, 106, 255);" + > + +
Should prefixCls work 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
Should prefixCls work 1`] = ` >
+ style="position: relative; background-color: rgb(0, 106, 255);" + > + +
Should prefixCls work 1`] = ` >
+ style="position: relative; background-color: rgb(22, 119, 255);" + > + +
{ const App = () => ; const { container } = render(); expect( - container.querySelector('.rc-color-picker-handler').getAttribute('style'), - ).toEqual('background-color: rgb(23, 120, 255);'); + (container.querySelector('.rc-color-picker-handler') as HTMLElement).style + .backgroundColor, + ).toEqual('rgb(23, 120, 255)'); }); it('Should rgb string work', () => { const App = () => ; const { container } = render(); expect( - container.querySelector('.rc-color-picker-handler').getAttribute('style'), - ).toEqual('background-color: rgb(23, 120, 255);'); + (container.querySelector('.rc-color-picker-handler') as HTMLElement).style + .backgroundColor, + ).toEqual('rgb(23, 120, 255)'); }); it('Should hex string work', () => { const App = () => ; const { container } = render(); expect( - container.querySelector('.rc-color-picker-handler').getAttribute('style'), - ).toEqual('background-color: rgb(23, 120, 255);'); + (container.querySelector('.rc-color-picker-handler') as HTMLElement).style + .backgroundColor, + ).toEqual('rgb(23, 120, 255)'); }); it('Should hsb obj work', () => { const App = () => ; const { container } = render(); expect( - container.querySelector('.rc-color-picker-handler').getAttribute('style'), - ).toEqual('background-color: rgb(23, 120, 255);'); + (container.querySelector('.rc-color-picker-handler') as HTMLElement).style + .backgroundColor, + ).toEqual('rgb(23, 120, 255)'); }); it('Should rgb obj work', () => { const App = () => ; const { container } = render(); expect( - container.querySelector('.rc-color-picker-handler').getAttribute('style'), - ).toEqual('background-color: rgb(23, 120, 255);'); + (container.querySelector('.rc-color-picker-handler') as HTMLElement).style + .backgroundColor, + ).toEqual('rgb(23, 120, 255)'); }); it('Should disabled work', () => { @@ -497,4 +508,137 @@ describe('ColorPicker', () => { spy.mockRestore(); }); + + describe('Accessibility tests', () => { + const Controlled = (props: Record) => { + const [value, setValue] = useState(defaultColor); + return ( + <> + +
{value.toHsbString()}
+ + ); + }; + + it('Should expose default aria-labels on the handles', () => { + render(); + + expect(screen.getByLabelText('Color picker')).toBeTruthy(); + expect(screen.getByLabelText('Hue')).toBeTruthy(); + expect(screen.getByLabelText('Alpha')).toBeTruthy(); + }); + + it('Should describe saturation & brightness via aria-valuetext by default', () => { + render(); + + expect(screen.getByLabelText('Color picker')).toHaveAttribute( + 'aria-valuetext', + 'Saturation 91%, Brightness 100%', + ); + }); + + it('Should override the aria-labels through the locale prop', () => { + render( + , + ); + + expect(screen.getByLabelText('Sélecteur')).toBeTruthy(); + expect(screen.getByLabelText('Teinte')).toBeTruthy(); + expect(screen.getByLabelText('Transparence')).toBeTruthy(); + expect(screen.getByLabelText('Sélecteur')).toHaveAttribute( + 'aria-valuetext', + 'Sat 91%, Lum 100%', + ); + }); + + it('Should change brightness with the Down arrow on the picker handle', () => { + const onChangeComplete = vi.fn(); + render(); + + const picker = screen.getByLabelText('Color picker'); + fireEvent.keyDown(picker, { key: 'ArrowDown' }); + fireEvent.keyUp(picker, { key: 'ArrowDown' }); + + // brightness starts at 100% and steps down to 99% + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 91%, 99%)', + ); + expect(onChangeComplete).toHaveBeenCalled(); + }); + + it('Should clamp brightness at 100% when pressing the Up arrow', () => { + render(); + + const picker = screen.getByLabelText('Color picker'); + fireEvent.keyDown(picker, { key: 'ArrowUp' }); + + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 91%, 100%)', + ); + }); + + it('Should increase saturation on the picker (Arrow Right)', () => { + const onChangeComplete = vi.fn(); + render(); + + const picker = screen.getByLabelText('Color picker'); + fireEvent.change(picker, { target: { value: '92' } }); + fireEvent.keyUp(picker, { key: 'ArrowRight' }); + + // saturation starts at 91% and steps up to 92% + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 92%, 100%)', + ); + expect(onChangeComplete).toHaveBeenCalled(); + }); + + it('Should decrease saturation on the picker (Arrow Left)', () => { + const onChangeComplete = vi.fn(); + render(); + + const picker = screen.getByLabelText('Color picker'); + fireEvent.change(picker, { target: { value: '90' } }); + fireEvent.keyUp(picker, { key: 'ArrowLeft' }); + + // saturation starts at 91% and steps down to 90% + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 90%, 100%)', + ); + expect(onChangeComplete).toHaveBeenCalled(); + }); + + it('Should change hue when the hue slider value changes via keyboard', () => { + const onChangeComplete = vi.fn(); + render(); + + const hue = screen.getByLabelText('Hue'); + fireEvent.change(hue, { target: { value: '100' } }); + fireEvent.keyUp(hue, { key: 'ArrowRight' }); + + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(100, 91%, 100%)', + ); + expect(onChangeComplete).toHaveBeenCalled(); + }); + + it('Should change alpha when the alpha slider value changes via keyboard', () => { + render(); + + const alpha = screen.getByLabelText('Alpha'); + fireEvent.change(alpha, { target: { value: '50' } }); + + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsba(215, 91%, 100%, 0.50)', + ); + }); + }); }); From 418d954ac01ee54122a3b406187568b9ab502e74 Mon Sep 17 00:00:00 2001 From: Pareder Date: Tue, 30 Jun 2026 13:39:04 +0300 Subject: [PATCH 2/3] Fix after review --- src/components/Handler.tsx | 87 ++++++++++++------ src/components/Picker.tsx | 14 ++- tests/__snapshots__/index.test.tsx.snap | 28 +++--- tests/index.test.tsx | 114 ++++++++++++++++++++++++ 4 files changed, 200 insertions(+), 43 deletions(-) diff --git a/src/components/Handler.tsx b/src/components/Handler.tsx index d9ae8469..98d454f2 100644 --- a/src/components/Handler.tsx +++ b/src/components/Handler.tsx @@ -34,11 +34,10 @@ const RANGE_INPUT_STYLE: React.CSSProperties = { pointerEvents: 'none', }; -interface HandlerAxis - extends Omit< - React.InputHTMLAttributes, - 'size' | 'value' | 'onChange' - > { +interface HandlerAxis extends Omit< + React.InputHTMLAttributes, + 'size' | 'value' | 'onChange' +> { value: number; onChange: (value: number) => void; onChangeComplete: (value: number) => void; @@ -61,18 +60,65 @@ const Handler: React.FC = ({ x, y, }) => { - // The browser ignores Up/Down on a horizontal range, so the vertical axis is - // handled here: clamp to its own [min, max] and emit through its callbacks. + const xValueRef = React.useRef(x.value); + xValueRef.current = x.value; + const yValueRef = React.useRef(y?.value); + yValueRef.current = y?.value; + + const stepAxis = ( + axis: HandlerAxis, + ref: React.MutableRefObject, + direction: 1 | -1, + ) => { + const stepSize = Number(axis.step ?? 1) || 1; + const min = Number(axis.min ?? 0); + const max = Number(axis.max ?? 100); + const current = ref.current ?? axis.value; + const next = Math.min(max, Math.max(min, current + direction * stepSize)); + ref.current = next; + axis.onChange(next); + }; + + // Left/Right drives the horizontal axis; Up/Down the vertical one (or the + // horizontal one for 1-D sliders). We handle these instead of the native range + // so behaviour is deterministic across browsers and safe under rapid presses. const handleKeyDown = (event: React.KeyboardEvent) => { - if (!y || !isVerticalKey(event.key)) { - return; + switch (event.key) { + case 'ArrowRight': + stepAxis(x, xValueRef, 1); + break; + case 'ArrowLeft': + stepAxis(x, xValueRef, -1); + break; + case 'ArrowUp': + if (y) { + stepAxis(y, yValueRef, 1); + } else { + stepAxis(x, xValueRef, 1); + } + break; + case 'ArrowDown': + if (y) { + stepAxis(y, yValueRef, -1); + } else { + stepAxis(x, xValueRef, -1); + } + break; + default: + return; } event.preventDefault(); - const step = Number(y.step ?? 1) || 1; - const min = Number(y.min ?? 0); - const max = Number(y.max ?? 100); - const delta = event.key === 'ArrowUp' ? step : -step; - y.onChange(Math.min(max, Math.max(min, y.value + delta))); + }; + + const handleKeyUp = (event: React.KeyboardEvent) => { + if (!VALUE_KEYS.includes(event.key)) { + return; + } + if (y && isVerticalKey(event.key)) { + y.onChangeComplete(yValueRef.current ?? y.value); + } else { + x.onChangeComplete(xValueRef.current ?? x.value); + } }; return ( @@ -90,17 +136,8 @@ const Handler: React.FC = ({ style={RANGE_INPUT_STYLE} disabled={disabled} onChange={event => x.onChange(Number(event.target.value))} - onKeyDown={y ? handleKeyDown : undefined} - onKeyUp={event => { - if (!VALUE_KEYS.includes(event.key)) { - return; - } - if (y && isVerticalKey(event.key)) { - y.onChangeComplete(y.value); - } else { - x.onChangeComplete(Number(event.currentTarget.value)); - } - }} + onKeyDown={handleKeyDown} + onKeyUp={handleKeyUp} />
); diff --git a/src/components/Picker.tsx b/src/components/Picker.tsx index 8b0155d7..1e473c4b 100644 --- a/src/components/Picker.tsx +++ b/src/components/Picker.tsx @@ -22,6 +22,10 @@ const Picker: FC = ({ const pickerRef = useRef(); const transformRef = useRef(); const colorRef = useRef(color); + // Keep the ref synced with the controlled color so the keyboard handlers below + // always read the latest value — even when several presses (across both axes) + // fire before the parent re-renders, so one axis can't overwrite the other. + colorRef.current = color; const onDragChange = useEvent((offsetValue: TransformOffset) => { const calcColor = calculateColor({ @@ -43,13 +47,15 @@ const Picker: FC = ({ onDragChangeComplete: () => onChangeComplete?.(colorRef.current), disabledDrag: disabled, }); - // ===================== Keyboard (2-D handler) ===================== const hsb = color.toHsb(); - // Build a new color from the current one with a single HSB channel changed. + // Build a new color from the *latest* one (the ref, not the render-time prop) const changeColor = (channel: 's' | 'b', percent: number) => { - const next = generateColor({ ...hsb, [channel]: percent / 100 }); + const next = generateColor({ + ...colorRef.current.toHsb(), + [channel]: percent / 100, + }); colorRef.current = next; onChange(next); }; @@ -91,7 +97,7 @@ const Picker: FC = ({
Should component onChange work on no control mode 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="91.37254901960785" /> @@ -65,7 +65,7 @@ exports[`ColorPicker > Should component onChange work on no control mode 1`] = ` max="359" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="215" /> @@ -97,7 +97,7 @@ exports[`ColorPicker > Should component onChange work on no control mode 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="100" /> @@ -150,7 +150,7 @@ exports[`ColorPicker > Should component render correct 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="91.37254901960785" /> @@ -188,7 +188,7 @@ exports[`ColorPicker > Should component render correct 1`] = ` max="359" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="215" /> @@ -220,7 +220,7 @@ exports[`ColorPicker > Should component render correct 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="100" /> @@ -276,7 +276,7 @@ exports[`ColorPicker > Should custom panel work 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="91.37254901960785" /> @@ -314,7 +314,7 @@ exports[`ColorPicker > Should custom panel work 1`] = ` max="359" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="215" /> @@ -346,7 +346,7 @@ exports[`ColorPicker > Should custom panel work 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="100" /> @@ -400,7 +400,7 @@ exports[`ColorPicker > Should disabled alpha work 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="91.37254901960785" /> @@ -438,7 +438,7 @@ exports[`ColorPicker > Should disabled alpha work 1`] = ` max="359" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="215" /> @@ -491,7 +491,7 @@ exports[`ColorPicker > Should prefixCls work 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="91.37254901960785" /> @@ -529,7 +529,7 @@ exports[`ColorPicker > Should prefixCls work 1`] = ` max="359" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="215" /> @@ -561,7 +561,7 @@ exports[`ColorPicker > Should prefixCls work 1`] = ` max="100" min="0" step="1" - style="position: absolute; inset: 0; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" + style="position: absolute; inset: 0px; width: 100%; height: 100%; margin: 0px; padding: 0px; opacity: 0; pointer-events: none;" type="range" value="100" /> diff --git a/tests/index.test.tsx b/tests/index.test.tsx index 52b62171..31818395 100644 --- a/tests/index.test.tsx +++ b/tests/index.test.tsx @@ -616,6 +616,72 @@ describe('ColorPicker', () => { expect(onChangeComplete).toHaveBeenCalled(); }); + it('Should step from the latest value on rapid saturation presses', () => { + render(); + + const picker = screen.getByLabelText('Color picker') as HTMLInputElement; + + // Two Left presses dispatched in the same batch (before the parent + // re-renders). Reading the stale prop would step 91% -> 90% twice; the + // latest-value ref keeps them stepping 91% -> 89%. + act(() => { + picker.dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true }), + ); + picker.dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true }), + ); + }); + + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 89%, 100%)', + ); + }); + + it('Should step from the latest value on rapid brightness presses', () => { + render(); + + const picker = screen.getByLabelText('Color picker') as HTMLInputElement; + + // Two Down presses dispatched in the same batch (before the parent + // re-renders). Reading the stale prop would step 100% -> 99% twice; the + // latest-value ref keeps them stepping 100% -> 98%. + act(() => { + picker.dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), + ); + picker.dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), + ); + }); + + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 91%, 98%)', + ); + }); + + it('Should step from the latest value on rapid saturation and brightness presses', () => { + render(); + + const picker = screen.getByLabelText('Color picker') as HTMLInputElement; + + // One Down (brightness 100% -> 99%) and one Left (saturation 91% -> 90%) + // dispatched in the same batch. Deriving the second change from the stale + // prop would revert the first axis; the refs keep both. + act(() => { + picker.dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), + ); + picker.dispatchEvent( + new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true }), + ); + }); + + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 90%, 99%)', + ); + }); + it('Should change hue when the hue slider value changes via keyboard', () => { const onChangeComplete = vi.fn(); render(); @@ -640,5 +706,53 @@ describe('ColorPicker', () => { 'hsba(215, 91%, 100%, 0.50)', ); }); + + it('Should ignore keys that do not change the value', () => { + const onChange = vi.fn(); + const onChangeComplete = vi.fn(); + render( + , + ); + + const picker = screen.getByLabelText('Color picker'); + // A non-value key (e.g. Tab) must neither step (keydown) nor commit (keyup). + fireEvent.keyDown(picker, { key: 'Tab' }); + fireEvent.keyUp(picker, { key: 'Tab' }); + + expect(onChange).not.toHaveBeenCalled(); + expect(onChangeComplete).not.toHaveBeenCalled(); + }); + + it('Should increase saturation with the Right arrow on the picker', () => { + render(); + + const picker = screen.getByLabelText('Color picker'); + fireEvent.keyDown(picker, { key: 'ArrowRight' }); + + // saturation steps 91% -> 92% + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 92%, 100%)', + ); + }); + + it('Should step a 1-D slider with the Up/Down arrows', () => { + render(); + + const hue = screen.getByLabelText('Hue'); + // No vertical axis on a slider, so Up/Down drive its single (hue) axis. + fireEvent.keyDown(hue, { key: 'ArrowUp' }); + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(216, 91%, 100%)', + ); + + fireEvent.keyDown(hue, { key: 'ArrowDown' }); + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 91%, 100%)', + ); + }); }); }); From c500853d581a121f2c5c65ed244687584f690ce9 Mon Sep 17 00:00:00 2001 From: Pareder Date: Mon, 27 Jul 2026 11:17:44 +0300 Subject: [PATCH 3/3] Fix after review --- src/components/Handler.tsx | 143 +++++++++++++---- src/components/Picker.tsx | 32 +++- tests/__snapshots__/index.test.tsx.snap | 85 +++++++++- tests/index.test.tsx | 203 +++++++++++++++++++----- 4 files changed, 383 insertions(+), 80 deletions(-) diff --git a/src/components/Handler.tsx b/src/components/Handler.tsx index 98d454f2..a4810fed 100644 --- a/src/components/Handler.tsx +++ b/src/components/Handler.tsx @@ -16,8 +16,6 @@ const VALUE_KEYS = [ 'PageDown', ]; -const isVerticalKey = (key: string) => key === 'ArrowUp' || key === 'ArrowDown'; - // The range input is a keyboard / screen-reader proxy only — the visible thumb // is the wrapping
. It must stay focusable and in the a11y tree, so it is // hidden with `opacity` (not `display`/`visibility`). These styles are inlined @@ -52,6 +50,9 @@ export interface HandlerProps { y?: HandlerAxis; } +type ValueRef = React.RefObject; +type ChangedRef = React.RefObject; + const Handler: React.FC = ({ size = 'default', color, @@ -60,48 +61,98 @@ const Handler: React.FC = ({ x, y, }) => { + const is2D = !!y; + + // Per-axis interaction state. Each input owns the value it is adjusting so a + // controlled parent re-rendering with a stale value mid-interaction cannot + // reset it. The `y` refs are inert for 1-D sliders. const xValueRef = React.useRef(x.value); - xValueRef.current = x.value; - const yValueRef = React.useRef(y?.value); - yValueRef.current = y?.value; + const xPrevRef = React.useRef(x.value); + const xChangedRef = React.useRef(false); + const xInputRef = React.useRef(null); + + const yValueRef = React.useRef(y?.value ?? 0); + const yPrevRef = React.useRef(y?.value ?? 0); + const yChangedRef = React.useRef(false); + const yInputRef = React.useRef(null); + + // Roving tab index: the 2-D picker is a single tab stop whose focus moves + // between the two axis inputs as the user switches direction, so it reads as + // one control while each axis stays independently operable by AT. + const [activeAxis, setActiveAxis] = React.useState<'x' | 'y'>('x'); + + // Preserve the in-progress value across re-renders; only resync when the + // controlled prop *genuinely* changes, so a stale echo of the pre-interaction + // value cannot clobber what the user is currently adjusting. + if (x.value !== xPrevRef.current) { + xPrevRef.current = x.value; + xValueRef.current = x.value; + } + if (y && y.value !== yPrevRef.current) { + yPrevRef.current = y.value; + yValueRef.current = y.value; + } const stepAxis = ( axis: HandlerAxis, - ref: React.MutableRefObject, + valueRef: ValueRef, + changedRef: ChangedRef, direction: 1 | -1, ) => { const stepSize = Number(axis.step ?? 1) || 1; const min = Number(axis.min ?? 0); const max = Number(axis.max ?? 100); - const current = ref.current ?? axis.value; + const current = valueRef.current; const next = Math.min(max, Math.max(min, current + direction * stepSize)); - ref.current = next; + // Clamped against a bound — nothing changed, so stay silent like a native + // range instead of emitting a redundant onChange. + if (next === current) { + return; + } + valueRef.current = next; + changedRef.current = true; axis.onChange(next); }; - // Left/Right drives the horizontal axis; Up/Down the vertical one (or the - // horizontal one for 1-D sliders). We handle these instead of the native range - // so behaviour is deterministic across browsers and safe under rapid presses. + // Move DOM focus (and the roving tab stop) onto the axis being adjusted so + // the screen reader tracks and announces the value that actually changed. + const focusAxis = (axis: 'x' | 'y') => { + setActiveAxis(axis); + const input = axis === 'y' ? yInputRef.current : xInputRef.current; + if (input && document.activeElement !== input) { + input.focus(); + } + }; + + // Left/Right always drives the x axis, Up/Down the y axis (or the single x + // axis on a 1-D slider). preventDefault stops the browser from also moving + // the focused input's native value. const handleKeyDown = (event: React.KeyboardEvent) => { switch (event.key) { case 'ArrowRight': - stepAxis(x, xValueRef, 1); + focusAxis('x'); + stepAxis(x, xValueRef, xChangedRef, 1); break; case 'ArrowLeft': - stepAxis(x, xValueRef, -1); + focusAxis('x'); + stepAxis(x, xValueRef, xChangedRef, -1); break; case 'ArrowUp': if (y) { - stepAxis(y, yValueRef, 1); + focusAxis('y'); + stepAxis(y, yValueRef, yChangedRef, 1); } else { - stepAxis(x, xValueRef, 1); + focusAxis('x'); + stepAxis(x, xValueRef, xChangedRef, 1); } break; case 'ArrowDown': if (y) { - stepAxis(y, yValueRef, -1); + focusAxis('y'); + stepAxis(y, yValueRef, yChangedRef, -1); } else { - stepAxis(x, xValueRef, -1); + focusAxis('x'); + stepAxis(x, xValueRef, xChangedRef, -1); } break; default: @@ -110,16 +161,28 @@ const Handler: React.FC = ({ event.preventDefault(); }; - const handleKeyUp = (event: React.KeyboardEvent) => { - if (!VALUE_KEYS.includes(event.key)) { - return; - } - if (y && isVerticalKey(event.key)) { - y.onChangeComplete(yValueRef.current ?? y.value); - } else { - x.onChangeComplete(xValueRef.current ?? x.value); - } - }; + // Each input completes its own axis. Because focus follows the adjusted axis, + // key up fires on the input whose value just changed. + const completeAxis = + (axis: HandlerAxis, valueRef: ValueRef, changedRef: ChangedRef) => + (event: React.KeyboardEvent) => { + if (!VALUE_KEYS.includes(event.key) || !changedRef.current) { + return; + } + changedRef.current = false; + axis.onChangeComplete(valueRef.current); + }; + + // Native value changes (Home/End/PageUp/PageDown and AT set-value / increment + // actions) feed the same interaction value for the input's own axis. + const changeAxis = + (axis: HandlerAxis, valueRef: ValueRef, changedRef: ChangedRef) => + (event: React.ChangeEvent) => { + const next = Number(event.target.value); + valueRef.current = next; + changedRef.current = true; + axis.onChange(next); + }; return (
= ({ style={{ position: 'relative', backgroundColor: color }} > x.onChange(Number(event.target.value))} + onChange={changeAxis(x, xValueRef, xChangedRef)} onKeyDown={handleKeyDown} - onKeyUp={handleKeyUp} + onKeyUp={completeAxis(x, xValueRef, xChangedRef)} + onFocus={is2D ? () => setActiveAxis('x') : undefined} /> + {y && ( + setActiveAxis('y')} + /> + )}
); }; diff --git a/src/components/Picker.tsx b/src/components/Picker.tsx index 79d4b84a..fa6e64be 100644 --- a/src/components/Picker.tsx +++ b/src/components/Picker.tsx @@ -1,5 +1,6 @@ import type { FC } from 'react'; import React, { useRef } from 'react'; +import type { Color } from '../color'; import useColorDrag from '../hooks/useColorDrag'; import type { BaseColorPickerProps, TransformOffset } from '../interface'; import { calcOffset, calculateColor, generateColor } from '../util'; @@ -11,6 +12,13 @@ import Transform from './Transform'; export type PickerProps = BaseColorPickerProps; +// A stable string identity for a color, used to tell a genuinely new controlled +// value apart from a stale echo of the same value. +const getColorKey = (color: Color) => { + const { h, s, b, a } = color.toHsb(); + return `${h},${s},${b},${a}`; +}; + const Picker: FC = ({ color, onChange, @@ -21,11 +29,20 @@ const Picker: FC = ({ }) => { const pickerRef = useRef(null); const transformRef = useRef(null); + // Candidate color for the active keyboard/drag interaction. Consulted on + // completion so the *latest* value is reported, even across several presses. const colorRef = useRef(color); - // Keep the ref synced with the controlled color so the keyboard handlers below - // always read the latest value — even when several presses (across both axes) - // fire before the parent re-renders, so one axis can't overwrite the other. - colorRef.current = color; + // Key of the controlled color seen on the previous render. Used to accept a + // genuinely new controlled color while ignoring a stale echo of the + // pre-interaction color that a controlled parent may re-render with before + // key up (validation, debouncing, an unrelated state update). Overwriting the + // ref with that stale value would make completion report the old color. + const prevColorKeyRef = useRef(getColorKey(color)); + const nextColorKey = getColorKey(color); + if (nextColorKey !== prevColorKeyRef.current) { + prevColorKeyRef.current = nextColorKey; + colorRef.current = color; + } const onDragChange = useEvent((offsetValue: TransformOffset) => { const calcColor = calculateColor({ @@ -76,9 +93,7 @@ const Picker: FC = ({ x={{ 'aria-label': locale.picker, 'aria-roledescription': locale.pickerDescription, - 'aria-valuetext': `${locale.saturation} ${Math.round( - hsb.s * 100, - )}%, ${locale.brightness} ${Math.round(hsb.b * 100)}%`, + 'aria-valuetext': `${locale.saturation}: ${Math.round(hsb.s * 100)}%`, min: 0, max: 100, value: hsb.s * 100, @@ -86,6 +101,9 @@ const Picker: FC = ({ onChangeComplete: () => onChangeComplete?.(colorRef.current), }} y={{ + 'aria-label': locale.picker, + 'aria-roledescription': locale.pickerDescription, + 'aria-valuetext': `${locale.brightness}: ${Math.round(hsb.b * 100)}%`, min: 0, max: 100, value: hsb.b * 100, diff --git a/tests/__snapshots__/index.test.tsx.snap b/tests/__snapshots__/index.test.tsx.snap index 17486e83..b8fd43be 100644 --- a/tests/__snapshots__/index.test.tsx.snap +++ b/tests/__snapshots__/index.test.tsx.snap @@ -22,15 +22,30 @@ exports[`ColorPicker > Should component onChange work on no control mode 1`] = ` +
Should component render correct 1`] = ` +
Should custom panel work 1`] = ` +
Should disabled alpha work 1`] = ` +
Should prefixCls work 1`] = ` +
{ ); }; + // The two picker axes share the control name (aria-label); each input is + // distinguished by its axis via aria-valuetext (the react-aria ColorArea + // pattern), so the tests grab them positionally: [saturation, brightness]. + const getSaturation = () => screen.getAllByLabelText('Color picker')[0]; + const getBrightness = () => screen.getAllByLabelText('Color picker')[1]; + it('Should expose default aria-labels on the handles', () => { render(); - expect(screen.getByLabelText('Color picker')).toBeTruthy(); + // Both picker axes share the "Color picker" name; the axis is conveyed + // through aria-valuetext rather than a distinct label. + expect(screen.getAllByLabelText('Color picker')).toHaveLength(2); expect(screen.getByLabelText('Hue')).toBeTruthy(); expect(screen.getByLabelText('Alpha')).toBeTruthy(); }); + it('Should expose saturation and brightness as separate range inputs', () => { + render(); + + // Two separately operable native ranges, each describing its own axis via + // aria-valuetext and sharing the 2-D slider role description, so AT can + // adjust each one on its own. + const [saturation, brightness] = screen.getAllByLabelText('Color picker'); + expect(saturation).toHaveAttribute('aria-valuetext', 'Saturation: 91%'); + expect(saturation).toHaveAttribute('aria-roledescription', '2D slider'); + expect(brightness).toHaveAttribute('aria-valuetext', 'Brightness: 100%'); + expect(brightness).toHaveAttribute('aria-roledescription', '2D slider'); + expect(brightness).toHaveAttribute('aria-orientation', 'vertical'); + }); + + it('Should adjust brightness with Up/Down while on the saturation axis', () => { + render(); + + const saturation = getSaturation(); + const brightness = getBrightness(); + saturation.focus(); + + // The two inputs act as one 2-D control: Up/Down drives brightness even + // while the saturation input is the focused one. + fireEvent.keyDown(saturation, { key: 'ArrowDown' }); + + expect(document.querySelector('.pick-color').innerHTML).toBe( + 'hsb(215, 91%, 99%)', + ); + // Focus follows the axis that changed. + expect(brightness).toHaveFocus(); + }); + + it('Should keep the 2-D picker a single tab stop (roving tabindex)', () => { + render(); + + const saturation = getSaturation(); + const brightness = getBrightness(); + + // Only one axis is in the tab order at a time. + expect(saturation).toHaveAttribute('tabindex', '0'); + expect(brightness).toHaveAttribute('tabindex', '-1'); + + // Switching direction moves the tab stop onto the adjusted axis. + fireEvent.keyDown(saturation, { key: 'ArrowDown' }); + expect(brightness).toHaveAttribute('tabindex', '0'); + expect(saturation).toHaveAttribute('tabindex', '-1'); + }); + it('Should describe saturation & brightness via aria-valuetext by default', () => { render(); - expect(screen.getByLabelText('Color picker')).toHaveAttribute( + expect(getSaturation()).toHaveAttribute( + 'aria-valuetext', + 'Saturation: 91%', + ); + expect(getBrightness()).toHaveAttribute( 'aria-valuetext', - 'Saturation 91%, Brightness 100%', + 'Brightness: 100%', ); }); @@ -551,22 +611,21 @@ describe('ColorPicker', () => { />, ); - expect(screen.getByLabelText('Sélecteur')).toBeTruthy(); + const [saturation, brightness] = screen.getAllByLabelText('Sélecteur'); + expect(screen.getAllByLabelText('Sélecteur')).toHaveLength(2); expect(screen.getByLabelText('Teinte')).toBeTruthy(); expect(screen.getByLabelText('Transparence')).toBeTruthy(); - expect(screen.getByLabelText('Sélecteur')).toHaveAttribute( - 'aria-valuetext', - 'Sat 91%, Lum 100%', - ); + expect(saturation).toHaveAttribute('aria-valuetext', 'Sat: 91%'); + expect(brightness).toHaveAttribute('aria-valuetext', 'Lum: 100%'); }); - it('Should change brightness with the Down arrow on the picker handle', () => { + it('Should change brightness with the Down arrow on the brightness axis', () => { const onChangeComplete = vi.fn(); render(); - const picker = screen.getByLabelText('Color picker'); - fireEvent.keyDown(picker, { key: 'ArrowDown' }); - fireEvent.keyUp(picker, { key: 'ArrowDown' }); + const brightness = getBrightness(); + fireEvent.keyDown(brightness, { key: 'ArrowDown' }); + fireEvent.keyUp(brightness, { key: 'ArrowDown' }); // brightness starts at 100% and steps down to 99% expect(document.querySelector('.pick-color').innerHTML).toBe( @@ -575,24 +634,50 @@ describe('ColorPicker', () => { expect(onChangeComplete).toHaveBeenCalled(); }); - it('Should clamp brightness at 100% when pressing the Up arrow', () => { - render(); + it('Should operate brightness through its native range control', () => { + const onChangeComplete = vi.fn(); + render(); - const picker = screen.getByLabelText('Color picker'); - fireEvent.keyDown(picker, { key: 'ArrowUp' }); + const brightness = getBrightness(); + // Emulate an assistive-technology set-value action on the native range, + // then complete the interaction on key up. + fireEvent.change(brightness, { target: { value: '80' } }); + fireEvent.keyUp(brightness, { key: 'ArrowDown' }); expect(document.querySelector('.pick-color').innerHTML).toBe( - 'hsb(215, 91%, 100%)', + 'hsb(215, 91%, 80%)', + ); + expect(onChangeComplete).toHaveBeenCalled(); + }); + + it('Should not emit changes when the arrow key clamps at a bound', () => { + const onChange = vi.fn(); + const onChangeComplete = vi.fn(); + render( + , ); + + const brightness = getBrightness(); + // brightness is already at 100%, so Up cannot step further. + fireEvent.keyDown(brightness, { key: 'ArrowUp' }); + fireEvent.keyUp(brightness, { key: 'ArrowUp' }); + + // A clamped press must not fire onChange (no value moved) nor complete. + expect(onChange).not.toHaveBeenCalled(); + expect(onChangeComplete).not.toHaveBeenCalled(); }); it('Should increase saturation on the picker (Arrow Right)', () => { const onChangeComplete = vi.fn(); render(); - const picker = screen.getByLabelText('Color picker'); - fireEvent.change(picker, { target: { value: '92' } }); - fireEvent.keyUp(picker, { key: 'ArrowRight' }); + const saturation = getSaturation(); + fireEvent.change(saturation, { target: { value: '92' } }); + fireEvent.keyUp(saturation, { key: 'ArrowRight' }); // saturation starts at 91% and steps up to 92% expect(document.querySelector('.pick-color').innerHTML).toBe( @@ -605,9 +690,9 @@ describe('ColorPicker', () => { const onChangeComplete = vi.fn(); render(); - const picker = screen.getByLabelText('Color picker'); - fireEvent.change(picker, { target: { value: '90' } }); - fireEvent.keyUp(picker, { key: 'ArrowLeft' }); + const saturation = getSaturation(); + fireEvent.change(saturation, { target: { value: '90' } }); + fireEvent.keyUp(saturation, { key: 'ArrowLeft' }); // saturation starts at 91% and steps down to 90% expect(document.querySelector('.pick-color').innerHTML).toBe( @@ -616,19 +701,60 @@ describe('ColorPicker', () => { expect(onChangeComplete).toHaveBeenCalled(); }); + // A controlled parent that keeps the color pinned while still re-rendering + // (validation, debouncing, an unrelated state update) reproduces the race + // where the stale prop echoes back mid-interaction. + const StalePinned = ({ onChangeComplete }: Record) => { + const [, force] = useState(0); + return ( + force(n => n + 1)} + onChangeComplete={onChangeComplete as (color: Color) => void} + /> + ); + }; + + it('Should complete the picker with the latest value despite a stale re-render', () => { + const onChangeComplete = vi.fn(); + render(); + + const saturation = getSaturation(); + // ArrowRight steps 91% -> 92% and triggers a re-render that echoes the + // stale 91% prop before key up. Completion must still report 92%. + fireEvent.keyDown(saturation, { key: 'ArrowRight' }); + fireEvent.keyUp(saturation, { key: 'ArrowRight' }); + + const [completedColor] = onChangeComplete.mock.calls.at(-1); + expect(completedColor.toHsbString()).toBe('hsb(215, 92%, 100%)'); + }); + + it('Should complete a slider with the latest value despite a stale re-render', () => { + const onChangeComplete = vi.fn(); + render(); + + const hue = screen.getByLabelText('Hue'); + // ArrowRight steps hue 215 -> 216 while the prop is pinned at 215. + fireEvent.keyDown(hue, { key: 'ArrowRight' }); + fireEvent.keyUp(hue, { key: 'ArrowRight' }); + + const [completedColor] = onChangeComplete.mock.calls.at(-1); + expect(completedColor.getHue()).toBe(216); + }); + it('Should step from the latest value on rapid saturation presses', () => { render(); - const picker = screen.getByLabelText('Color picker') as HTMLInputElement; + const saturation = getSaturation(); // Two Left presses dispatched in the same batch (before the parent // re-renders). Reading the stale prop would step 91% -> 90% twice; the // latest-value ref keeps them stepping 91% -> 89%. act(() => { - picker.dispatchEvent( + saturation.dispatchEvent( new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true }), ); - picker.dispatchEvent( + saturation.dispatchEvent( new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true }), ); }); @@ -641,16 +767,16 @@ describe('ColorPicker', () => { it('Should step from the latest value on rapid brightness presses', () => { render(); - const picker = screen.getByLabelText('Color picker') as HTMLInputElement; + const brightness = getBrightness(); // Two Down presses dispatched in the same batch (before the parent // re-renders). Reading the stale prop would step 100% -> 99% twice; the // latest-value ref keeps them stepping 100% -> 98%. act(() => { - picker.dispatchEvent( + brightness.dispatchEvent( new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), ); - picker.dispatchEvent( + brightness.dispatchEvent( new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), ); }); @@ -663,16 +789,17 @@ describe('ColorPicker', () => { it('Should step from the latest value on rapid saturation and brightness presses', () => { render(); - const picker = screen.getByLabelText('Color picker') as HTMLInputElement; + const saturation = getSaturation(); + const brightness = getBrightness(); // One Down (brightness 100% -> 99%) and one Left (saturation 91% -> 90%) // dispatched in the same batch. Deriving the second change from the stale - // prop would revert the first axis; the refs keep both. + // prop would revert the first axis; the shared color ref keeps both. act(() => { - picker.dispatchEvent( + brightness.dispatchEvent( new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }), ); - picker.dispatchEvent( + saturation.dispatchEvent( new KeyboardEvent('keydown', { key: 'ArrowLeft', bubbles: true }), ); }); @@ -718,10 +845,10 @@ describe('ColorPicker', () => { />, ); - const picker = screen.getByLabelText('Color picker'); + const saturation = getSaturation(); // A non-value key (e.g. Tab) must neither step (keydown) nor commit (keyup). - fireEvent.keyDown(picker, { key: 'Tab' }); - fireEvent.keyUp(picker, { key: 'Tab' }); + fireEvent.keyDown(saturation, { key: 'Tab' }); + fireEvent.keyUp(saturation, { key: 'Tab' }); expect(onChange).not.toHaveBeenCalled(); expect(onChangeComplete).not.toHaveBeenCalled(); @@ -730,8 +857,8 @@ describe('ColorPicker', () => { it('Should increase saturation with the Right arrow on the picker', () => { render(); - const picker = screen.getByLabelText('Color picker'); - fireEvent.keyDown(picker, { key: 'ArrowRight' }); + const saturation = getSaturation(); + fireEvent.keyDown(saturation, { key: 'ArrowRight' }); // saturation steps 91% -> 92% expect(document.querySelector('.pick-color').innerHTML).toBe(