Menus: a mark holds its item, so a neighbor shifted into its place is not chosen; a move after the menu closed marks nothing

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

src/menus/menu.rs

@@ -2,7 +2,8 @@ use std::rc::Rc;
 
 use gpui::{
     Animation, AnimationExt, AnyElement, App, Bounds, ElementId, Entity, InteractiveElement,
-    IntoElement, ParentElement, Pixels, Point, Styled, Window, anchored, canvas, div, prelude::*,
+    IntoElement, ParentElement, Pixels, Point, SharedString, Styled, Window, anchored, canvas, div,
+    prelude::*,
 };
 
 use super::{
@@ -34,8 +35,12 @@ pub(super) fn at_depth<'a>(menu: &'a Menu, levels: &[Level], depth: usize) -> Op
 fn settle(menu: &Menu, levels: &mut Vec<Level>) {
     let mut here = menu;
     for depth in 0..levels.len() {
-        let column = levels[depth].column;
+        let (column, name) = (levels[depth].column, levels[depth].name.clone());
-        let at = levels[depth].at.filter(|&ix| here.runs(ix, column));
+        // A mark holds its item, not just its place.
+        let at = levels[depth].at.filter(|&ix| {
+            here.item_at(ix, column)
+                .is_some_and(|item| !item.disabled && item.label == name)
+        });
         levels[depth].at = at;
         match at
             .and_then(|ix| here.item_at(ix, column))
@@ -50,11 +55,12 @@ fn settle(menu: &Menu, levels: &mut Vec<Level>) {
     }
 }
 
-/// An open level: marked row, button, and their boxes.
+/// An open level: marked row, button, its label, and boxes.
-#[derive(Clone, Copy, Debug, Default, PartialEq)]
+#[derive(Clone, Debug, Default, PartialEq)]
 pub(super) struct Level {
     pub(super) at: Option<usize>,
     pub(super) column: usize,
+    pub(super) name: SharedString,
     pub(super) row: Bounds<Pixels>,
     pub(super) panel: Bounds<Pixels>,
 }
@@ -126,12 +132,18 @@ impl Open {
 
 /// A level marking row `at` at its first enabled button.
 fn first_of(menu: &Menu, at: Option<usize>) -> Level {
+    let Some(ix) = at else {
+        return Level::default();
+    };
+    let column = menu.first(ix).expect("a stepped row runs");
     Level {
         at,
-        column: match at {
+        column,
-            Some(ix) => menu.first(ix).expect("a stepped row runs"),
+        name: menu
-            None => 0,
+            .item_at(ix, column)
-        },
+            .expect("its first spot")
+            .label
+            .clone(),
         ..Level::default()
     }
 }
@@ -146,19 +158,21 @@ pub(super) fn mark(
     cx: &mut App,
 ) {
     state.update(cx, |open, cx| {
-        let Some(here) = at_depth(menu, &open.levels, depth) else {
+        if open.levels.len() <= depth {
+            return log::debug!("menu: level {depth} closed before its mark");
+        }
+        let Some(item) =
+            at_depth(menu, &open.levels, depth).and_then(|here| here.item_at(at, column))
+        else {
             return;
         };
-        let sub = matches!(
+        let (sub, name) = (matches!(item.kind, Kind::Sub(_)), item.label.clone());
-            here.item_at(at, column).map(|item| &item.kind),
-            Some(Kind::Sub(_))
-        );
         open.levels.truncate(depth + 1);
         let level = &mut open.levels[depth];
         if level.at != Some(at) {
             level.row = Bounds::default();
         }
-        (level.at, level.column) = (Some(at), column);
+        (level.at, level.column, level.name) = (Some(at), column, name);
         if open_sub && sub {
             open.levels.push(Level::default());
         }

src/menus/model.rs

@@ -147,11 +147,6 @@ impl Menu {
         }
     }
 
-    /// Whether the spot at row `ix`, button `column`, runs.
-    pub(super) fn runs(&self, ix: usize, column: usize) -> bool {
-        self.item_at(ix, column).is_some_and(|item| !item.disabled)
-    }
-
     /// Row `ix`'s first spot that runs.
     pub(super) fn first(&self, ix: usize) -> Option<usize> {
         match self.entries.get(ix) {

src/menus/tests/buttons.rs

@@ -15,7 +15,7 @@ struct Clip {
     root: FocusHandle,
     ran: Vec<SharedString>,
     paste_off: bool,
-    shown: usize,
+    gone: Option<&'static str>,
 }
 
 impl Clip {
@@ -41,9 +41,12 @@ impl Clip {
                         .keys("secondary-v")
                         .disabled(self.paste_off)
                         .on_click(run("paste")),
+                    MenuItem::new("Delete")
+                        .icon(IconName::Trash2)
+                        .on_click(run("delete")),
                 ]
                 .into_iter()
-                .take(self.shown),
+                .filter(|item| Some(item.label.as_ref()) != self.gone),
             )
             .item(MenuItem::new("Select all").on_click(run("select all")))
     }
@@ -68,7 +71,7 @@ fn clip(cx: &mut TestAppContext) -> (gpui::Entity<Clip>, &mut VisualTestContext)
         root: cx.focus_handle(),
         ran: Vec::new(),
         paste_off: false,
-        shown: 3,
+        gone: None,
     });
     settle(cx);
     (view, cx)
@@ -109,7 +112,7 @@ fn arrows_walk_a_button_row_past_its_disabled_button(cx: &mut TestAppContext) {
     press("down", cx);
     press("left", cx);
     press("enter", cx);
-    assert_eq!(ran(&view, cx), ["cut", "paste", "paste"], "left wraps");
+    assert_eq!(ran(&view, cx), ["cut", "paste", "delete"], "left wraps");
     right_click(cx);
     press("down", cx);
     press("down", cx);
@@ -134,6 +137,32 @@ fn a_pressed_button_runs_and_a_disabled_one_does_not(cx: &mut TestAppContext) {
     );
 }
 
+#[gpui::test]
+fn a_move_after_a_button_closed_the_menu_does_nothing(cx: &mut TestAppContext) {
+    let (view, cx) = clip(cx);
+    right_click(cx);
+    let at = cx.debug_bounds("menu-button-0-2").expect("paste").center();
+    // Both events land before any repaint.
+    cx.update(|window, cx| {
+        let down = gpui::MouseDownEvent {
+            position: at,
+            button: MouseButton::Left,
+            modifiers: Modifiers::none(),
+            click_count: 1,
+            first_mouse: false,
+        };
+        let moved = gpui::MouseMoveEvent {
+            position: at + point(px(1.0), px(0.0)),
+            pressed_button: Some(MouseButton::Left),
+            modifiers: Modifiers::none(),
+        };
+        window.dispatch_event(gpui::PlatformInput::MouseDown(down), cx);
+        window.dispatch_event(gpui::PlatformInput::MouseMove(moved), cx);
+    });
+    settle(cx);
+    assert_eq!(ran(&view, cx), ["paste"]);
+}
+
 #[gpui::test]
 fn the_filter_finds_a_button_by_its_name(cx: &mut TestAppContext) {
     let (view, cx) = clip(cx);
@@ -159,7 +188,7 @@ fn arrows_cross_buttons_while_the_filter_holds_focus(cx: &mut TestAppContext) {
 fn a_marked_button_turned_off_or_gone_runs_nothing(cx: &mut TestAppContext) {
     for change in [
         |clip: &mut Clip| clip.paste_off = true,
-        |clip: &mut Clip| clip.shown = 1,
+        |clip: &mut Clip| clip.gone = Some("Paste"),
     ] {
         let (view, cx) = clip(cx);
         right_click(cx);