diff --git a/crates/component/src/menu/context_menu.rs b/crates/component/src/menu/context_menu.rs index bd3b9a6417..84be9465ff 100644 --- a/crates/component/src/menu/context_menu.rs +++ b/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>, + } + + impl Render for FocusRecordingRoot { + fn render(&mut self, window: &mut Window, _: &mut Context) -> 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| { diff --git a/crates/component/src/menu/popup_menu.rs b/crates/component/src/menu/popup_menu.rs index 3c8ff70f4f..8fad53fde5 100644 --- a/crates/component/src/menu/popup_menu.rs +++ b/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) diff --git a/crates/kit/tests/menu.rs b/crates/kit/tests/menu.rs index 1487dc05fe..a2e8b247a2 100644 --- a/crates/kit/tests/menu.rs +++ b/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, text: Entity,