menu: Focus a context menu before its first frame

crates/component/src/menu/context_menu.rs

@@ -177,13 +177,7 @@ struct DeferredMenu {
 }
 
 impl DeferredMenu {
-    fn build_menu(&self, window: &mut Window, cx: &mut App) -> AnyElement {
-        // Focus the menu, so that can be handle the action.
-        let focus_handle = self.menu_view.focus_handle(cx);
-        if !focus_handle.contains_focused(window, cx) {
-            focus_handle.focus(window, cx);
-        }
-
+    fn build_menu(&self, window: &mut Window) -> AnyElement {
         deferred(
             anchored().child(
                 div()
@@ -254,7 +248,7 @@ impl Element for DeferredMenu {
         if !self.draws.get() {
             return;
         }
-        let mut menu = self.build_menu(window, cx);
+        let mut menu = self.build_menu(window);
         menu.prepaint_as_root(bounds.origin, window.viewport_size().into(), window, cx);
         self.menu = Some(menu);
     }
@@ -505,6 +499,11 @@ fn open_menu(
                 input.set_selection_focus(Some(menu.focus_handle(cx)), cx);
             }
 
+            // Focus before the menu's first frame, not while prepainting it:
+            // a frame reports one focused accessibility node, and the element
+            // that held focus may already have claimed it by then.
+            menu.focus_handle(cx).focus(window, cx);
+
             // Set up the subscription for dismiss handling.
             // Hold a Weak here, not a strong clone: the closure
             // would otherwise close the cycle
@@ -616,8 +615,8 @@ mod tests {
             click_count: 1,
             first_mouse: false,
         });
-        // The menu entity is built in a deferred callback, then rendered
-        // (which also focuses it) on the next draw.
+        // The menu entity is built and focused in a deferred callback, then
+        // rendered on the next draw.
         cx.run_until_parked();
         cx.update(|window, cx| {
             _ = window.draw(cx);
@@ -765,6 +764,74 @@ mod tests {
         cx.update(|cx| cx.assert_no_new_leaks(&before));
     }
 
+    /// Records whether the content held focus as each frame began.
+    struct FocusRecordingRoot {
+        content_focus: FocusHandle,
+        content_focused_at_render: Rc<Cell<bool>>,
+    }
+
+    impl Render for FocusRecordingRoot {
+        fn render(&mut self, window: &mut Window, _: &mut Context<Self>) -> impl IntoElement {
+            self.content_focused_at_render
+                .set(self.content_focus.is_focused(window));
+            div()
+                .size_full()
+                .child(
+                    div()
+                        .id("content")
+                        .h(px(40.))
+                        .track_focus(&self.content_focus),
+                )
+                .child(
+                    div()
+                        .id("tab")
+                        .h(px(60.))
+                        .context_menu(|menu, _, _| menu.menu("Close", Box::new(RemoveTab))),
+                )
+        }
+    }
+
+    /// The issue shape (#3364): a frame reports one focused accessibility
+    /// node, claimed in prepaint by whichever focused element comes first.
+    /// Moving focus to the menu while prepainting it let the previous focus
+    /// claim the frame first, which aborts debug builds.
+    #[gpui::test]
+    fn menu_takes_focus_before_its_first_frame(cx: &mut TestAppContext) {
+        cx.update(|cx| crate::init(cx));
+        let content_focused_at_render = Rc::new(Cell::new(false));
+        let (root, cx) = cx.add_window_view({
+            let content_focused_at_render = content_focused_at_render.clone();
+            move |window, cx| {
+                let content_focus = cx.focus_handle();
+                content_focus.focus(window, cx);
+                FocusRecordingRoot {
+                    content_focus,
+                    content_focused_at_render,
+                }
+            }
+        });
+        let content_focus = root.read_with(cx, |root, _| root.content_focus.clone());
+        cx.update(|window, cx| {
+            window.draw(cx).clear(cx);
+        });
+        assert!(content_focused_at_render.get());
+
+        cx.simulate_mouse_down(
+            point(px(50.), px(70.)),
+            MouseButton::Right,
+            Default::default(),
+        );
+
+        assert!(
+            !content_focused_at_render.get(),
+            "focus must leave the content before the frame that draws the menu"
+        );
+        cx.update(|window, cx| {
+            let focused = window.focused(cx);
+            assert!(focused.is_some() && focused.as_ref() != Some(&content_focus));
+        });
+    }
+
     #[gpui::test]
     fn shortcut_hint_is_painted_on_the_frame_the_menu_opens(cx: &mut TestAppContext) {
         cx.update(|cx| {

crates/component/src/menu/popup_menu.rs

@@ -1242,6 +1242,15 @@ impl PopupMenu {
             .selected(selected)
             .on_hover(cx.listener(move |this, hovered, window, cx| {
                 if *hovered {
+                    // Pointing elsewhere closes the open submenu; take back
+                    // the focus the keyboard may have moved into it.
+                    if this.selected_index != Some(ix)
+                        && this.active_submenu().is_some_and(|submenu| {
+                            submenu.focus_handle(cx).contains_focused(window, cx)
+                        })
+                    {
+                        this.focus_handle.focus(window, cx);
+                    }
                     this.selected_index = Some(ix);
                 } else if !is_submenu
                     && this.selected_index == Some(ix)

crates/kit/tests/menu.rs

@@ -9,8 +9,8 @@ use gpui_kit::test::{TestAppContextExt, TestSupportExt, TestWindowExt};
 use gpui_kit::{
     AnyWindowHandle, App, AppContext, ClipboardItem, Context, Entity, FocusHandle, Focusable as _,
     InputEvent, KeyDownEvent, KeyUpEvent, Keystroke, LongPressEvent, MouseButton, MouseDownEvent,
-    MouseUpEvent, Pixels, Point, TestAppContext, TouchPhase, Window, WindowHandle, actions,
-    base::Root, div, point, prelude::*, px, size,
+    MouseMoveEvent, MouseUpEvent, Pixels, Point, TestAppContext, TouchPhase, Window, WindowHandle,
+    actions, base::Root, div, point, prelude::*, px, size,
 };
 use std::{cell::Cell, rc::Rc, time::Duration};
 
@@ -607,6 +607,78 @@ fn submenu_select_all_uses_the_parent_input_action_target(cx: &mut TestAppContex
     .unwrap();
 }
 
+/// Pointing at another item closes a submenu the keyboard had entered, so the
+/// parent menu must take focus back for its keys to keep working.
+#[gpui_kit::test]
+fn hovering_away_from_a_keyboard_entered_submenu_refocuses_its_parent(cx: &mut TestAppContext) {
+    cx.update(gpui_kit::init);
+    let (handle, inputs) = common::open_window(cx, Some(size(px(640.), px(480.))), |window, cx| {
+        cx.new(|cx| ContextMenuInputs {
+            source: cx.new(|cx| TextareaState::new(window, cx).context_menu(false)),
+            other: cx.new(|cx| InputState::new(window, cx)),
+            action_target: None,
+        })
+    });
+    let source_id = inputs.read_with(cx, |inputs, _| ("input", inputs.source.entity_id()));
+    let mut press = Point::default();
+    cx.update_window(handle.into(), |_, window, cx| {
+        window.render_frame(cx);
+        press = window.find(source_id).bounds().center();
+        window.right_click(source_id, cx);
+    })
+    .unwrap();
+    cx.run_until_parked();
+    cx.update_window(handle.into(), |_, window, cx| {
+        window.render_frame(cx);
+        window.press("down", cx);
+        window.press("down", cx);
+        // There is no room on the right, so the submenu opens to the left.
+        window.press("left", cx);
+        window.render_frame(cx);
+        assert_eq!(
+            window.within("submenu").find("popup-menu").focused(),
+            Some(true)
+        );
+
+        // The menu opens at the press, so its first item lies just below it.
+        let parent = window
+            .find_all("popup-menu")
+            .into_iter()
+            .map(|menu| menu.bounds())
+            .find(|bounds| bounds.contains(&(press + point(px(8.), px(8.)))))
+            .unwrap();
+        let first_item = window
+            .find_all(0usize)
+            .into_iter()
+            .map(|item| item.bounds())
+            .find(|bounds| parent.contains(&bounds.center()))
+            .unwrap();
+        window.dispatch_event(
+            MouseMoveEvent {
+                position: first_item.center(),
+                pressed_button: None,
+                modifiers: Default::default(),
+            }
+            .to_platform_input(),
+            cx,
+        );
+    })
+    .unwrap();
+    cx.run_until_parked();
+    cx.update_window(handle.into(), |_, window, cx| {
+        window.render_frame(cx);
+        assert!(window.try_find("submenu").is_none());
+        window.press("escape", cx);
+    })
+    .unwrap();
+    cx.run_until_parked();
+    cx.update_window(handle.into(), |_, window, cx| {
+        window.render_frame(cx);
+        assert!(window.try_find("popup-menu").is_none());
+    })
+    .unwrap();
+}
+
 struct LongPressMenus {
     input: Entity<InputState>,
     text: Entity<TextViewState>,