From 31df3426989207d322b41be48377e916ca84ca79 Mon Sep 17 00:00:00 2001 From: Tryanks Date: Fri, 2 Oct 2026 01:12:10 +0800 Subject: [PATCH] fix(ui): host the touch sheet on gpui-base Sheet The bottom sheet rebuilt what gpui_base::Sheet owns: the overlay press, Escape, and it had no focus trap. It now supplies only the scrim and the surface. OutsideDismissal keeps the release of the dismissing press from reaching what the sheet uncovered, which base's press-time dismissal cannot do; it is armed from the dismissal instead of the press, so the sheet arms it from Sheet::request_close, and a new press disarms it so an Escape costs nothing. --- crates/ui/src/overlay/mod.rs | 37 +++++++++--- crates/ui/src/palette.rs | 4 +- crates/ui/src/widgets/menu.rs | 4 +- crates/ui/src/widgets/popover.rs | 99 ++++++++++++++++++++++---------- 4 files changed, 100 insertions(+), 44 deletions(-) diff --git a/crates/ui/src/overlay/mod.rs b/crates/ui/src/overlay/mod.rs index e211e9588..722e13dba 100644 --- a/crates/ui/src/overlay/mod.rs +++ b/crates/ui/src/overlay/mod.rs @@ -17,10 +17,16 @@ use crate::touch_selection::WindowTouchSelectionOverlay; use dialog::ActiveDialog; use notification::NotificationList; -/// Retains the dismissed pointer sequence after its popup content unmounts. -/// Mount `release_listener` with the trigger, not the transient surface. +/// The release guard for popups gpui-base dismisses on a press outside them. +/// +/// gpui-base closes a popover, menu or sheet on the outside press itself, +/// and GPUI then delivers that press's release, and the click it completes, +/// to whatever the popup uncovered. Arming the guard from the dismissal +/// swallows that one release; the next press disarms it, so a dismissal by +/// Escape or by choosing an item costs nothing. Mount `release_listener` +/// with the trigger, not the transient surface, which is gone by the release. #[derive(Clone, Default)] -pub(crate) struct OutsideDismissal(Rc>>); +pub(crate) struct OutsideDismissal(Rc>); impl OutsideDismissal { pub(crate) fn new(id: ElementId, window: &mut Window, cx: &mut App) -> Self { @@ -30,20 +36,33 @@ impl OutsideDismissal { .clone() } - pub(crate) fn consume(&self, event: &gpui::MouseDownEvent, window: &mut Window, cx: &mut App) { - self.0.set(Some(event.button)); + /// The popup was dismissed; the release of the press that did it, if + /// one is in progress, is swallowed. + pub(crate) fn dismissed(&self) { + self.0.set(true); + } + + /// Dismissed by `event`, which is swallowed as well: a press that closes + /// a menu or a picker does nothing else. + pub(crate) fn consume(&self, window: &mut Window, cx: &mut App) { + self.dismissed(); window.prevent_default(); cx.stop_propagation(); } pub(crate) fn release_listener(&self) -> impl IntoElement { - let pending = self.0.clone(); + let armed = self.0.clone(); gpui::canvas( |_, _, _| {}, move |_, _, window, _| { - window.on_mouse_event(move |event: &gpui::MouseUpEvent, phase, window, cx| { - if phase.capture() && pending.get() == Some(event.button) { - pending.set(None); + let disarm = armed.clone(); + window.on_mouse_event(move |_: &gpui::MouseDownEvent, phase, _, _| { + if phase.capture() { + disarm.set(false); + } + }); + window.on_mouse_event(move |_: &gpui::MouseUpEvent, phase, window, cx| { + if phase.capture() && armed.replace(false) { window.prevent_default(); cx.stop_propagation(); } diff --git a/crates/ui/src/palette.rs b/crates/ui/src/palette.rs index bf8993097..4280af4e5 100644 --- a/crates/ui/src/palette.rs +++ b/crates/ui/src/palette.rs @@ -840,8 +840,8 @@ impl Render for CommandPalette { window.prevent_default(); cx.stop_propagation(); }) - .on_mouse_down_out(cx.listener(move |this, event, window, cx| { - dismissal.consume(event, window, cx); + .on_mouse_down_out(cx.listener(move |this, _, window, cx| { + dismissal.consume(window, cx); this.close(cx); })); let overlay = div() diff --git a/crates/ui/src/widgets/menu.rs b/crates/ui/src/widgets/menu.rs index 1a7a48873..7c58d3912 100644 --- a/crates/ui/src/widgets/menu.rs +++ b/crates/ui/src/widgets/menu.rs @@ -505,8 +505,8 @@ impl Ren .position(position) .snap_to_window_with_margin(px(8.)) .child(div().child(menu.clone()).on_mouse_down_out( - move |event, window, cx| { - dismissal.consume(event, window, cx); + move |_, window, cx| { + dismissal.consume(window, cx); menu.update(cx, |_, cx| cx.emit(DismissEvent)); }, )), diff --git a/crates/ui/src/widgets/popover.rs b/crates/ui/src/widgets/popover.rs index 812caaffd..77bed5671 100644 --- a/crates/ui/src/widgets/popover.rs +++ b/crates/ui/src/widgets/popover.rs @@ -6,8 +6,7 @@ use gpui::{ prelude::FluentBuilder as _, }; use gpui::{ - Focusable as _, Role, SharedString, StatefulInteractiveElement as _, anchored, deferred, div, - point, px, + Focusable as _, Role, SharedString, StatefulInteractiveElement as _, deferred, div, px, }; use gpui_base::StyledExt as _; use std::rc::Rc; @@ -142,6 +141,10 @@ impl RenderOnce for Popover { return self.render_sheet(window, cx); } + // gpui-base's `overlay_closable` dismisses on the outside press and + // lets it through: it would also focus, select or press what the + // popup uncovered. A press that closes a picker or a menu does + // nothing else, so the press and its release are consumed here. let dismissal = crate::overlay::OutsideDismissal::new(self.id.clone(), window, cx); let release = dismissal.clone(); let closable = self.overlay_closable; @@ -173,8 +176,8 @@ impl RenderOnce for Popover { gpui_base::v_flex() .id("tcode-popover-content") .when(closable, |el| { - el.on_mouse_down_out(move |event, window, cx| { - dismissal.consume(event, window, cx); + el.on_mouse_down_out(move |_, window, cx| { + dismissal.consume(window, cx); popover.update(cx, |state, cx| state.dismiss(window, cx)); cx.notify(parent); }) @@ -209,7 +212,6 @@ impl Popover { } }); let dismissal = crate::overlay::OutsideDismissal::new(self.id.clone(), window, cx); - let outside = dismissal.clone(); let closable = self.overlay_closable; let open = state.read(cx).is_open(); // Keep the sheet mounted through its fade-out before dismissal. @@ -244,7 +246,6 @@ impl Popover { // The grabber, the title row and the hairline above the content. let chrome_height = px(16.) + px(48.) + px(1.); let close = state.clone(); - let backdrop = state.clone(); let focus = state.read(cx).focus_handle(cx); let content = self .content @@ -255,16 +256,6 @@ impl Popover { .role(Role::Group) .aria_label(self.sheet_title.clone().unwrap_or_default()) .occlude() - .track_focus(&focus) - .key_context("Popover") - .on_action(window.listener_for(&state, PopoverState::on_action_cancel)) - .when(closable, |surface| { - surface.on_mouse_down_out(move |event, window, cx| { - outside.consume(event, window, cx); - backdrop.update(cx, |state, cx| state.dismiss(window, cx)); - cx.notify(parent); - }) - }) // Offset animation would move the hit targets, so fade with the // backdrop while keeping the sheet in its final position. .opacity(progress) @@ -329,22 +320,34 @@ impl Popover { .children(content) .children(self.children), ); + let request_close = state.clone(); root = root.child( deferred( - anchored().position(point(px(0.), px(0.))).child( - div() - .id("touch-picker-backdrop") - .debug_selector(|| "touch-picker-backdrop".into()) - .occlude() - .w(viewport.width) - .h(viewport.height) - .pb(insets.bottom) - .bg(crate::material::scrim(progress, cx)) - .flex() - .flex_col() - .justify_end() - .child(surface), - ), + gpui_base::Sheet::new(cx) + .focus_handle(focus) + .overlay_closable(closable) + .request_close(move |window, cx| { + dismissal.dismissed(); + request_close.update(cx, |state, cx| state.dismiss(window, cx)); + cx.notify(parent); + }) + .overlay( + div() + .id("touch-picker-backdrop") + .debug_selector(|| "touch-picker-backdrop".into()) + .absolute() + .inset_0() + .occlude() + .bg(crate::material::scrim(progress, cx)), + ) + .surface( + div() + .absolute() + .left_0() + .bottom(insets.bottom) + .w(viewport.width) + .child(surface), + ), ) .with_priority(gpui_base::POPUP_PRIORITY), ); @@ -356,7 +359,7 @@ impl Popover { #[cfg(test)] mod tests { use super::*; - use gpui::{Render, TestAppContext}; + use gpui::{Render, TestAppContext, point}; use std::cell::Cell; struct SheetHarness(Rc>); @@ -539,6 +542,40 @@ mod tests { } } + #[gpui::test] + fn a_dismissal_without_a_press_leaves_the_next_click_alone(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + let clicks = Rc::new(Cell::new(0)); + let releases = Rc::new(Cell::new(0)); + let (_, cx) = cx.add_window_view({ + let clicks = clicks.clone(); + let releases = releases.clone(); + move |_, _| OutsideHarness { + kind: Kind::Sheet, + clicks, + releases, + } + }); + cx.simulate_resize(gpui::size(px(393.), px(852.))); + cx.update(|window, cx| window.draw(cx).clear(cx)); + let trigger = cx.debug_bounds("outside-trigger").unwrap().center(); + cx.simulate_click(trigger, Default::default()); + cx.update(|window, cx| window.draw(cx).clear(cx)); + cx.update(|window, cx| window.draw(cx).clear(cx)); + assert!(cx.debug_bounds("touch-picker-sheet").is_some()); + + cx.simulate_keystrokes("escape"); + cx.run_until_parked(); + cx.executor() + .advance_clock(std::time::Duration::from_millis(200)); + cx.update(|window, cx| window.draw(cx).clear(cx)); + assert!(cx.debug_bounds("touch-picker-sheet").is_none()); + + let position = cx.debug_bounds("outside-target").unwrap().center(); + cx.simulate_click(position, Default::default()); + assert_eq!((clicks.get(), releases.get()), (1, 1)); + } + #[gpui::test] fn bottom_sheet_options_receive_clicks_above_the_backdrop(cx: &mut TestAppContext) { cx.update(crate::theme::init);