feat: Improve keyboard & screen-reader accessibility#286
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughColorPicker 增加了基于 locale 的无障碍文案、Handler 的隐藏 range 键盘控制、焦点可见样式，以及覆盖这些交互与属性的测试。 ChangesKeyboard Navigation & Accessibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the accessibility of the ColorPicker component by introducing keyboard navigation (2D slider handler) for saturation and brightness, adding customizable locale support for screen reader labels, and updating components like Handler, Picker, and Slider to support range inputs and ARIA attributes. Feedback was provided to address potential race conditions during rapid keyboard interactions in controlled mode by using mutable refs to track the latest color and vertical axis values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // ===================== 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); | ||
| }; |
There was a problem hiding this comment.
In a controlled component, rapid keyboard interactions (like pressing or holding arrow keys) can trigger multiple state updates before the parent component re-renders and passes down the updated color prop. Because changeColor derives the next color from the potentially stale color prop, rapid changes to one channel (e.g., saturation) can revert changes made to the other channel (e.g., brightness) in rapid succession.
To prevent this race condition, we should keep colorRef.current in sync with the color prop during render, and derive the next color from colorRef.current instead of the color prop.
| // ===================== 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); | |
| }; | |
| // ===================== Keyboard (2-D handler) ===================== | |
| colorRef.current = color; | |
| // Build a new color from the current one with a single HSB channel changed. | |
| const changeColor = (channel: 's' | 'b', percent: number) => { | |
| const currentHsb = colorRef.current.toHsb(); | |
| const next = generateColor({ ...currentHsb, [channel]: percent / 100 }); | |
| colorRef.current = next; | |
| onChange(next); | |
| }; |
| const Handler: React.FC<HandlerProps> = ({ | ||
| 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<HTMLInputElement>) => { | ||
| 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 ( | ||
| <div | ||
| className={clsx(`${prefixCls}-handler`, { | ||
| [`${prefixCls}-handler-sm`]: size === 'small', | ||
| })} | ||
| style={{ backgroundColor: color }} | ||
| /> | ||
| style={{ position: 'relative', backgroundColor: color }} | ||
| > | ||
| <input | ||
| step={1} | ||
| {...omit(x, ['onChangeComplete'])} | ||
| type="range" | ||
| className={`${prefixCls}-handler-range`} | ||
| 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)); | ||
| } | ||
| }} | ||
| /> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Since the vertical axis (y) is not a native range input, its value changes are handled entirely in JavaScript via onKeyDown. In controlled mode, rapid key presses or holding down the arrow keys can cause handleKeyDown and onKeyUp to read stale y.value props before the parent component has committed the state update and re-rendered.
To ensure smooth and correct keyboard navigation for the vertical axis, we should track the latest value in a mutable ref (lastValueRef) and update it synchronously on each key down/up event.
const Handler: React.FC<HandlerProps> = ({
size = 'default',
color,
prefixCls,
disabled,
x,
y,
}) => {
const lastValueRef = React.useRef(y?.value);
if (y && y.value !== lastValueRef.current) {
lastValueRef.current = y.value;
}
// 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<HTMLInputElement>) => {
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;
const nextValue = Math.min(max, Math.max(min, (lastValueRef.current ?? y.value) + delta));
lastValueRef.current = nextValue;
y.onChange(nextValue);
};
return (
<div
className={clsx(prefixCls + "-handler", {
[prefixCls + "-handler-sm"]: size === "small",
})}
style={{ position: 'relative', backgroundColor: color }}
>
<input
step={1}
{...omit(x, ['onChangeComplete'])}
type="range"
className={prefixCls + "-handler-range"}
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(lastValueRef.current ?? y.value);
} else {
x.onChangeComplete(Number(event.currentTarget.value));
}
}}
/>
</div>
);
};
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Picker.tsx (1)
20-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick win请为
locale增加本地兜底，避免可选值被直接解引用导致崩溃Line 20 的
locale是可选，但 Line 71-75 直接读取locale.picker/locale.saturation等；一旦该层收到undefined，会触发运行时异常并中断交互。建议修复（示例）
const Picker: FC<PickerProps> = ({ color, onChange, prefixCls, onChangeComplete, disabled, locale, }) => { + const pickerLabel = locale?.picker ?? 'Color picker'; + const pickerDescription = locale?.pickerDescription; + const saturationLabel = locale?.saturation ?? 'Saturation'; + const brightnessLabel = locale?.brightness ?? 'Brightness'; + const pickerRef = useRef(); const transformRef = useRef(); const colorRef = useRef(color); @@ x={{ - 'aria-label': locale.picker, - 'aria-roledescription': locale.pickerDescription, - 'aria-valuetext': `${locale.saturation} ${Math.round( + 'aria-label': pickerLabel, + 'aria-roledescription': pickerDescription, + 'aria-valuetext': `${saturationLabel} ${Math.round( hsb.s * 100, - )}%, ${locale.brightness} ${Math.round(hsb.b * 100)}%`, + )}%, ${brightnessLabel} ${Math.round(hsb.b * 100)}%`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Picker.tsx` around lines 20 - 75, The Picker component reads optional prop locale directly (used in Handler aria attributes like locale.picker, locale.saturation, locale.brightness) which can be undefined and cause runtime crashes; fix by providing a safe default fallback for locale at the top of the component (e.g., const safeLocale = locale ?? DEFAULT_LOCALE) or via destructuring with defaults, then replace direct uses of locale.* with safeLocale.* (ensure DEFAULT_LOCALE is defined or imported) so aria labels always have values.
🧹 Nitpick comments (2)
tests/index.test.tsx (1)
633-642: ⚡ Quick win建议添加
onChangeComplete回调验证以保持测试一致性。前面的键盘导航测试（色调、饱和度、亮度）都会验证
onChangeComplete是否被调用，但此测试仅检查透明度值的变化，缺少对onChangeComplete的验证。为了保持测试套件的一致性并确保透明度滑块的
onChangeComplete行为得到覆盖，建议添加相应的断言。🧪 建议的测试增强
it('Should change alpha when the alpha slider value changes via keyboard', () => { - render(<Controlled />); + const onChangeComplete = vi.fn(); + render(<Controlled onChangeComplete={onChangeComplete} />); const alpha = screen.getByLabelText('Alpha'); fireEvent.change(alpha, { target: { value: '50' } }); + fireEvent.keyUp(alpha, { key: 'ArrowRight' }); expect(document.querySelector('.pick-color').innerHTML).toBe( 'hsba(215, 91%, 100%, 0.50)', ); + expect(onChangeComplete).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/index.test.tsx` around lines 633 - 642, The alpha-slider test ("Should change alpha when the alpha slider value changes via keyboard") currently only asserts the DOM color change; add a mock for onChangeComplete and pass it into the Controlled test component (Controlled) so you can assert it was called when the alpha input is changed; specifically, create a jest.fn() for onChangeComplete, render <Controlled onChangeComplete={onChangeCompleteMock} />, fireEvent.change on the element grabbed by screen.getByLabelText('Alpha'), then assert onChangeCompleteMock was called (and optionally called with the expected alpha/hsba payload) in addition to the existing document.querySelector('.pick-color').innerHTML check.assets/index.less (1)
58-62: 💤 Low value建议验证焦点轮廓的对比度。
当前焦点样式使用
outline: 2px solid#1677ff。虽然 `outline-offset: 1px` 和处理器的白色边框（`border: 2px solid `#fff）为轮廓提供了一定的背景对比，但在某些极浅或白色的色调下，蓝色轮廓的可见性可能仍然不足。考虑添加一个双层轮廓（例如使用
box-shadow添加外层深色描边）以确保在所有颜色背景下都有足够的对比度。♻️ 建议的增强方案
&:focus-visible, &:focus-within { outline: 2px solid `#1677ff`; outline-offset: 1px; + box-shadow: 0 0 0 4px rgba(255, 255, 255, 0.8); }这会在蓝色轮廓外添加一个半透明白色光晕，确保在深色和浅色背景下都有良好的可见性。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/index.less` around lines 58 - 62, The focus outline (selectors &:focus-visible and &:focus-within) uses only outline: 2px solid `#1677ff` which may lack contrast on some backgrounds; update these selectors to add a double-layer visual (keep the existing blue outline but add an outer ring via box-shadow or an extra outline) so the focus is always visible—for example, add a box-shadow that renders a thin semi-opaque white or dark halo outside the blue outline (e.g., an outer 2–3px ring using rgba(...) for light/dark backgrounds) and keep outline-offset as needed; modify the &:focus-visible and &:focus-within block to include this additional visual ring while preserving the existing outline and offset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/Picker.tsx`:
- Around line 20-75: The Picker component reads optional prop locale directly
(used in Handler aria attributes like locale.picker, locale.saturation,
locale.brightness) which can be undefined and cause runtime crashes; fix by
providing a safe default fallback for locale at the top of the component (e.g.,
const safeLocale = locale ?? DEFAULT_LOCALE) or via destructuring with defaults,
then replace direct uses of locale.* with safeLocale.* (ensure DEFAULT_LOCALE is
defined or imported) so aria labels always have values.
---
Nitpick comments:
In `@assets/index.less`:
- Around line 58-62: The focus outline (selectors &:focus-visible and
&:focus-within) uses only outline: 2px solid `#1677ff` which may lack contrast on
some backgrounds; update these selectors to add a double-layer visual (keep the
existing blue outline but add an outer ring via box-shadow or an extra outline)
so the focus is always visible—for example, add a box-shadow that renders a thin
semi-opaque white or dark halo outside the blue outline (e.g., an outer 2–3px
ring using rgba(...) for light/dark backgrounds) and keep outline-offset as
needed; modify the &:focus-visible and &:focus-within block to include this
additional visual ring while preserving the existing outline and offset.
In `@tests/index.test.tsx`:
- Around line 633-642: The alpha-slider test ("Should change alpha when the
alpha slider value changes via keyboard") currently only asserts the DOM color
change; add a mock for onChangeComplete and pass it into the Controlled test
component (Controlled) so you can assert it was called when the alpha input is
changed; specifically, create a jest.fn() for onChangeComplete, render
<Controlled onChangeComplete={onChangeCompleteMock} />, fireEvent.change on the
element grabbed by screen.getByLabelText('Alpha'), then assert
onChangeCompleteMock was called (and optionally called with the expected
alpha/hsba payload) in addition to the existing
document.querySelector('.pick-color').innerHTML check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8732dbee-7a87-4d9f-9eb1-9ef62c29c1b9
⛔ Files ignored due to path filters (1)
tests/__snapshots__/index.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
assets/index.lesssrc/ColorPicker.tsxsrc/components/ColorBlock.tsxsrc/components/Handler.tsxsrc/components/Picker.tsxsrc/components/Slider.tsxsrc/interface.tstests/index.test.tsx
|
@afc163 @zombieJ @meet-student @QDyanbing Could you please review? |
2 similar comments
|
@afc163 @zombieJ @meet-student @QDyanbing Could you please review? |
|
@afc163 @zombieJ @meet-student @QDyanbing Could you please review? |
|
@afc163 Could you please take a look? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #286 +/- ##
===========================================
- Coverage 100.00% 99.62% -0.38%
===========================================
Files 15 15
Lines 902 1056 +154
Branches 89 108 +19
===========================================
+ Hits 902 1052 +150
- Misses 0 4 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
@afc163 Done. |
|
Thanks for the PR. The overall direction makes sense, but there are a few correctness issues that should be fixed before this is mergeable:
After those are fixed, please add/adjust tests for the optional-locale case and rapid keyboard updates in controlled mode. Codecov is currently failing as well, so the added tests should help there. |
2 participants
Summary
Makes the color picker fully operable by keyboard and meaningful to assistive technology. The saturation/brightness panel and the hue/alpha sliders previously responded to mouse/touch only; they now expose native slider semantics, full arrow-key control, focus indication, and localizable accessible names.
Motivation
The handles were non-focusable
<div>s with no role or value, so keyboard and screen-reader users could not read or change the color. This addresses WCAG 2.1.1 (Keyboard), 2.4.7 (Focus Visible), and 4.1.2 (Name, Role, Value).What changed
<input type="range">(Handler.tsx). This gives us, for free, thesliderrole,aria-valuemin/max/now, and native keyboard handling (arrows, Home/End, PageUp/PageDown).onKeyDownto drive the vertical axis (brightness), since a native range is 1-D. Result: one focusable thumb where ←/→ = saturation and ↑/↓ = brightness.opacity: 0; pointer-events: none; position: absolute), not the stylesheet, so consumers who don't importassets/index.less(e.g. antd's own styling) get correct rendering with no setup.opacity(notdisplay:none) keeps it focusable and in the a11y tree.:focus-visible/:focus-withinoutline on-handlerso keyboard focus is visible on the thumb.localeprop — all accessible names are localizable via a singlelocaleprop on<ColorPicker>:ColorBlockaccepts standard div attributes —ColorBlockPropsnow extendsReact.HTMLAttributes<HTMLDivElement>and spreads them onto the root. This lets consumers attach interaction/ARIA props (role,tabIndex,aria-*, keyboard handlers) when using it as a clickable trigger, keeping the component itself presentational.API additions (non-breaking)
ColorPicker/BaseColorPickerProps: new optionallocale.Slider(BaseSliderProps): new optionalaria-label.ColorBlock: props widened toHTMLAttributes<HTMLDivElement>(superset of the previousclassName/style/onClick).<input class="{prefix}-handler-range">.Related issues
Fixes ant-design/ant-design#57935.
Summary by CodeRabbit
发布说明