fix: refactor clear icon to button element to make it accessible#1224
fix: refactor clear icon to button element to make it accessible#1224Pareder wants to merge 1 commit into
Conversation
|
@Pareder is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
概览PR 为 Select 组件的清除按钮添加可访问性标签支持，扩展 变更内容Clear 按钮可访问性标签支持
可能相关的 PR
建议的审查人
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
src/BaseSelect/index.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/SelectInput/index.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/hooks/useAllowClear.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1224 +/- ##
=======================================
Coverage 99.44% 99.44%
=======================================
Files 31 31
Lines 1266 1267 +1
Branches 442 465 +23
=======================================
+ Hits 1259 1260 +1
Misses 7 7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 improves the accessibility and keyboard navigation of the clear button in the Select component by changing its markup from a div to a native button element and adding support for an accessible label. Feedback on these changes suggests adding a default fallback for the label to prevent type contract violations and ensure accessibility. Additionally, minor cleanups are recommended in the tests to remove an unused mock and simplify click event dispatching.
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.
| return { | ||
| allowClear: mergedAllowClear, | ||
| clearIcon: mergedAllowClear ? allowClearConfig.clearIcon || clearIcon || '×' : null, | ||
| label: mergedAllowClear ? allowClearConfig.label : '', |
There was a problem hiding this comment.
The AllowClearConfig interface defines label as a required string. However, when allowClear is passed as true or as an object without a label property, allowClearConfig.label will be undefined. Returning allowClearConfig.label directly violates the type contract and leaves the clear button without an accessible name for screen readers.\n\nProviding a default fallback like 'clear' resolves the type mismatch and ensures the button remains accessible by default.
| label: mergedAllowClear ? allowClearConfig.label : '', | |
| label: mergedAllowClear ? allowClearConfig.label || 'clear' : '', |
| const clickPreventDefault = jest.fn(); | ||
| const { container } = render( |
There was a problem hiding this comment.
The clickPreventDefault mock is unused because the component does not call preventDefault() on the click event (it only calls it on the mousedown event). Removing this definition avoids unused variable warnings and simplifies the test setup.
const { container } = render(
| const clickEvent = createEvent.click(container.querySelector('.rc-select-clear')); | ||
| clickEvent.preventDefault = clickPreventDefault; | ||
| fireEvent(container.querySelector('.rc-select-clear'), clickEvent); |
There was a problem hiding this comment.
Since we don't need to mock preventDefault on the click event, we can simplify the event dispatching by directly calling fireEvent.click.
| const clickEvent = createEvent.click(container.querySelector('.rc-select-clear')); | |
| clickEvent.preventDefault = clickPreventDefault; | |
| fireEvent(container.querySelector('.rc-select-clear'), clickEvent); | |
| fireEvent.click(container.querySelector('.rc-select-clear')); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/hooks/useAllowClear.tsx`:
- Line 41: The clear button label can be undefined when callers pass allowClear
or clearIcon without allowClear.label; update useAllowClear to provide a
non-empty fallback string for the button label (e.g. a default like "Clear" or
an i18n key) instead of passing allowClearConfig.label directly—locate the
mergedAllowClear construction and the place where label: mergedAllowClear ?
allowClearConfig.label : '' is set and replace it so label is always a safe,
non-empty string (refer to mergedAllowClear and allowClearConfig.label in
useAllowClear).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e63c184-4f12-49e8-b5e1-a7e46891d276
⛔ Files ignored due to path filters (1)
tests/__snapshots__/Select.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/BaseSelect/index.tsxsrc/SelectInput/index.tsxsrc/hooks/useAllowClear.tsxtests/Select.test.tsxtests/shared/allowClearTest.tsx
| return { | ||
| allowClear: mergedAllowClear, | ||
| clearIcon: mergedAllowClear ? allowClearConfig.clearIcon || clearIcon || '×' : null, | ||
| label: mergedAllowClear ? allowClearConfig.label : '', |
There was a problem hiding this comment.
清除按钮在未传 allowClear.label 时可能缺少可访问名称
这里直接透传 allowClearConfig.label，当调用方只写 allowClear 或只传 clearIcon 时会是 undefined，下游 button 可能变成无可访问名称（尤其是纯图标 clearIcon）。建议提供非空兜底文案（可后续接入 i18n 文案源）。
💡 建议修复
return useMemo(() => {
const mergedAllowClear =
!disabled &&
allowClearConfig.allowClear !== false &&
(displayValues.length || mergedSearchValue) &&
!(mode === 'combobox' && mergedSearchValue === '');
return {
allowClear: mergedAllowClear,
clearIcon: mergedAllowClear ? allowClearConfig.clearIcon || clearIcon || '×' : null,
- label: mergedAllowClear ? allowClearConfig.label : '',
+ label: mergedAllowClear ? allowClearConfig.label ?? 'Clear' : '',
};
}, [allowClearConfig, clearIcon, disabled, displayValues.length, mergedSearchValue, mode]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| label: mergedAllowClear ? allowClearConfig.label : '', | |
| label: mergedAllowClear ? allowClearConfig.label ?? 'Clear' : '', |
🤖 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/hooks/useAllowClear.tsx` at line 41, The clear button label can be
undefined when callers pass allowClear or clearIcon without allowClear.label;
update useAllowClear to provide a non-empty fallback string for the button label
(e.g. a default like "Clear" or an i18n key) instead of passing
allowClearConfig.label directly—locate the mergedAllowClear construction and the
place where label: mergedAllowClear ? allowClearConfig.label : '' is set and
replace it so label is always a safe, non-empty string (refer to
mergedAllowClear and allowClearConfig.label in useAllowClear).
1 participant
This PR improves clear button functionality by:
divto navitebuttonelementaria-labelattribute by extendingallowClearobject typeSummary by CodeRabbit
发行说明
新功能
button元素，支持键盘操作激活。改进
aria-label属性。