fix: enable spacebar to be typed in opened mode#1204
fix: enable spacebar to be typed in opened mode#1204claytonlin1110 wants to merge 2 commits intoreact-component:masterfrom
Conversation
|
@claytonlin1110 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 @claytonlin1110, 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 refines the keyboard interaction within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
浏览此 PR 为 Select 组件的键盘事件处理添加了测试覆盖，包括在不同配置模式（搜索、组合框）下的空格键和回车键行为验证，无功能代码变动。 变更
估计代码审查工作量🎯 2 (简单) | ⏱️ ~10 分钟 可能相关的 PR
建议审查者
诗句
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
claytonlin1110
force-pushed
the
fix/spacebar-enable-opened
branch
from
a2df78d to
f366628
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1204 +/- ##
=======================================
Coverage 99.43% 99.43%
=======================================
Files 31 31
Lines 1236 1236
Branches 446 424 -22
=======================================
Hits 1229 1229
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 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
The pull request aims to address an issue where the spacebar cannot be typed in an opened select box. However, the provided code changes in src/BaseSelect/index.tsx modify the behavior of the Enter key. The change to the Enter key's event.preventDefault() logic appears to introduce a regression: it now only prevents default form submission when the select dropdown is not already open. This means if a user presses Enter while the dropdown is open (e.g., to confirm a selection), the default browser behavior, such as form submission, might occur instead of the intended action within the select component. Please clarify if the intention was to modify the Enter key or if the spacebar issue needs to be addressed in a different part of the codebase, and consider reverting the Enter key change to maintain existing functionality.
| const isSpaceKey = key === ' '; | ||
|
|
||
| // Enter or Space opens dropdown (ARIA combobox: spacebar should open) | ||
| if (isEnterKey || isSpaceKey) { | ||
| // Do not submit form when type in the input; prevent Space from scrolling page | ||
| if (mode !== 'combobox') { | ||
| event.preventDefault(); | ||
| } | ||
|
|
||
| if (isEnterKey || isSpaceKey) { | ||
| // We only manage open state here, close logic should handle by list component |
There was a problem hiding this comment.
The event.preventDefault() call for the Enter key has been moved inside the if (!mergedOpen) block. This means that when the select dropdown is already open (mergedOpen is true), pressing the Enter key will no longer prevent the default browser behavior (e.g., form submission) if mode !== 'combobox'. This is a regression, as Enter key typically confirms selection and prevents form submission when the dropdown is active. The original placement ensured event.preventDefault() was always called for Enter key in non-combobox modes.
// Do not submit form when type in the input
if (mode !== 'combobox') {
event.preventDefault();
}
// We only manage open state here, close logic should handle by list component
if (!mergedOpen) {
triggerOpen(true);
}
|
@afc163 Please review |
|
感觉移动 event.preventDefault() 的位置影响有点大 |
|
I think moving the position of event.preventDefault() (so it runs only when !mergedOpen) is the right fix. PR #1203 is feasible for typing space in the input but not sufficient for space-to-open without scroll; keeping the “by dropdown state” condition is the better approach. |
|
Any feedback @QDyanbing @afc163 ? |
|
I actually considered this approach at the beginning as well. |
|
@zombieJ 也看看呢 |
claytonlin1110
force-pushed
the
fix/spacebar-enable-opened
branch
from
bca683f to
e88d02d
Compare
|
@QDyanbing I just improved the test case. feel free to review. |
![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
🧹 Nitpick comments (1)
tests/Select.test.tsx (1)
3070-3110: Enter 键测试缺少下拉框状态断言且与 PR 目标关联不明确两个 Enter 键测试均在下拉框未打开的情况下分发事件，但：
- 防止表单提交（
preventDefault）的场景更关键的是下拉框已打开时按 Enter 选中选项的情况，该场景未被覆盖。- 非 combobox 测试（Line 3071）在期望
preventDefault被调用的同时，也应断言expectOpen(container)以确认下拉框确实被打开，避免测试在行为变更后出现假阳性。- 建议在测试上方添加注释，说明这是防止 PR 修复意外影响 Enter 键行为的回归测试，否则这两个测试与 PR 目标（空格键修复）之间的关联难以理解。
+ // 以下为回归测试：验证本次空格键修复未意外改变 Enter 键的 preventDefault 行为 describe('Enter key behavior (preventDefault)', () => { it('should call preventDefault on Enter when not combobox to avoid form submit', () => { ... input.dispatchEvent(keyDownEvent); expect(preventDefaultSpy).toHaveBeenCalled(); + // 同时确认下拉框已被打开 + expectOpen(container); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Select.test.tsx` around lines 3070 - 3110, The Enter-key tests dispatch keydown events while the dropdown is closed; update the non-combobox test that expects preventDefault to first open the dropdown and assert it (use expectOpen(container) or the existing helper) before dispatching the Enter event, and add a new assertion that when the dropdown is open pressing Enter selects an option and calls preventDefault; for the combobox test ensure it remains asserting no preventDefault while documenting with a brief comment that these are regression tests protecting Enter behavior alongside the PR's space-key fix (reference the Select component, mode="combobox", expectOpen(container), and preventDefault spy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Select.test.tsx`:
- Around line 3010-3029: Test relies on side effects to open the dropdown but
never asserts it, so add an explicit open check before dispatching the space
key: after fireEvent.change(input, { target: { value: 'hello' } }) call
expectOpen(container, true) (or perform an explicit open action such as
fireEvent.mouseDown on the trigger) to guarantee mergedOpen === true, then
dispatch the KeyboardEvent and assert preventDefault; reference expectOpen,
fireEvent.change, input.dispatchEvent, and the mergedOpen condition when making
the change.
---
Nitpick comments:
In `@tests/Select.test.tsx`:
- Around line 3070-3110: The Enter-key tests dispatch keydown events while the
dropdown is closed; update the non-combobox test that expects preventDefault to
first open the dropdown and assert it (use expectOpen(container) or the existing
helper) before dispatching the Enter event, and add a new assertion that when
the dropdown is open pressing Enter selects an option and calls preventDefault;
for the combobox test ensure it remains asserting no preventDefault while
documenting with a brief comment that these are regression tests protecting
Enter behavior alongside the PR's space-key fix (reference the Select component,
mode="combobox", expectOpen(container), and preventDefault spy).
| it('should allow typing space when search has value (React state updated via fireEvent)', () => { | ||
| const { container } = render( | ||
| <Select showSearch options={[{ value: 'test', label: 'test' }]} />, | ||
| ); | ||
|
|
||
| const input = container.querySelector('input'); | ||
| fireEvent.focus(input); | ||
| fireEvent.change(input, { target: { value: 'hello' } }); | ||
|
|
||
| const keyDownEvent = new KeyboardEvent('keydown', { | ||
| key: ' ', | ||
| code: 'Space', | ||
| bubbles: true, | ||
| }); | ||
| const preventDefaultSpy = jest.spyOn(keyDownEvent, 'preventDefault'); | ||
|
|
||
| input.dispatchEvent(keyDownEvent); | ||
|
|
||
| expect(preventDefaultSpy).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
测试未明确验证下拉框处于打开状态
此测试的核心目的是覆盖 PR 修复的场景（下拉框已打开时允许键入空格），但未调用 expectOpen(container, true) 来显式断言下拉框确实处于打开状态。测试逻辑依赖 fireEvent.change 的副作用来隐式打开下拉框，这使得测试与修复条件（mergedOpen === true）之间的关联不透明，且容易因实现变动而失效。
建议在分发空格键事件前先显式打开下拉框并断言其状态：
✏️ 建议修改
- it('should allow typing space when search has value (React state updated via fireEvent)', () => {
+ it('should allow typing space when the dropdown is open', () => {
const { container } = render(
<Select showSearch options={[{ value: 'test', label: 'test' }]} />,
);
const input = container.querySelector('input');
fireEvent.focus(input);
- fireEvent.change(input, { target: { value: 'hello' } });
+ // 显式打开下拉框并验证其状态，对应 PR 修复的核心条件 mergedOpen === true
+ toggleOpen(container);
+ expectOpen(container, true);
const keyDownEvent = new KeyboardEvent('keydown', {
key: ' ',
code: 'Space',
bubbles: true,
});
const preventDefaultSpy = jest.spyOn(keyDownEvent, 'preventDefault');
input.dispatchEvent(keyDownEvent);
expect(preventDefaultSpy).not.toHaveBeenCalled();
});📝 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.
| it('should allow typing space when search has value (React state updated via fireEvent)', () => { | |
| const { container } = render( | |
| <Select showSearch options={[{ value: 'test', label: 'test' }]} />, | |
| ); | |
| const input = container.querySelector('input'); | |
| fireEvent.focus(input); | |
| fireEvent.change(input, { target: { value: 'hello' } }); | |
| const keyDownEvent = new KeyboardEvent('keydown', { | |
| key: ' ', | |
| code: 'Space', | |
| bubbles: true, | |
| }); | |
| const preventDefaultSpy = jest.spyOn(keyDownEvent, 'preventDefault'); | |
| input.dispatchEvent(keyDownEvent); | |
| expect(preventDefaultSpy).not.toHaveBeenCalled(); | |
| }); | |
| it('should allow typing space when the dropdown is open', () => { | |
| const { container } = render( | |
| <Select showSearch options={[{ value: 'test', label: 'test' }]} />, | |
| ); | |
| const input = container.querySelector('input'); | |
| fireEvent.focus(input); | |
| // 显式打开下拉框并验证其状态，对应 PR 修复的核心条件 mergedOpen === true | |
| toggleOpen(container); | |
| expectOpen(container, true); | |
| const keyDownEvent = new KeyboardEvent('keydown', { | |
| key: ' ', | |
| code: 'Space', | |
| bubbles: true, | |
| }); | |
| const preventDefaultSpy = jest.spyOn(keyDownEvent, 'preventDefault'); | |
| input.dispatchEvent(keyDownEvent); | |
| expect(preventDefaultSpy).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Select.test.tsx` around lines 3010 - 3029, Test relies on side effects
to open the dropdown but never asserts it, so add an explicit open check before
dispatching the space key: after fireEvent.change(input, { target: { value:
'hello' } }) call expectOpen(container, true) (or perform an explicit open
action such as fireEvent.mouseDown on the trigger) to guarantee mergedOpen ===
true, then dispatch the KeyboardEvent and assert preventDefault; reference
expectOpen, fireEvent.change, input.dispatchEvent, and the mergedOpen condition
when making the change.
2 participants
🔗 Related Issues
#56939
#56943
Background & Solution
Background
Due to this PR, space bar is not typed in opened select box.
Solution
Open select box by space only when it's not opened yet so that user can still type a space in opened box.
Summary by CodeRabbit
发行说明