fix(ui): host the touch sheet on gpui-base Sheet

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<std::cell::Cell<Option<gpui::MouseButton>>>);
+pub(crate) struct OutsideDismissal(Rc<std::cell::Cell<bool>>);
 
 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();
                     }

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

crates/ui/src/widgets/menu.rs

@@ -505,8 +505,8 @@ impl<T: InteractiveElement + ParentElement + Styled + IntoElement + 'static> 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));
                             },
                         )),

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<Cell<usize>>);
@@ -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);