Repository navigation
menu: Focus a context menu before its first frame - #3422
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <[email protected]>
linruohan
pushed a commit
to linruohan/gpui-component
that referenced
this pull request
Oct 9, 2026
Closes longbridge#3364 ## Description With accessibility active, opening a `ContextMenu` aborts a debug build with `set_focus called more than once in a single frame`. GPUI registers the focused element as the frame's accessibility focus while prepainting it, and allows one per frame. `ContextMenu` moved focus to its menu inside the menu's own prepaint. The menu is deferred and prepaints last, so the element that held focus before (for example the right-clicked input) had already registered, and the menu registered a second time. - `ContextMenu` now focuses the menu in the deferred callback that builds it, before its first frame is drawn, after the input selection ownership from longbridge#3382 is resolved. `DeferredMenu::build_menu` no longer touches focus. - The old prepaint focus ran on every frame and pulled focus back whenever it left the menu. That also covered a case it was never written for: entering a submenu with the keyboard and then pointing at another parent item closes the submenu while it still holds focus, leaving Escape and the arrow keys with nowhere to go. `PopupMenu` now takes focus back itself when a hover closes a focused submenu, which also applies to `DropdownMenu` and `AppMenuBar`. Behavior change: while a context menu is open, focus that the application moves elsewhere is no longer pulled back to the menu on the next frame. ## How to Test ```bash cargo test -p gpui-component --lib menu:: cargo test -p gpui-kit --features test-support --test menu ``` - `menu_takes_focus_before_its_first_frame` checks that focus has left the previous element when the frame that draws the menu begins rendering. It fails on `main`. - `hovering_away_from_a_keyboard_entered_submenu_refocuses_its_parent` enters a submenu with the keyboard, hovers another parent item, and checks that Escape still dismisses the menu. It passes on `main`, fails with only the `ContextMenu` change, and passes with this PR. The headless test platform cannot activate the accessibility tree, so the tests cover the focus ordering that causes the panic rather than the panic itself. This has not been run against a screen reader on a native debug build. ## Checklist - [x] I have read the [CONTRIBUTING](../CONTRIBUTING.md) document and followed the guidelines. - [x] Reviewed the changes in this PR and confirmed AI generated code (If any) is accurate. - [x] Passed `cargo run` for story tests related to the changes. - [ ] Tested macOS, Windows and Linux platforms performance (if the change is platform-specific) Co-authored-by: Claude Opus 5.5 <[email protected]>
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
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.
Closes #3364
Description
With accessibility active, opening a
ContextMenuaborts a debug build withset_focus called more than once in a single frame.GPUI registers the focused element as the frame's accessibility focus while prepainting it, and allows one per frame.
ContextMenumoved focus to its menu inside the menu's own prepaint. The menu is deferred and prepaints last, so the element that held focus before (for example the right-clicked input) had already registered, and the menu registered a second time.ContextMenunow focuses the menu in the deferred callback that builds it, before its first frame is drawn, after the input selection ownership from input: Preserve selection, focus ring and context menu actions #3382 is resolved.DeferredMenu::build_menuno longer touches focus.PopupMenunow takes focus back itself when a hover closes a focused submenu, which also applies toDropdownMenuandAppMenuBar.Behavior change: while a context menu is open, focus that the application moves elsewhere is no longer pulled back to the menu on the next frame.
How to Test
menu_takes_focus_before_its_first_framechecks that focus has left the previous element when the frame that draws the menu begins rendering. It fails onmain.hovering_away_from_a_keyboard_entered_submenu_refocuses_its_parententers a submenu with the keyboard, hovers another parent item, and checks that Escape still dismisses the menu. It passes onmain, fails with only theContextMenuchange, and passes with this PR.The headless test platform cannot activate the accessibility tree, so the tests cover the focus ordering that causes the panic rather than the panic itself. This has not been run against a screen reader on a native debug build.
Checklist
cargo runfor story tests related to the changes.