Maps · View: a focused pin keeps focus off view

Each pin holds a keyed focus handle of its own, a Tab stop and pressable
while its place is in view, so a focused pin that pans out of view keeps
focus and the arrows go on panning, and a popup that scrolls away hands
focus back to its pin wherever it lies. A popup whose place is off view
takes no focus. Tests hold the tall card's cap and scrolling body, the
map staying under its wheel, and markers refuse a repeated key.

src/maps/marker.rs

@@ -46,12 +46,13 @@ impl MapMarker {
         self
     }
 
-    /// The pin with its tip at a view point; pressable when the map has a handler.
+    /// The pin with its tip at a view point. With the map's handler it holds `focus` in view and out, and while `seen` it takes Tab and presses.
     pub(crate) fn place(
         self,
         owner: &ElementId,
         (x, y): (f32, f32),
-        on_marker: Option<OnMarker>,
+        press: Option<(FocusHandle, OnMarker)>,
+        seen: bool,
         cx: &App,
     ) -> AnyElement {
         let theme = cx.theme();
@@ -67,12 +68,12 @@ impl MapMarker {
             .border_1()
             .border_color(gpui::transparent_black())
             .child(Icon::new(IconName::MapPin).size(IconSize::Xl).color(ink))
-            .when_some(on_marker, |pin, on_marker| {
+            .when_some(press, |pin, (focus, on_marker)| {
                 let key = self.key.clone();
-                pin.tab_index(0)
-                    .focus_ring(cx)
-                    .cursor_pointer()
-                    .on_click(move |_, window, cx| on_marker(&key, window, cx))
+                pin.track_focus(&focus).focus_ring(cx).when(seen, |pin| {
+                    pin.cursor_pointer()
+                        .on_click(move |_, window, cx| on_marker(&key, window, cx))
+                })
             });
         let point = || {
             div()

src/maps/tests/mod.rs

@@ -405,3 +405,32 @@ fn a_drag_moves_its_own_map_alone(cx: &mut TestAppContext) {
     );
     assert_eq!(moves(&host, cx), west, "the west map stayed");
 }
+
+/// A focused pin keeps focus as it leaves the view, so the arrows go on panning, and it takes no press out there.
+#[gpui::test]
+fn keys_keep_panning_after_the_focused_pin_leaves_the_view(cx: &mut TestAppContext) {
+    let (host, cx) = stage(map, cx);
+    focus_map(cx);
+    press("tab", cx);
+    let before = moves(&host, cx);
+    for _ in 0..4 {
+        press("left", cx);
+    }
+    assert_eq!(moves(&host, cx), before + 4, "every arrow panned");
+    let (x, _) = last(&host, cx).to_view(LatLon::new(0.0, 0.0), SIZE, TILE);
+    assert!(x > SIZE.0, "the pin lies east of the view: {x}");
+    press("enter", cx);
+    assert_eq!(
+        said(&host, cx),
+        Vec::<SharedString>::new(),
+        "no press out of view"
+    );
+}
+
+#[test]
+#[should_panic(expected = "two markers keyed twin")]
+fn two_markers_with_one_key_fail() {
+    let at = LatLon::new(0.0, 0.0);
+    let _ = MapView::new("map", start())
+        .markers([MapMarker::new("twin", at), MapMarker::new("twin", at)]);
+}

src/maps/tests/popup.rs

@@ -1,4 +1,7 @@
-use gpui::{AnyElement, Entity, IntoElement, Modifiers, TestAppContext, VisualTestContext};
+use gpui::{
+    AnyElement, Entity, InteractiveElement, IntoElement, Modifiers, ParentElement, ScrollDelta,
+    ScrollWheelEvent, TestAppContext, TouchPhase, VisualTestContext, div, point, px,
+};
 
 use super::{Stage, at, focus_map, hear, last, moves, press, said, say, settle, stage, start};
 use crate::maps::{LatLon, LocationPicker, MapMarker, MapPopup, MapView};
@@ -160,3 +163,83 @@ fn escape_from_the_popup_hands_focus_back(cx: &mut TestAppContext) {
         "the pin holds focus"
     );
 }
+
+/// The popup scrolls out of view with its place; focus goes back to its pin, off view too, and the arrows go on panning.
+#[gpui::test]
+fn keys_keep_panning_after_the_popup_leaves_the_view(cx: &mut TestAppContext) {
+    let (host, cx) = stage(opening, cx);
+    open_by_pin(cx);
+    let before = moves(&host, cx);
+    for _ in 0..5 {
+        press("left", cx);
+    }
+    assert_eq!(moves(&host, cx), before + 5, "every arrow panned");
+}
+
+fn far_opening(stage: &Stage, owner: Entity<Stage>) -> AnyElement {
+    let opened = owner.clone();
+    let map = MapView::new("map", start())
+        .markers([MapMarker::new("middle", LatLon::new(0.0, 0.0))])
+        .on_marker(move |_, _, cx| {
+            opened.update(cx, |stage, cx| {
+                stage.open = true;
+                cx.notify();
+            })
+        })
+        .on_viewport(move |view, _, cx| hear(&owner, view, cx));
+    match stage.open {
+        true => map
+            .popup(MapPopup::new(LatLon::new(-60.0, 120.0), "Far").on_close(|_, _| {}))
+            .into_any_element(),
+        false => map.into_any_element(),
+    }
+}
+
+/// A popup whose place lies away from the view takes no focus: the pin keeps it, and the arrows pan.
+#[gpui::test]
+fn a_popup_away_from_the_view_takes_no_focus(cx: &mut TestAppContext) {
+    let (host, cx) = stage(far_opening, cx);
+    open_by_pin(cx);
+    let before = moves(&host, cx);
+    press("left", cx);
+    assert_eq!(moves(&host, cx), before + 1);
+}
+
+fn tall(_: &Stage, owner: Entity<Stage>) -> AnyElement {
+    let lines = (0..20).map(|ix| {
+        let line = div().child(format!("Line {ix}"));
+        match ix {
+            19 => line.debug_selector(|| "last-line".into()),
+            _ => line,
+        }
+    });
+    MapView::new("map", start())
+        .popup(
+            MapPopup::new(LatLon::new(0.0, 0.0), "Tall")
+                .child(div().children(lines))
+                .on_close(|_, _| {}),
+        )
+        .on_viewport(move |view, _, cx| hear(&owner, view, cx))
+        .into_any_element()
+}
+
+/// A card taller than the room under its point stops inside the map, and a wheel over it scrolls its body, not the map.
+#[gpui::test]
+fn a_tall_card_scrolls_its_body_and_holds_the_map(cx: &mut TestAppContext) {
+    let (host, cx) = stage(tall, cx);
+    let before = cx.debug_bounds("last-line").expect("the last line");
+    assert!(
+        before.top() > px(300.0),
+        "the card stops inside the map: {before:?}"
+    );
+    cx.simulate_event(ScrollWheelEvent {
+        position: point(px(200.0), px(250.0)),
+        delta: ScrollDelta::Pixels(point(px(0.0), px(-60.0))),
+        modifiers: Modifiers::none(),
+        touch_phase: TouchPhase::Moved,
+    });
+    settle(cx);
+    let after = cx.debug_bounds("last-line").expect("the last line");
+    assert_eq!(after.top(), before.top() - px(60.0), "the body scrolled");
+    assert_eq!(moves(&host, cx), 0, "the map stayed");
+}

src/maps/view.rs

@@ -1,4 +1,4 @@
-use std::{f64::consts::LOG2_E, rc::Rc};
+use std::{collections::HashSet, f64::consts::LOG2_E, rc::Rc};
 
 use gpui::{
     AnyElement, App, AppContext as _, Bounds, ElementId, EmptyView, Entity, EntityId, ImageSource,
@@ -36,7 +36,7 @@ struct Stage {
     takeover: Option<Entity<Takeover>>,
 }
 
-/// A map of tiles the host supplies by zoom, column and row, with pins and a popup over them; a pin takes a press only while its place is in view. A drag or the wheel pans it; Command or Control with the wheel zooms about the pointer; focused, the arrows pan and + and - zoom, as its buttons do. The owner keeps the viewport; a new one from the owner shows at once. It fills its box; the host gives it a height.
+/// A map of tiles the host supplies by zoom, column and row, with pins and a popup over them; a pin that holds focus keeps it off view, but takes Tab and presses only while its place is in view. A drag or the wheel pans it; Command or Control with the wheel zooms about the pointer; focused, the arrows pan and + and - zoom, as its buttons do. The owner keeps the viewport; a new one from the owner shows at once. It fills its box; the host gives it a height.
 #[derive(IntoElement)]
 pub struct MapView {
     id: ElementId,
@@ -87,8 +87,18 @@ impl MapView {
         self
     }
 
+    /// Pins keyed by their places; two with one key fail, as they would share a focus.
     pub fn markers(mut self, markers: impl IntoIterator<Item = MapMarker>) -> Self {
         self.markers = markers.into_iter().collect();
+        let mut keys = HashSet::new();
+        for marker in &self.markers {
+            assert!(
+                keys.insert(marker.key.clone()),
+                "map {:?}: two markers keyed {}",
+                self.id,
+                marker.key
+            );
+        }
         self
     }
 
@@ -121,6 +131,11 @@ fn sized(bounds: Bounds<Pixels>) -> (f32, f32) {
     (f32::from(bounds.size.width), f32::from(bounds.size.height))
 }
 
+/// Whether a view point lies in a view of `size`.
+fn inside((x, y): (f32, f32), (w, h): (f32, f32)) -> bool {
+    (0.0..=w).contains(&x) && (0.0..=h).contains(&y)
+}
+
 impl RenderOnce for MapView {
     fn render(self, window: &mut Window, cx: &mut App) -> impl IntoElement {
         let id = self.id.clone();
@@ -139,10 +154,23 @@ impl RenderOnce for MapView {
                 .collect(),
             None => Vec::new(),
         };
+        let markers: Vec<AnyElement> = self
+            .markers
+            .into_iter()
+            .map(|marker| {
+                let at = view.to_view(marker.at, size, tile);
+                let seen = inside(at, size);
+                let press = self.on_marker.clone().map(|on_marker| {
+                    let key = (id.clone(), format!("pin-{}", marker.key));
+                    (tab_stop(key.into(), seen, window, cx), on_marker)
+                });
+                marker.place(&id, at, press, seen, cx)
+            })
+            .collect();
         let shown = self
             .popup
             .map(|popup| (view.to_view(popup.at, size, tile), popup))
-            .filter(|((x, y), _)| (0.0..=size.0).contains(x) && (0.0..=size.1).contains(y));
+            .filter(|(at, _)| inside(*at, size));
         let closer = shown.as_ref().and_then(|(_, popup)| popup.closer());
         let popup = match shown {
             Some((at, popup)) => {
@@ -182,15 +210,6 @@ impl RenderOnce for MapView {
             let (local, stage) = (local.clone(), stage.clone());
             Rc::new(move |cx| (local.read(cx).value, sized(stage.read(cx).bounds)))
         };
-        let markers: Vec<AnyElement> = self
-            .markers
-            .into_iter()
-            .map(|marker| {
-                let at = view.to_view(marker.at, size, tile);
-                let seen = (0.0..=size.0).contains(&at.0) && (0.0..=size.1).contains(&at.1);
-                marker.place(&id, at, self.on_marker.clone().filter(|_| seen), cx)
-            })
-            .collect();
         let zoom = |levels: f64, key: &'static str, icon: IconName, words: &'static str| {
             let (now, set) = (now.clone(), set.clone());
             let (least, most) = MapViewport::ZOOMS;