fix: wrong focus behavior when configuring menu selectability #818
Conversation
|
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修改 Menu 的聚焦与选中状态处理：引入并合并 select 键（internalSelectKeys / mergedSelectKeys），在 focus 流程中当 selectable 为 true 时优先聚焦首个选中键，否则尝试 mergedActiveKey，再回退到计算的默认焦点键；补充/调整相关单元测试覆盖行为。 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 用户
participant Menu as Menu 组件
participant Keys as 键来源器
participant Focus as DOM.focus
User->>Menu: 触发 menuRef.focus()
Menu->>Keys: 计算候选焦点键
alt selectable == true
Keys->>Keys: 读取 mergedSelectKeys
alt mergedSelectKeys 首项 有效
Keys-->>Menu: 返回 mergedSelectKeys[0] (已选项)
else
Keys->>Keys: 检查 mergedActiveKey
alt mergedActiveKey 有效
Keys-->>Menu: 返回 mergedActiveKey
else
Keys-->>Menu: 返回 defaultFocusKey
end
end
else selectable == false
Keys-->>Menu: 返回 defaultFocusKey
end
Menu->>Focus: 对应元素调用 focus()
Focus-->>Menu: 聚焦完成 / 抛出异常
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟 Possibly related PRs
Suggested reviewers
诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Summary of ChangesHello @codewizardTM, 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 addresses and resolves an issue where the menu's focus behavior was incorrect, particularly when 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
|
![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 fixes an issue with menu focus behavior when selectable is configured. The changes correctly determine which item to focus based on whether the menu is selectable and if any items are already selected. The logic seems correct and addresses the issue described. I've added one comment with a suggestion to simplify the new logic and improve the code structure for better maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Menu.tsx (2)
404-404: 移除不必要的可选链由于
getMergedSelectKeys()始终返回数组（在第 434-444 行确保），可选链?.是不必要的。应用此差异简化代码：
- if (!getMergedSelectKeys()?.length) { + if (!getMergedSelectKeys().length) {
445-447: 考虑内联或重新放置辅助函数
getMergedSelectKeys函数非常简单且仅在一处使用（第 404 行）。可以考虑：
- 内联调用（更简单）：直接使用
mergedSelectKeys而不是通过函数包装- 移动位置（更好的组织）：将函数定义移到
mergedSelectKeys定义附近（第 434 行之后），提高代码可读性选项 1：内联（推荐）
- if (!getMergedSelectKeys()?.length) { + if (!mergedSelectKeys.length) { if (mergedActiveKey && keys.includes(mergedActiveKey)) { shouldFocusKey = mergedActiveKey; } else { shouldFocusKey = defaultFocusKey; } } } else { shouldFocusKey = defaultFocusKey; } const elementToFocus = key2element.get(shouldFocusKey); if (shouldFocusKey && elementToFocus) { elementToFocus?.focus?.(options); } }, findItem: ({ key: itemKey }) => { const keys = getKeys(); const { key2element } = refreshElements(keys, uuid); return key2element.get(itemKey) || null; }, }; }); // ======================== Select ======================== // >>>>> Select keys const [internalSelectKeys, setMergedSelectKeys] = useControlledState( defaultSelectedKeys || [], selectedKeys, ); const mergedSelectKeys = React.useMemo(() => { if (Array.isArray(internalSelectKeys)) { return internalSelectKeys; } if (internalSelectKeys === null || internalSelectKeys === undefined) { return EMPTY_LIST; } return [internalSelectKeys]; }, [internalSelectKeys]); -function getMergedSelectKeys() { - return mergedSelectKeys; -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Menu.tsx(2 hunks)
🔇 Additional comments (1)
src/Menu.tsx (1)
401-413: 该审查评论基于对代码设计的误解，应予以忽略第 403 行的注释明确记录了设计意图："if there is already a selected item, do not apply the focus"（如果已有选中项，则不应用焦点）。当
selectable=true且存在已选中的键时，shouldFocusKey保持undefined是有意为之，用于保留选中状态而不强制将焦点转移到其他元素。这不是 bug。现有测试（Menu.spec.tsx 第 795-861 行）验证了焦点逻辑在其他场景下正确运行，Focus.spec.tsx 测试了多个焦点场景均通过。没有证据表明此行为会破坏键盘导航。
Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #818 +/- ##
==========================================
- Coverage 99.72% 99.59% -0.14%
==========================================
Files 26 26
Lines 727 733 +6
Branches 199 202 +3
==========================================
+ Hits 725 730 +5
- Misses 2 3 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Focus.spec.tsx (2)
210-255: 测试逻辑正确，建议保持一致性。测试用例正确验证了
selectable={false}时，无论之前的交互如何，调用menuRef.current.focus()都会聚焦第一个可用项。建议：为了与文件中其他测试保持一致，建议在第 230 行使用
await act(async () => render(<TestApp />))包裹 render 调用。- const { getByTestId, container } = render(<TestApp />); + const { getByTestId, container } = await act(async () => render(<TestApp />));
256-276: 建议保持测试风格一致性。与前一个测试用例类似，建议在第 276 行使用
await act(async () => render(<TestApp />))包裹 render 调用，以与文件中其他测试保持一致。- const { getByTestId, container } = render(<TestApp />); + const { getByTestId, container } = await act(async () => render(<TestApp />));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Focus.spec.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Focus.spec.tsx (2)
src/interface.ts (1)
MenuRef(124-132)tests/util.ts (1)
isActive(1-9)
🔇 Additional comments (2)
tests/Focus.spec.tsx (2)
4-4: 导入语句合理。引入
KeyCode和isActive工具函数以支持新的焦点行为测试，符合测试需求。Also applies to: 7-7
31-47: 辅助函数实现合理。
keyDown函数正确模拟了键盘事件并处理了 SubMenu 动画的计时器推进。循环内多次调用act()虽然不太常见，但对于逐步推进计时器并允许 React 批量更新是合理的做法。
|
@gemini-code-assist 请再次 CR |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with focus behavior in the Menu component when configuring menu selectability. The changes modify the logic for determining which item should be focused based on whether the menu is selectable and whether there are already selected items. Additionally, tests have been added to verify the new focus behavior in both selectable and non-selectable scenarios. I have added review comments to address a potential issue with the test cleanup.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Menu.tsx (1)
392-407: 合并选择键计算可再简化，避免不必要的分支与潜在类型不一致useControlledState 在本处的范型应为 string[]；当前对 null/undefined/非数组的分支判断没有实际用处，也可能让 TS 推断变复杂。建议对齐上方 openKeys 的写法，直接用 state 或空数组即可。
- const [internalSelectKeys, setMergedSelectKeys] = useControlledState( - defaultSelectedKeys || [], - selectedKeys, - ); - const mergedSelectKeys = React.useMemo(() => { - if (Array.isArray(internalSelectKeys)) { - return internalSelectKeys; - } - - if (internalSelectKeys === null || internalSelectKeys === undefined) { - return EMPTY_LIST; - } - - return [internalSelectKeys]; - }, [internalSelectKeys]); + const [internalSelectKeys, setMergedSelectKeys] = useControlledState( + defaultSelectedKeys, + selectedKeys, + ); + const mergedSelectKeys = internalSelectKeys || EMPTY_LIST;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Menu.tsx(1 hunks)tests/Focus.spec.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Focus.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/Menu.tsx (1)
src/hooks/useAccessibility.ts (2)
refreshElements(149-165)getFocusableElements(110-113)
|
@gemini-code-assist 请完整的检测 PR 内容，并且对聚焦可能存在的 edge case 进行分析。 |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue with menu focus behavior by differentiating between selectable and non-selectable items. The logic change in src/Menu.tsx is sound, and the accompanying tests in tests/Focus.spec.tsx provide good coverage for the new behavior. I've provided a suggestion to refactor the new logic for better readability and maintainability. Additionally, I've pointed out a couple of minor issues in the new test code, including a potential crash and some leftover commented code that should be cleaned up. Overall, this is a solid fix.
|
@zombieJ 再帮忙 cr 一下 |
| mergedActiveKey && key2element.has(mergedActiveKey) ? mergedActiveKey : undefined; | ||
|
|
||
| const shouldFocusKey = selectable | ||
| ? (selectedFocusKey ?? activeFocusKey ?? defaultFocusKey) |
There was a problem hiding this comment.
这里逻辑不对，mergedActiveKey 就是表示需要被聚焦的 key，应该是上游如果 activeKey 非受控，则在打开的时候 useLayoutEffect 更新一下 activeKey 到 selected 上
2 participants
This PR fixes issue ant-design/ant-design#53495
Determine the focus key based on the selectable configuration:
If the item is not selectable, focus the default item.
If the item is selectable, decide whether to focus based on whether there is already a selected item.
Summary by CodeRabbit
Bug Fixes
Tests