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);