-
-
Notifications
You must be signed in to change notification settings - Fork 484
fix: refactor clear icon to button element to make it accessible #1224
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,12 +5,13 @@ import { useMemo } from 'react'; | |||||
| export interface AllowClearConfig { | ||||||
| allowClear: boolean; | ||||||
| clearIcon: React.ReactNode; | ||||||
| label: string; | ||||||
| } | ||||||
|
|
||||||
| export const useAllowClear = ( | ||||||
| prefixCls: string, | ||||||
| displayValues: DisplayValueType[], | ||||||
| allowClear?: boolean | { clearIcon?: React.ReactNode }, | ||||||
| allowClear?: boolean | { clearIcon?: React.ReactNode; label?: string }, | ||||||
| clearIcon?: React.ReactNode, | ||||||
| disabled: boolean = false, | ||||||
| mergedSearchValue?: string, | ||||||
|
|
@@ -37,6 +38,7 @@ export const useAllowClear = ( | |||||
| return { | ||||||
| allowClear: mergedAllowClear, | ||||||
| clearIcon: mergedAllowClear ? allowClearConfig.clearIcon || clearIcon || '×' : null, | ||||||
| label: mergedAllowClear ? allowClearConfig.label : '', | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 清除按钮在未传 这里直接透传 💡 建议修复 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
Suggested change
🤖 Prompt for AI Agents |
||||||
| }; | ||||||
| }, [allowClearConfig, clearIcon, disabled, displayValues.length, mergedSearchValue, mode]); | ||||||
| }; | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2508,7 +2508,7 @@ describe('Select.Basic', () => { | |||||||||
| it('should be focused when click clear button', () => { | ||||||||||
| jest.useFakeTimers(); | ||||||||||
|
|
||||||||||
| const mouseDownPreventDefault = jest.fn(); | ||||||||||
| const clickPreventDefault = jest.fn(); | ||||||||||
| const { container } = render( | ||||||||||
|
Comment on lines
+2511
to
2512
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
| <Select allowClear value="1"> | ||||||||||
| <Option value="1">1</Option> | ||||||||||
|
|
@@ -2518,15 +2518,71 @@ describe('Select.Basic', () => { | |||||||||
|
|
||||||||||
| expect(container.querySelector('.rc-select-clear')).toBeTruthy(); | ||||||||||
|
|
||||||||||
| const mouseDownEvent = createEvent.mouseDown(container.querySelector('.rc-select-clear')); | ||||||||||
| mouseDownEvent.preventDefault = mouseDownPreventDefault; | ||||||||||
| fireEvent(container.querySelector('.rc-select-clear'), mouseDownEvent); | ||||||||||
| const clickEvent = createEvent.click(container.querySelector('.rc-select-clear')); | ||||||||||
| clickEvent.preventDefault = clickPreventDefault; | ||||||||||
| fireEvent(container.querySelector('.rc-select-clear'), clickEvent); | ||||||||||
|
Comment on lines
+2521
to
+2523
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we don't need to mock
Suggested change
|
||||||||||
| jest.runAllTimers(); | ||||||||||
|
|
||||||||||
| expect(container.querySelector('.rc-select').className).toContain('-focused'); | ||||||||||
| jest.useRealTimers(); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it('renders clear as an accessible button with label', () => { | ||||||||||
| const { container } = render( | ||||||||||
| <Select allowClear={{ label: 'Clear all' }} value="1"> | ||||||||||
| <Option value="1">1</Option> | ||||||||||
| <Option value="2">2</Option> | ||||||||||
| </Select>, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| const clear = container.querySelector('.rc-select-clear'); | ||||||||||
| expect(clear.tagName).toBe('BUTTON'); | ||||||||||
| expect(clear).toHaveAttribute('type', 'button'); | ||||||||||
| expect(clear).toHaveAttribute('aria-label', 'Clear all'); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it('clicking clear does not open the dropdown', () => { | ||||||||||
| const onPopupVisibleChange = jest.fn(); | ||||||||||
| const { container } = render( | ||||||||||
| <Select value="1" allowClear onPopupVisibleChange={onPopupVisibleChange}> | ||||||||||
| <Option value="1">One</Option> | ||||||||||
| <Option value="2">Two</Option> | ||||||||||
| </Select>, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| // mousedown should be prevented (keeps focus on input) and must not open | ||||||||||
| const mouseDownEvent = createEvent.mouseDown(container.querySelector('.rc-select-clear')); | ||||||||||
| fireEvent(container.querySelector('.rc-select-clear'), mouseDownEvent); | ||||||||||
| expect(mouseDownEvent.defaultPrevented).toBe(true); | ||||||||||
|
|
||||||||||
| fireEvent.click(container.querySelector('.rc-select-clear')); | ||||||||||
|
|
||||||||||
| expectOpen(container, false); | ||||||||||
| expect(onPopupVisibleChange).not.toHaveBeenCalledWith(true); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it('clears value via keyboard activation', () => { | ||||||||||
| const onChange = jest.fn(); | ||||||||||
| const onClear = jest.fn(); | ||||||||||
| const { container } = render( | ||||||||||
| <Select defaultValue="1" allowClear onChange={onChange} onClear={onClear}> | ||||||||||
| <Option value="1">1</Option> | ||||||||||
| <Option value="2">2</Option> | ||||||||||
| </Select>, | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| const clear = container.querySelector<HTMLButtonElement>('.rc-select-clear'); | ||||||||||
|
|
||||||||||
| clear.focus(); | ||||||||||
| expect(clear).toHaveFocus(); | ||||||||||
| // Enter/Space on a native button dispatch a click event | ||||||||||
| fireEvent.click(clear); | ||||||||||
|
|
||||||||||
| expect(onChange).toHaveBeenCalledWith(undefined, undefined); | ||||||||||
| expect(onClear).toHaveBeenCalled(); | ||||||||||
| expect(container.querySelector('input').value).toBe(''); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it('should support title', () => { | ||||||||||
| const { container: container1 } = render(<Select defaultValue="lucy" options={[]} />); | ||||||||||
| expect(container1.querySelector('.rc-select').getAttribute('title')).toBeFalsy(); | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
AllowClearConfiginterface defineslabelas a requiredstring. However, whenallowClearis passed astrueor as an object without alabelproperty,allowClearConfig.labelwill beundefined. ReturningallowClearConfig.labeldirectly 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.