fix: fix focus when active element is removed #791
Conversation
Walkthrough调整 Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer as 调用方
participant MenuHandle as Menu.imperativeHandle
participant MenuState as Menu 状态 (keys, items)
Consumer->>MenuHandle: call focus()
MenuHandle->>MenuState: read mergedActiveKey, keys, firstFocusable, firstNonDisabled
alt mergedActiveKey 为真 且 keys 包含 mergedActiveKey
MenuHandle->>MenuState: focus(mergedActiveKey)
Note right of MenuHandle #D6EAF8: 使用合法的 mergedActiveKey
else 否则
MenuHandle->>MenuState: focus(firstFocusable || firstNonDisabled)
Note right of MenuHandle #FDEBD0: 回退到首个可聚焦或非禁用项
end
MenuState-->>MenuHandle: focus result
MenuHandle-->>Consumer: 返回/完成
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #791 +/- ##
=======================================
Coverage 99.72% 99.72%
=======================================
Files 27 27
Lines 734 736 +2
Branches 198 199 +1
=======================================
+ Hits 732 734 +2
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Could you add a test case? |
The only way I have been able to reproduce this issue outside of a test is when there is a I have tried to recreate the issue in a test, but have been unable to do so. Setting the focus with |
|
@afc163 Not sure where to go from here since I have been unable to reproduce this issue in a test. |
|
@afc163 I spent some more time on this and was able to create a test which verifies the fix. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/Menu.spec.tsx (3)
806-806: 使用 jest.spyOn 监听 focus，而非直接覆写实例方法直接覆写 DOM 实例方法可读性差且不易恢复；使用
jest.spyOn更安全。建议修改如下：
- const focusSpy = jest.fn(); @@ - const item1 = getByTestId('item1'); - item1.focus = focusSpy; + const item1 = getByTestId('item1'); + const focusSpy = jest.spyOn(item1, 'focus').mockImplementation(() => {});并在断言后恢复：
- expect(focusSpy).toHaveBeenCalled(); + expect(focusSpy).toHaveBeenCalled(); + focusSpy.mockRestore();Also applies to: 851-853
847-849: 断言元素被移除时更语义化的写法与其依赖
getBy*抛错，不如使用queryBy*明确表达“应不存在”。应用以下补丁（与上文对解构同步）：
- // Verify the item is removed - expect(() => getByTestId('item2')).toThrow(); + // Verify the item is removed + expect(queryByTestId('item2')).toBeNull();
856-858: 这里无需额外 act 包裹
menuRef.current.focus()为同步调用；@testing-library/react已为事件做了 act 封装。可直接调用以简化测试。建议：
- await act(async () => { - menuRef.current.focus(); - }); + menuRef.current.focus();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/Menu.spec.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Menu.spec.tsx (1)
src/interface.ts (1)
MenuRef(124-132)
🔇 Additional comments (1)
tests/Menu.spec.tsx (1)
11-11: 引入 domHook OK用于模拟可见性场景以触发可聚焦路径，合理。
|
@afc163 Please advise when this feature is expected to be released. Thanks! |
When using the menu with a dropdown that has
autofocuson, if the menu is closed and the current active item is removed, opening the menu will not focus any item and the arrow keys will not function.I have attached a video showing this behavior before and after my fix.
Before
In this video I have created a button and a dropdown with a menu. The button removes the second menu item. I tab to the dropdown, open it, and use the arrow keys to select the second item. Then I press ESC to close the menu. I then tab to the button, press it using ENTER, and then reopen the dropdown by tabbing to it and pressing enter. You will notice that the focus is not in the menu - also I am pressing arrow keys but they do nothing.
rc-menu-pre-fix-2.mp4
After
I perform the same steps as above, but the focus moves to the first item after removing the second item.
rc-menu-post-fix.mp4
Summary by CodeRabbit