fix: refactor clear icon to button element to make it accessible#971
Open
Pareder wants to merge 1 commit into
Open
fix: refactor clear icon to button element to make it accessible#971Pareder wants to merge 1 commit into
Pareder 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. |
1 task
Contributor
概览将 ClearIcon 从 Icon 组件中提取为独立的按钮组件，改进键盘可访问性；同时简化 Icon 组件，并更新所有选择器的导入和用法；最后在清除后恢复输入焦点。 变更ClearIcon 提取与 Icon 重构
代码评审工作量估计🎯 3 (中等) | ⏱️ ~20 分钟 诗
🚥 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/PickerInput/Selector/ClearIcon.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/PickerInput/Selector/Icon.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/PickerInput/Selector/Input.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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request extracts the ClearIcon component into its own file, updates it to render as a native <button> element instead of a <span> with a button role, and ensures that the selector is focused when cleared. The review comments point out critical type safety and runtime stability improvements, such as using optional chaining when calling .focus() on selectorRef.current, extending React.ButtonHTMLAttributes instead of generic HTML attributes for the button-based ClearIconProps, and safely accessing locale.clear using optional chaining.
| const onSelectorClear = () => { | ||
| triggerSubmitChange(null); | ||
| triggerOpen(false, { force: true }); | ||
| selectorRef.current.focus(); |
Contributor
There was a problem hiding this comment.
The selectorRef.current reference can be null or undefined (e.g., during initial render or if unmounted). To prevent potential runtime TypeError crashes, use optional chaining when calling .focus().
Suggested change
| selectorRef.current.focus(); | |
| selectorRef.current?.focus(); |
| import PickerContext from '../context'; | ||
| import { clsx } from 'clsx'; | ||
|
|
||
| export interface ClearIconProps extends React.HtmlHTMLAttributes<HTMLElement> { |
Contributor
There was a problem hiding this comment.
Since the ClearIcon component renders a native <button> element and forwards ...restProps to it, the props interface should extend React.ButtonHTMLAttributes<HTMLButtonElement> instead of React.HtmlHTMLAttributes<HTMLElement>. This provides accurate type safety for button-specific attributes.
Suggested change
| export interface ClearIconProps extends React.HtmlHTMLAttributes<HTMLElement> { | |
| export interface ClearIconProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { |
| <button | ||
| {...restProps} | ||
| type="button" | ||
| aria-label={locale.clear} |
Contributor
There was a problem hiding this comment.
If locale is not defined in the PickerContext (for example, in custom context providers or test environments), accessing locale.clear will throw a runtime error. Using optional chaining prevents this crash.
Suggested change
| aria-label={locale.clear} | |
| aria-label={locale?.clear} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/index.less (1)
411-420:⚠️ Potential issue | 🟠 Major | ⚡ Quick win需要为清除按钮补齐基础 reset 样式
ClearIcon已改为原生button，但当前.rc-picker-clear没有重置按钮默认样式；在部分浏览器会出现默认边框/背景/内边距，导致 UI 回归。建议在该规则内显式重置。建议修改
&-clear { position: absolute; top: 0; inset-inline-end: 0; cursor: pointer; + padding: 0; + border: 0; + background: transparent; + appearance: none; &-btn::after { content: '×'; } }🤖 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 411 - 420, The clear button (ClearIcon) is now a native button but .rc-picker-clear (selector represented by &-clear and its &-btn) doesn't reset browser default button styles; update the &-clear / &-clear-btn rule to explicitly reset styles by setting background: transparent; border: none; padding: 0; margin: 0; appearance: none; outline: none; line-height: 1; and font: inherit (plus cursor: pointer if not present) so the ClearIcon renders consistently across browsers.
🤖 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/PickerInput/SinglePicker.tsx`:
- Around line 388-391: The clear handler onSelectorClear currently calls
selectorRef.current.focus() which immediately triggers onSelectorFocus and
re-opens the panel; fix by adding a one-time skip flag (e.g., ignoreNextFocus or
skipOpenOnNextFocus) set in onSelectorClear before calling focus, then check
that flag at the start of onSelectorFocus and, if set, clear it and return
without calling triggerOpen(true); ensure the flag is stored on the component
instance (ref or state) so it survives the synchronous focus event and is
cleared after the single use.
---
Outside diff comments:
In `@assets/index.less`:
- Around line 411-420: The clear button (ClearIcon) is now a native button but
.rc-picker-clear (selector represented by &-clear and its &-btn) doesn't reset
browser default button styles; update the &-clear / &-clear-btn rule to
explicitly reset styles by setting background: transparent; border: none;
padding: 0; margin: 0; appearance: none; outline: none; line-height: 1; and
font: inherit (plus cursor: pointer if not present) so the ClearIcon renders
consistently across browsers.
🪄 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: e71b75b5-d4f1-4666-97a4-b7c5c2a1501d
⛔ Files ignored due to path filters (2)
tests/__snapshots__/picker.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/range.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
assets/index.lesssrc/PickerInput/Selector/ClearIcon.tsxsrc/PickerInput/Selector/Icon.tsxsrc/PickerInput/Selector/Input.tsxsrc/PickerInput/Selector/RangeSelector.tsxsrc/PickerInput/Selector/SingleSelector/index.tsxsrc/PickerInput/SinglePicker.tsx
Comment on lines
388
to
+391
| const onSelectorClear = () => { | ||
| triggerSubmitChange(null); | ||
| triggerOpen(false, { force: true }); | ||
| selectorRef.current.focus(); |
Contributor
There was a problem hiding this comment.
清除后立即 focus 会把弹层重新打开
在 Line 391 调用 focus() 后，会走到 Line 575 的 onSelectorFocus，并在 Line 578 再次 triggerOpen(true)，导致 Line 390 的关闭逻辑被抵消。建议对“清除触发的这一次 focus”加开关，跳过自动打开。
建议修复（示例）
+ const skipOpenOnNextFocusRef = React.useRef(false);
const onSelectorClear = () => {
triggerSubmitChange(null);
triggerOpen(false, { force: true });
+ skipOpenOnNextFocusRef.current = true;
selectorRef.current.focus();
};
const onSelectorFocus: SelectorProps['onFocus'] = (event) => {
lastOperation('input');
+ if (skipOpenOnNextFocusRef.current) {
+ skipOpenOnNextFocusRef.current = false;
+ onSharedFocus(event);
+ return;
+ }
triggerOpen(true, {
inherit: true,
});
onSharedFocus(event);
};Also applies to: 575-581
🤖 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/PickerInput/SinglePicker.tsx` around lines 388 - 391, The clear handler
onSelectorClear currently calls selectorRef.current.focus() which immediately
triggers onSelectorFocus and re-opens the panel; fix by adding a one-time skip
flag (e.g., ignoreNextFocus or skipOpenOnNextFocus) set in onSelectorClear
before calling focus, then check that flag at the start of onSelectorFocus and,
if set, clear it and return without calling triggerOpen(true); ensure the flag
is stored on the component instance (ref or state) so it survives the
synchronous focus event and is cleared after the single use.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'useOpen' --exec cat {}Length of output: 13105
🏁 Script executed:
#!/bin/bash
fd -t f 'useDelayState' --exec cat {}Length of output: 1508
@afc163 这个问题确认有效，来龙去脉如下：
根本原因
useDelayState 中，setRafOpen(false, true)（immediately=true 时）会同步调用 doUpdate() → setState(false)，但这只是调度 React 状态更新，rafOpen 变量在当前渲染闭包里仍然是 true，要等到下一次 re-render 才会变成 false。
// useDelayState
const updateValue = (next, immediately?) => {
nextValueRef.current = next;
if (next || immediately) {
doUpdate(); // ← 同步调用，但 setState() 是异步生效的
} else {
rafRef.current = raf(doUpdate);
}
};时序问题
onSelectorClear()
├─ triggerOpen(false, { force: true }) // rafOpen 仍为 true（re-render 还没发生）
└─ selectorRef.current.focus()
└─ onSelectorFocus()（同步触发）
└─ triggerOpen(true, { inherit: true })
// setOpen 内：!inherit || rafOpen = false || true = true → 允许执行
// setRafOpen(true) 再次将 nextValueRef.current 置为 true
// React 最终 re-render 时取的是 true → 弹层重新打开
因此在 useOpen.setOpen 里，inherit: true 的保护逻辑失效了，因为 rafOpen 读到的还是旧值。
建议修复
使用一个 ref 旗标在 onSelectorClear 里标记"下一次 focus 跳过 open"：
+ const skipOpenOnNextFocusRef = React.useRef(false);
const onSelectorClear = () => {
triggerSubmitChange(null);
triggerOpen(false, { force: true });
+ skipOpenOnNextFocusRef.current = true;
selectorRef.current.focus();
};
const onSelectorFocus: SelectorProps['onFocus'] = (event) => {
lastOperation('input');
+ if (skipOpenOnNextFocusRef.current) {
+ skipOpenOnNextFocusRef.current = false;
+ onSharedFocus(event);
+ return;
+ }
triggerOpen(true, {
inherit: true,
});
onSharedFocus(event);
};这样可以在清除后正确聚焦输入框，同时不会导致弹层重新打开。
🧬 Code Graph Analysis Results
Relevant snippets (for understanding src/PickerInput/SinglePicker.tsx behavior)
src/interface.tsx
- Lines 288-305 —
SharedHTMLAttrsexport type SharedHTMLAttrs = Omit< React.InputHTMLAttributes<HTMLDivElement>, | 'value' | 'defaultValue' | 'onChange' | 'placeholder' | 'id' | 'onInvalid' | 'disabled' | 'onFocus' | 'onBlur' | 'onSelect' | 'min' | 'max' | 'onKeyDown' | 'size' | 'prefix' >;
- Lines 494-546 —
SelectorProps<DateType>export interface SelectorProps<DateType = any> extends SharedHTMLAttrs { picker: PickerMode; prefix?: React.ReactNode; clearIcon?: React.ReactNode; suffixIcon?: React.ReactNode; className?: string; style?: React.CSSProperties; /** Add `-placeholder` className as a help info */ activeHelp?: boolean; focused: boolean; onFocus: (event: React.FocusEvent<HTMLInputElement>, index?: number) => void; onBlur: (event: React.FocusEvent<HTMLInputElement>, index?: number) => void; /** Trigger by `enter` key */ onSubmit: VoidFunction; /** `preventDefault` is deprecated which will remove from future version. */ onKeyDown?: LegacyOnKeyDown; locale: Locale; generateConfig: GenerateConfig<DateType>; direction?: 'ltr' | 'rtl'; onClick: React.MouseEventHandler<HTMLDivElement>; onClear: VoidFunction; classNames: string; styles?: React.CSSProperties; format: FormatType<DateType>[]; maskFormat?: string; onInputChange: VoidFunction; onInvalid: (valid: boolean, index?: number) => void; preserveInvalidOnBlur?: boolean; open: boolean; onOpenChange: OnOpenChange; inputReadOnly?: boolean; }
- Lines 327-465 —
SharedPickerProps<DateType>(many props consumed byuseFilledPropsinSinglePicker) - Lines 468-472 —
PickerRefexport interface PickerRef { nativeElement: HTMLDivElement; focus: (options?: FocusOptions) => void; blur: VoidFunction; }
- Lines 105-107 —
BaseInfoexport interface BaseInfo { range?: 'start' | 'end'; }
- Lines 86-86 —
PanelModeexport type PanelMode = 'time' | 'date' | 'week' | 'month' | 'quarter' | 'year' | 'decade';
- Lines 88-88 —
InternalModeexport type InternalMode = PanelMode | 'datetime';
src/PickerInput/hooks/useFilledProps.ts
- Lines 72-220 —
useFilledProps(...)/** * Parameters: props (picker config), optional updater callback. * Returns: [filledProps, internalPicker, complexPicker, formatList, maskFormat, isInvalidateDate] * * - Derives `internalPicker` (adds 'datetime' when `picker==='date' && showTime`) * - Computes merged `locale` / `showTime` config * - Builds `formatList` and `maskFormat` * - Computes `disabledDate` boundary and `isInvalidateDate(date, ...)` * - Exposes filled props including: * - picker/value/defaultValue/pickerValue/defaultPickerValue (normalized to arrays) * - components.input, clearIcon, showTime * - No explicit error handling; uses memoization for derived values. */ export default function useFilledProps(...) { ... }
src/PickerInput/hooks/useRangeValue.ts
- Lines 164-332 — default export
useRangeValue(...)/** * Parameters: * - info: { generateConfig, locale, allowEmpty, order, picker, ...callbacks } * - mergedValue, setInnerValue, getCalendarValue * - triggerCalendarChange(nextValue), disabled list, formatList * - focused, open * - isInvalidateDate(date, { from?, activeIndex }) * * Returns tuple: [flushSubmit(index, needTriggerChange), triggerSubmitChange(value)] * * Key behavior: * - `triggerSubmitChange` validates: * - allowEmpty rules * - order rules (optionally sorts via orderDates) * - invalidation via `isInvalidateDate` for start/end * - If valid and `onChange` exists, calls `onChange(cloneOrNull, dateStringsOrNull)` * - Updates `calendarValue` via `triggerCalendarChange(clone)` * - Uses an effect (via `useLockEffect`) to auto-submit when interaction finishes. * No explicit error handling is present. */ export default function useRangeValue(...) { ... }
- Lines 155-159 —
triggerOk(used foronOkin some pickers)triggerOk = () => { if (onOk) { onOk(calendarValue()); } }
src/PickerInput/hooks/useRangeActive.ts
- Lines 14-96 — default export
useRangeActive(disabled, empty?, mergedOpen?)/** * Parameters: disabled:boolean[], optional empty:boolean[], optional mergedOpen:boolean. * Returns: * - focused:boolean * - triggerFocus(next:boolean) -> void * - lastOperation(type?) -> OperationType * - activeIndex:number (initial 0) * - setActiveIndex(index:number) * - nextActiveIndex(nextValue) -> number|null * - activeList:number[] (ref snapshot) * - updateSubmitIndex(index|null) * - hasActiveSubmitValue(index:number) -> boolean * * Key behavior: * - Tracks lastOperation in a ref * - When `focused || mergedOpen` lock-effect runs, clears activeList if losing focus * - Pushes activeIndex into activeList when focused becomes true */ export default function useRangeActive(...) { ... }
- Lines 5-5 —
OperationType:export type OperationType = 'input' | 'panel' | 'preset-click';
- Lines 49-54 —
lastOperation(type?)lastOperation = (type?: OperationType) => { if (type) { lastOperationRef.current = type; } return lastOperationRef.current; }
src/PickerInput/hooks/useRangePickerValue.ts
- Lines 37-236 — default export
useRangePickerValue(...)/** * Parameters: * - generateConfig, locale * - calendarValue (array for picker fields) * - modes: PanelMode[] * - open:boolean, activeIndex:number * - pickerMode: InternalMode, multiplePanel:boolean * - defaultPickerValue, pickerValue arrays * - timeDefaultValue (legacy) * - onPickerValueChange? (RangePicker onPickerValueChange) * - minDate?, maxDate? * * Returns: [currentPickerValue (DateType), setCurrentIndexPickerValue(value)] * * Key behavior: * - Controlled per-field start/end via `useControlledState` * - Computes current picker value based on activeIndex and time picker merging * - On open/activeIndex switch, may reset picker value from defaultPickerValue, * calendarValue, or "now", applying min/max clamping (with offset when multiplePanel) * - Calls `onPickerValueChange(clone, { source, range:'start'|'end', mode })` when changed * No explicit error handling is present. */ export default function useRangePickerValue(...) { ... }
- Lines 102-123 —
setCurrentPickerValue(...)change notification logicsetCurrentPickerValue = (nextPickerValue, source='panel') => { ... }
- Lines 132-158 —
getEndDatePickerValue(...)end-offset logicconst getEndDatePickerValue = (startDate, endDate) => { ... }
src/PickerInput/Popup/index.tsx
- Lines 53-259 —
Popup<DateType>(props)/** * Parameters: PopupProps<DateType>, including: * - panelRender?, internalMode, picker, showNow * - range/multiple/activeInfo, presets + hover/submit handlers * - focus handlers (onFocus/onBlur), direction * - value, onSelect, isInvalid, defaultOpenValue, onOk/onSubmit * - classNames/styles * * Returns: a ReactNode representing the popup panel layout. * * Key behavior: * - Computes panel offset/arrow position when `range` is true using ResizeObserver * - Builds valueList from `toArray(value)`, supports time-picker empty => defaultOpenValue * - Determines footer submit invalidation via `isInvalid` * - Renders PresetPanel + PopupPanel + Footer, and applies `panelRender` if provided * No explicit error handling is present. */ export default function Popup<DateType extends object = any>(props: PopupProps<DateType>) { ... }
- Lines 22-51 —
PopupProps<DateType> - Lines 175-183 —
onFooterSubmit()(time-picker extraonSelectthenonOk()+onSubmit())onFooterSubmit = () => { if (isTimePickerEmptyValue) { onSelect(defaultOpenValue); } onOk(); onSubmit(); }
src/PickerTrigger/index.tsx
- Lines 63-112 —
PickerTrigger(...)wrapper around rc-trigger/** * Parameters: PickerTriggerProps (popupElement, visible, onClose, alignment/motion/layout options). * Returns: Trigger-wrapped children with popup behavior. * * Key behavior: * - Uses `getRealPlacement(placement, direction==='rtl')` * - Passes `popupVisible={visible}` and calls `onClose()` when popup becomes hidden * - Configures popup motion via `transitionName`, stretches minWidth, and adds class for range/rtl * No explicit error handling is present. */ function PickerTrigger(...) { ... }
- Lines 43-61 —
PickerTriggerProps
src/hooks/useSemantic.ts
- Lines 19-36 —
useSemantic(classNames, styles)/** * Parameters: optional classNames and styles from SharedPickerProps. * Returns: [mergedClassNames, mergedStyles], each ensuring `.popup` exists. * Memoized by React.useMemo; no explicit error handling. */ export default function useSemantic(...) { ... }
src/PickerInput/hooks/useToggleDates.ts
- Lines 10-29 — default export
useToggleDates(generateConfig, locale, panelMode)/** * Parameters: generateConfig, locale, panelMode. * Returns: function toggleDates(list: DateType[], target: DateType) => DateType[] * * Key behavior: * - Finds target in list using `isSame(generateConfig, locale, date, target, panelMode)` * - If present: removes it; else: appends it * No explicit error handling is present. */ export default function useToggleDates(...) { ... }
src/PickerInput/hooks/useFieldsInvalidate.ts
- Lines 8-54 — default export
useFieldsInvalidate(calendarValue, isInvalidateDate, allowEmpty?)/** * Parameters: * - calendarValue: [DateType, DateType] array * - isInvalidateDate(date, { activeIndex, from? }) * - allowEmpty?: boolean[] * * Returns: [submitInvalidates, onSelectorInvalid] * - submitInvalidates: boolean[] per field used by selector aria-disabled / invalid state * - onSelectorInvalid(invalid, index): sets internal invalid flags for typing invalids * * Key behavior: * - If typing produced invalid => true * - If field has value and isInvalidateDate returns true => true * - If empty and allowEmpty[index] is false => true * No explicit error handling is present. */ export default function useFieldsInvalidate(...) { ... }
- Lines 18-20 —
onSelectorInvalidupdater
src/PickerInput/RangePicker.tsx (not SinglePicker, but shares logic patterns)
- Lines 237-242 —
triggerOpen(nextOpen, config?)/** * Parameters: nextOpen:boolean, optional config * Behavior: if all fields are disabled or nextOpen is false, updates merged open. * Returns void. */ triggerOpen: OnOpenChange = (nextOpen, config?: OpenConfig) => { ... }
- Lines 500-507 —
onPresetSubmit(...)(preset click behavior)onPresetSubmit = (nextValues) => { const passed = triggerSubmitChange(nextValues); if (passed) { lastOperation('preset-click'); triggerOpen(false,{force:true}); } }
- Lines 652-685 —
onSelectorFocus(...)(focus/confirm logic; useful for comparison with SinglePicker focus/confirm flow)
If you want, tell me what specific behavior in SinglePicker.tsx you’re trying to verify (open/confirm/hover/selector typing/pickerValue sync), and I can narrow this list to only the directly involved hooks/components.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #971 +/- ##
=======================================
Coverage 98.81% 98.81%
=======================================
Files 65 66 +1
Lines 2695 2696 +1
Branches 744 744
=======================================
+ Hits 2663 2664 +1
Misses 29 29
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves clear button functionality by:
aria-labelattribute with the locale valueFixes ant-design/ant-design#57933.
Summary by CodeRabbit
发布说明
Bug Fixes
Style
Refactor