fix: trigger onChange when manually clearing input via select all + delete #947
Conversation
…elete When users manually select all text (Ctrl+A) and press Delete/Backspace, the onChange callback is now properly triggered with null, matching the behavior of the clear button. Previously, the input would reset to the previous value on blur without triggering onChange, which was inconsistent and prevented users from clearing dates via keyboard. Changes: - Modified useInputProps to detect empty input and trigger onChange(null) - Added handlers in Input component for both formatted and non-formatted inputs - Updated SingleSelector to properly handle null values by calling onClear - Added comprehensive test coverage with 11 new tests Fixes: #52473 (ant-design/ant-design) All existing tests pass (453 tests), ensuring backward compatibility.
|
@alii13 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @alii13, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a significant user experience and accessibility flaw in date input components. Previously, manually clearing a date field by selecting all text and pressing delete or backspace would not correctly trigger the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough在输入与选择处理路径中加入对手动清空的显式处理：当输入变为空或通过键盘（全选 + Backspace/Delete）清除时，触发相应的清空回调（onChange(null)/onClear），更新输入状态并提前返回以阻止后续处理。 Changes
Sequence Diagram(s)sequenceDiagram
participant 用户
participant Input as Input 组件
participant Hook as useInputProps
participant Selector as SingleSelector
participant Parent as 上层 onChange/onClear
Note over 用户,Input: 用户全选并按 Delete/Backspace 或 将输入清空
用户->>Input: 输入事件 / keydown
alt 检测到手动清空 (text === '' 且 当前值非空)
Input->>Input: 阻止默认、重置输入值
Input->>Parent: onChange(null) / onClear()
Note right of Parent: 上层接收空值清空事件
else 常规格式化/校验流程
Input->>Hook: 继续格式化与校验（可能触发 onModify/onChange）
end
Note over Input,Hook: 若 text === '' 导致钩子提前返回
Input->>Hook: text === ''
Hook->>Hook: onInvalid(false), onChange(null) 并提前返回
Hook-->>Parent: onChange(null)
Note over Selector: 通过选择器接口触发选择/清空
用户->>Selector: 选择器产生 date (可能为 null)
alt date === null
Selector->>Parent: onClear()
else
Selector->>Parent: onChange([date])
end
Estimated code review effort🎯 3 (中等) | ⏱️ ~20-25 minutes 需要额外关注：
Possibly related issues
Possibly related PRs
Suggested reviewers
诗语
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/manual-clear.spec.tsx (1)
🔇 Additional comments (3)
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 addresses an issue where the onChange callback was not triggered when manually clearing a date input using select all + delete. The solution involves adding logic to detect empty input and trigger onChange(null), handling null values properly, and adding comprehensive tests. The changes are fully backward compatible and include new tests to ensure comprehensive coverage.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/PickerInput/Selector/Input.tsx(2 hunks)src/PickerInput/Selector/SingleSelector/index.tsx(1 hunks)src/PickerInput/Selector/hooks/useInputProps.ts(1 hunks)tests/manual-clear.spec.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/PickerInput/Selector/Input.tsx (1)
tests/util/commonUtil.tsx (1)
inputValue(163-165)
tests/manual-clear.spec.tsx (1)
tests/util/commonUtil.tsx (3)
getDay(186-195)openPicker(78-87)waitFakeTimer(71-76)
🪛 Biome (2.1.2)
src/PickerInput/Selector/Input.tsx
[error] 284-284: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (4)
src/PickerInput/Selector/SingleSelector/index.tsx (1)
133-139: 逻辑正确！null 值的处理非常合理，通过调用
onClear()而不是onChange([date])，使手动清除输入的行为与清除按钮保持一致。src/PickerInput/Selector/hooks/useInputProps.ts (1)
188-192: 空输入处理实现正确！通过调用
onChange(null, index)并提前返回，确保空输入被正确传播为 null 值。这与清除按钮的行为保持一致。src/PickerInput/Selector/Input.tsx (1)
216-231: 清除逻辑实现正确！对于无格式输入，当用户选中全部内容并按下 Backspace/Delete 时，正确地清除了输入值并阻止了默认行为。这确保了手动清除与清除按钮的行为一致。
tests/manual-clear.spec.tsx (1)
1-307: 测试覆盖全面且结构良好！测试文件提供了出色的覆盖范围：
- 单选择器的多种清除场景（Delete、Backspace、输入变更）
- 范围选择器的开始/结束/双输入清除
- 与清除按钮行为的对比验证
- 不同 picker 模式的支持
- 无效部分输入的失焦行为
测试结构清晰，使用了适当的 fake timers 和异步等待，确保了行为的准确验证。
- Extracted duplicate logic into reusable handleManualClear function - Added JSDoc comment explaining the function's purpose - Added inline comment explaining why event.preventDefault() is called - Improved code maintainability and testability
- Renamed function to follow event handler naming convention - Added explanatory comment for null date handling - Clarifies that null dates are delegated to onClear handler
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/PickerInput/Selector/Input.tsx(2 hunks)src/PickerInput/Selector/SingleSelector/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/PickerInput/Selector/Input.tsx
🔇 Additional comments (1)
src/PickerInput/Selector/SingleSelector/index.tsx (1)
156-165: 正确集成了新的清空处理逻辑。将
handleSingleChange传递给useInputProps正确地连接了手动清空的处理路径，确保当输入被清空时能够触发onClear回调。这个改动与 PR 目标一致，使手动清空行为与点击清空按钮的行为保持一致。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #947 +/- ##
=======================================
Coverage 98.80% 98.81%
=======================================
Files 65 65
Lines 2680 2691 +11
Branches 744 748 +4
=======================================
+ Hits 2648 2659 +11
Misses 29 29
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Added test case for mask format inputs to cover onFormatKeyDown path - Ensures handleManualClear return statement is covered by tests - Increases test coverage to 456 tests total
|
@li-jia-nan / @afc163 / @zombieJ Can someone please take a look at this PR whenever you have time. Thank you! |
|
@zombieJ Please review this |
|
Bumping this up, incase missed it. |
|
@gemini-code-assist Pls help to CR agaign. |
There was a problem hiding this comment.
Code Review
This is a great fix that addresses an important UX and accessibility issue with manually clearing date inputs. The approach of handling empty input strings in useInputProps and delegating to onClear in SingleSelector is clean and ensures consistent behavior with the clear button. The addition of handleManualClear in the Input component centralizes the keyboard handling logic nicely.
The new test suite manual-clear.spec.tsx is very comprehensive and covers a wide range of scenarios for both single and range pickers. I have a few suggestions to make the tests for the RangePicker even more robust by adding assertions for the onChange callback, which is the core of this fix.
Overall, excellent work on this PR!
- Changed approach per reviewer feedback (@zombieJ) - Detect manual clear in onChange event when input becomes empty - Only allow manual clear when clearIcon is present (allowClear enabled) - If allowClear is disabled, input resets to previous valid date - Works for all input methods (keyboard, paste, voice, etc) - No keyboard event detection needed - cleaner and more general Changes: - Input.tsx: Check clearIcon in onChange to allow/prevent manual clear - SingleSelector: Delegate to onClear when date is null and clearIcon exists - useInputProps: Trigger onChange(null) when text becomes empty - Tests: Updated to use fireEvent.change instead of keyDown - Added test for allowClear={false} behavior All 455 tests pass with 100% line coverage on new code
|
@gemini-code-assist can you re-review the PR? |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of onChange not being triggered when an input is manually cleared. The changes are well-implemented across useInputProps.ts, Input.tsx, and SingleSelector/index.tsx to handle the cleared state correctly by propagating a null value, but only when allowClear is enabled. The solution is robust, maintaining consistency with the existing clear button behavior. The addition of a comprehensive suite of 11 new tests in manual-clear.spec.tsx is excellent, providing strong confidence in the fix and covering various scenarios for both single and range pickers. The code is clean, and the problem and solution are very clearly documented in the pull request description. Great work!
- Renamed tests to accurately reflect what they test - RangePicker doesn't immediately trigger onChange when clearing individual inputs (this is expected behavior due to range validation logic) - Tests verify input values are cleared, not onChange behavior - All 11 manual clear tests passing Before: 'should trigger onChange when manually clearing start input' After: 'should clear start input value when manually clearing' This addresses review feedback while clarifying the actual behavior being tested.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/manual-clear.spec.tsx (3)
165-187: 应添加 onChange 断言以验证回调行为。此测试验证了起始输入被清空后的视觉状态，但缺少对
onChange回调被正确调用的验证。应添加断言确保onChange以正确的参数被调用。基于先前的评审建议，应添加：
await waitFakeTimer(); +expect(onChange).toHaveBeenCalledWith([null, getDay('2023-08-15')], ['', '2023-08-15']); expect(startInput.value).toBe('');
189-211: 应添加 onChange 断言以验证回调行为。与起始输入测试类似，此测试应验证
onChange在结束输入被清空时以正确的参数被调用。基于先前的评审建议，应添加：
await waitFakeTimer(); +expect(onChange).toHaveBeenCalledWith([getDay('2023-08-01'), null], ['2023-08-01', '']); expect(endInput.value).toBe('');
213-241: 应添加 onChange 断言以验证清空两个输入后的最终状态。当两个输入都被清空后，应验证最终的
onChange调用使用null作为日期值，因为范围中的两个值现在都为空。基于先前的评审建议，应在测试末尾添加：
await waitFakeTimer(); +expect(onChange).toHaveBeenLastCalledWith(null, ['', '']); expect(startInput.value).toBe(''); expect(endInput.value).toBe('');
🧹 Nitpick comments (2)
tests/manual-clear.spec.tsx (2)
243-265: 潜在的测试重复。此测试与第 165-187 行的测试非常相似，都测试清空 RangePicker 的起始输入。主要区别在于：
- 此测试没有
needConfirm={false}属性- 此测试没有
fireEvent.blur- 此测试包含初始值断言（第 257 行）
建议：
- 如果这两个测试旨在验证不同的场景（例如，有/无
needConfirm或有/无blur），请更新测试描述以明确区分它们- 或者，如果它们测试相同的行为，考虑合并这些测试或删除其中一个以减少冗余
269-306: 测试设计良好，验证了核心 PR 目标。此测试有效地验证了手动清空与点击清空按钮的行为一致性，这是 PR 的主要目标之一。测试隔离了两个独立的实例并正确地验证了两者都调用
onChange(null, null)。考虑添加一个类似的测试来比较 RangePicker 的手动清空与清空按钮行为，以确保两种组件的行为一致性。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/manual-clear.spec.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/manual-clear.spec.tsx (1)
tests/util/commonUtil.tsx (3)
getDay(186-195)openPicker(78-87)waitFakeTimer(71-76)
🔇 Additional comments (2)
tests/manual-clear.spec.tsx (2)
8-16: 测试设置正确。beforeEach 和 afterEach 的配置合理，使用假计时器来测试异步行为，并在每个测试后正确清理。
1-308: 审查意见包含多项不准确的指控，需要更正。您的审查意见在以下方面有误：
无需显式键盘事件测试 - 根据 git 历史，实现使用"onChange detection instead of keyDown"，而非直接监听键盘事件。因此
fireEvent.change()是正确且充分的测试方式，无需额外的键盘事件模拟。对比测试已存在 - 虽然对比测试仅覆盖 Picker（第305行附近），但并非完全缺失。若要改进，可考虑添加 RangePicker 的对应对比测试。
第67-90行的测试无问题 - 该测试"should reset invalid partial input on blur without triggering onChange"是关于无效输入的重置行为，不是手动清空测试，其描述与实现相符。
实际需要改进的是：
- RangePicker 测试缺少 onChange 断言 - 第165-265行的四个 RangePicker 测试（清空开始输入、结束输入、两个输入、输入值）都没有验证 onChange 的调用，而单选选择器测试则包含此断言，应保持一致。
Likely an incorrect or invalid review comment.
|
@zombieJ thanks for your suggestions. I have resolved those changes, can you please do a re-review whenever you get time? Thanks! |
3 participants
Title
fix: trigger onChange when manually clearing input via select all + delete
Description
This PR fixes the issue where manually clearing a date input by selecting all text (Ctrl+A/Cmd+A) and pressing Delete/Backspace does not trigger the
onChangecallback.Problem
When users manually select and delete date text in the input field:
onChangewas NOT triggeredThis was a UX and accessibility issue that prevented keyboard-only users from clearing dates effectively.
Solution
Changes Made:
useInputProps.ts- Added logic to detect empty input and triggeronChange(null)Input.tsx- Added handlers for both formatted and non-formatted inputsSingleSelector/index.tsx- Updated to properly handle null valuesmanual-clear.spec.tsx- Added 11 comprehensive testsBehavior After Fix:
onChangeis called with(null, null)Testing
All Tests Pass
Manual Testing
onChangeis triggeredBackward Compatibility
✅ Fully backward compatible - all existing tests pass
Fixes
Fixes ant-design/ant-design#52473
Summary by CodeRabbit
Bug Fixes
Tests