diff --git a/crates/component/src/menu/context_menu.rs b/crates/component/src/menu/context_menu.rs index 78fdf42a2f..b46352c24c 100644 --- a/crates/component/src/menu/context_menu.rs +++ b/crates/component/src/menu/context_menu.rs @@ -1,10 +1,14 @@ -use std::{cell::RefCell, rc::Rc}; +use std::{ + cell::{Cell, RefCell}, + rc::Rc, +}; use gpui::{ Anchor, AnyElement, App, Context, DismissEvent, Element, ElementId, Entity, FocusHandle, Focusable, GlobalElementId, Hitbox, HitboxBehavior, InspectorElementId, InteractiveElement, - IntoElement, MouseButton, MouseDownEvent, ParentElement, Pixels, Point, StyleRefinement, - Styled, Subscription, Window, anchored, deferred, div, prelude::FluentBuilder, px, + IntoElement, LayoutId, MouseButton, MouseDownEvent, ParentElement, Pixels, Point, + StyleRefinement, Styled, Subscription, Window, anchored, deferred, div, prelude::FluentBuilder, + px, }; use crate::menu::PopupMenu; @@ -128,6 +132,14 @@ struct ContextMenuSharedState { pub struct ContextMenuState { element: Option, + /// Whether this trigger draws the open menu this frame. + /// + /// Triggers without an `ElementId` fall back to their code location, so + /// rows rendered from one call site share the element state and all see + /// the menu as open. Only the trigger that was pressed draws it: stacked + /// copies of one `PopupMenu` share an item's pending-click state, and the + /// covered copies clear it on mouse up before the visible one fires. + draws_menu: Rc>, shared_state: Rc>, } @@ -135,6 +147,7 @@ impl Default for ContextMenuState { fn default() -> Self { Self { element: None, + draws_menu: Rc::default(), shared_state: Rc::new(RefCell::new(ContextMenuSharedState { menu_view: None, open: false, @@ -146,6 +159,80 @@ impl Default for ContextMenuState { } } +/// The deferred menu layer, laid out by every trigger that shares the open +/// state but drawn only by the one whose bounds contain the press. +struct DeferredMenu { + draws: Rc>, + menu: Option, +} + +impl IntoElement for DeferredMenu { + type Element = Self; + + fn into_element(self) -> Self::Element { + self + } +} + +impl Element for DeferredMenu { + type RequestLayoutState = (); + type PrepaintState = (); + + fn id(&self) -> Option { + None + } + + fn source_location(&self) -> Option<&'static std::panic::Location<'static>> { + None + } + + fn request_layout( + &mut self, + _: Option<&GlobalElementId>, + _: Option<&InspectorElementId>, + window: &mut Window, + cx: &mut App, + ) -> (LayoutId, ()) { + let menu = self.menu.as_mut().expect("menu should exist"); + (menu.request_layout(window, cx), ()) + } + + fn prepaint( + &mut self, + _: Option<&GlobalElementId>, + _: Option<&InspectorElementId>, + _: gpui::Bounds, + _: &mut (), + window: &mut Window, + cx: &mut App, + ) { + if !self.draws.get() { + return; + } + if let Some(menu) = &mut self.menu { + menu.prepaint(window, cx); + } + } + + fn paint( + &mut self, + _: Option<&GlobalElementId>, + _: Option<&InspectorElementId>, + _: gpui::Bounds, + _: &mut (), + _: &mut (), + window: &mut Window, + cx: &mut App, + ) { + if !self.draws.get() { + return; + } + if let Some(menu) = &mut self.menu { + menu.paint(window, cx); + } + } +} + impl Element for ContextMenu { type RequestLayoutState = ContextMenuState; type PrepaintState = Hitbox; @@ -182,6 +269,7 @@ impl Element for ContextMenu< .trigger_focus_handle .get_or_insert_with(|| cx.focus_handle()); let menu_view = state.shared_state.borrow().menu_view.clone(); + let draws_menu = Rc::new(Cell::new(false)); let mut menu_element = None; if open { let has_menu_item = menu_view @@ -223,6 +311,10 @@ impl Element for ContextMenu< ); } } + let menu_element = menu_element.map(|menu| DeferredMenu { + draws: draws_menu.clone(), + menu: Some(menu), + }); let mut element = this .element @@ -237,6 +329,7 @@ impl Element for ContextMenu< layout_id, ContextMenuState { element: Some(element), + draws_menu, shared_state: state.shared_state.clone(), }, ) @@ -261,6 +354,8 @@ impl Element for ContextMenu< { window.set_focus_handle(trigger_focus, cx); } + let position = request_layout.shared_state.borrow().position; + request_layout.draws_menu.set(bounds.contains(&position)); if let Some(element) = &mut request_layout.element { element.prepaint(window, cx); } @@ -366,6 +461,7 @@ impl Element for ContextMenu< #[cfg(test)] mod tests { use super::*; + use crate::menu::PopupMenuItem; use crate::theme::Theme; use gpui::{ Context, FocusHandle, IntoElement, KeyBinding, Render, TestAppContext, VisualTestContext, @@ -495,6 +591,64 @@ mod tests { } } + /// The issue shape (#3134): rows rendered from one call site without an + /// `ElementId` share the context menu's element state. + struct RowsRoot { + clicked: Rc>, + } + + impl Render for RowsRoot { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div().size_full().children((0..3).map(|_| { + let clicked = self.clicked.clone(); + div() + .w(px(100.)) + .h(px(30.)) + .context_menu(move |menu, _, _| { + let clicked = clicked.clone(); + menu.item( + PopupMenuItem::new("Favorite") + .on_click(move |_, _, _| clicked.set(clicked.get() + 1)), + ) + }) + })) + } + } + + #[gpui::test] + fn item_click_fires_once_from_rows_without_an_id(cx: &mut TestAppContext) { + cx.update(|cx| crate::init(cx)); + let clicked = Rc::new(Cell::new(0)); + let (_, cx) = cx.add_window_view({ + let clicked = clicked.clone(); + move |_, _| RowsRoot { clicked } + }); + cx.update(|window, cx| { + window.draw(cx).clear(cx); + }); + + // Right-click the second row; the menu opens at the press position. + let press = point(px(10.), px(40.)); + cx.simulate_mouse_down(press, MouseButton::Right, Default::default()); + cx.simulate_mouse_up(press, MouseButton::Right, Default::default()); + cx.run_until_parked(); + cx.update(|window, cx| { + window.draw(cx).clear(cx); + }); + + // Click the first item, which sits inside the menu's content padding. + let item = point(press.x + px(30.), press.y + px(17.)); + cx.simulate_mouse_move(item, None, Default::default()); + cx.simulate_click(item, Default::default()); + cx.run_until_parked(); + + assert_eq!( + clicked.get(), + 1, + "the item's on_click must fire exactly once" + ); + } + #[gpui::test] fn shortcut_hint_is_painted_on_the_frame_the_menu_opens(cx: &mut TestAppContext) { cx.update(|cx| {