T24d round 2 fix

- BrowserPreview keeps a ScrollHandle for its strip and a tracked focus
  handle per thumbnail; the focused thumbnail scrolls into view once, through
  forms::options::reveal and the canvas now shared as revealer
- Popup draws its reveal through the same revealer
- test tab_brings_a_hidden_frame_into_view

AGENTS.md

@@ -82,7 +82,7 @@ Ely GPUI Component. A component library for GPUI, in light and dark.
 - A window keeps one `feedback::Toaster` and renders one `ToastViewport`; anything holding the entity pushes to it. Messages share `primitives::Severity`.
 - Only gpui can build `ExternalPaths` with paths, so drop rules live in `forms::files::dropped`, where tests reach them.
 - gpui runs key bindings before key listeners. A container takes a child's bound key through the action with `capture_action`, as `Form` takes `Submit`.
-- `ScrollHandle::scroll_to_item` runs in the container's prepaint against last frame's bounds and overflow, so a call before a list's first frame does nothing. Call it from a child's prepaint once the handle has bounds, then request one frame (`navigation::editor_tabs`). A floating list reveals its cursor through `forms::options::reveal`. Never poll layout with `request_animation_frame`.
+- `ScrollHandle::scroll_to_item` runs in the container's prepaint against last frame's bounds and overflow, so a call before a list's first frame does nothing. Call it from a child's prepaint once the handle has bounds, then request one frame (`navigation::editor_tabs`). A floating list reveals its cursor, and a strip its focused item, through `forms::options::reveal` and `revealer`. Never poll layout with `request_animation_frame`.
 - gpui animations run on the wall clock; timers run on the executor's clock, which tests advance by hand. A test that needs an animation settled turns on the theme's reduced motion. Clocks read `background_executor().now()` and wake only their own view.
 - Machine-read marks, QR codes and barcodes, paint `ink` on `paper`: dark on light in both themes, so cameras read them.
 - A layer over pressable content that must still pass scrolls to its ancestors uses `block_mouse_except_scroll`; `occlude` hides them from scrolls too.

TASKS.md

@@ -280,3 +280,4 @@ Per-component lines live in `tasks/`. Tags there:
 | T24c | 1 | FAIL | at 280px the path vanished and Accept all drew outside the box; the diff's fold rows took presses but no Tab |
 | T24c | 2 | PASS | |
 | T24d | 1 | FAIL | at 280px eight thumbnails ran past the browser card; the artifact panel lost its title and its controls ran out of the box |
+| T24d | 2 | FAIL | Tab to a later thumbnail left the strip where it was, the focused frame and its ring clipped |

src/agent/previews.rs

@@ -1,17 +1,17 @@
 use std::rc::Rc;
 
 use gpui::{
-    Animation, AnimationExt, AnyElement, App, Div, ElementId, InteractiveElement, IntoElement,
-    MouseButton, ParentElement, Point, RenderOnce, SharedString, StatefulInteractiveElement,
-    Styled, Window, div, prelude::*, relative,
+    Animation, AnimationExt, AnyElement, App, Div, ElementId, FocusHandle, InteractiveElement,
+    IntoElement, MouseButton, ParentElement, Point, RenderOnce, ScrollHandle, SharedString,
+    StatefulInteractiveElement, Styled, Window, div, prelude::*, relative,
 };
 
 use crate::{
     buttons::{ButtonVariant, IconButton},
     documents::source,
-    forms::{Pick, Run},
+    forms::{Pick, Run, reveal, revealer},
     motion::{self, Spinner},
-    primitives::{FocusRing, Icon, IconName, Image},
+    primitives::{FocusRing, Icon, IconName, Image, tab_stop},
     theme::{ActiveTheme, AvatarSize, ControlSize, IconSize, Radius, TextSize},
     typography::Ellipsis,
 };
@@ -118,8 +118,38 @@ impl BrowserPreview {
     }
 }
 
+/// A frame strip's scroll, and the frame it last brought into view.
+#[derive(Default)]
+struct Strip {
+    scroll: ScrollHandle,
+    revealed: Option<usize>,
+}
+
 impl RenderOnce for BrowserPreview {
-    fn render(self, _: &mut Window, cx: &mut App) -> impl IntoElement {
+    fn render(self, window: &mut Window, cx: &mut App) -> impl IntoElement {
+        let pressable = self.on_show.is_some();
+        let handles: Vec<FocusHandle> = (0..self.frames.len())
+            .map(|ix| {
+                tab_stop(
+                    (self.id.clone(), format!("frame-focus-{ix}")).into(),
+                    pressable,
+                    window,
+                    cx,
+                )
+            })
+            .collect();
+        let focused = handles.iter().position(|handle| handle.is_focused(window));
+        let state = window.use_keyed_state((self.id.clone(), "strip-state"), cx, |_, _| {
+            Strip::default()
+        });
+        let reveal = reveal(
+            &state,
+            |strip| &mut strip.revealed,
+            focused.is_some(),
+            focused.unwrap_or(0),
+            cx,
+        );
+        let scroll = state.read(cx).scroll.clone();
         let theme = cx.theme();
         let colors = theme.colors.clone();
         let thumb = theme.avatar_size(AvatarSize::Lg);
@@ -127,6 +157,7 @@ impl RenderOnce for BrowserPreview {
         let strip = (self.frames.len() > 1).then(|| {
             div()
                 .id((self.id.clone(), "strip"))
+                .track_scroll(&scroll)
                 .overflow_x_scroll()
                 .flex()
                 .gap_1p5()
@@ -154,7 +185,7 @@ impl RenderOnce for BrowserPreview {
                         );
                     tile.style().aspect_ratio = Some(FRAME);
                     tile.when_some(show, |tile, show| {
-                        tile.tab_index(0)
+                        tile.track_focus(&handles[ix])
                             .focus_ring(cx)
                             .cursor_pointer()
                             .on_mouse_down(MouseButton::Left, |_, window, _| {
@@ -166,6 +197,9 @@ impl RenderOnce for BrowserPreview {
                             })
                     })
                 }))
+                .when_some(reveal, |strip, (ix, done)| {
+                    strip.child(revealer(&scroll, ix, done))
+                })
         });
         card(cx)
             .child(address_bar(&self.id, self.url, self.loading, None, cx))

src/agent/tests/previews.rs

@@ -212,3 +212,35 @@ fn a_narrow_panel_wraps_its_header(cx: &mut TestAppContext) {
         top.get()
     );
 }
+
+#[gpui::test]
+fn tab_brings_a_hidden_frame_into_view(cx: &mut TestAppContext) {
+    setup(cx);
+    let height = Rc::new(Cell::new(Pixels::ZERO));
+    let seen = height.clone();
+    let (view, cx) = cx.add_window_view(|_, _| Strip {
+        height: seen,
+        asked: Vec::new(),
+    });
+    cx.update(|window, _| window.activate_window());
+    settle(cx);
+    for _ in 0..8 {
+        cx.update(|window, _| window.focus_next());
+        settle(cx);
+    }
+    settle(cx);
+    let thumb = cx.update(|window, cx| {
+        cx.theme()
+            .avatar_size(AvatarSize::Lg)
+            .to_pixels(window.rem_size())
+    });
+    let last = point(px(280.0 - 24.0), height.get() - px(9.0) - thumb / 2.0);
+    cx.simulate_mouse_move(last, None, Modifiers::none());
+    cx.simulate_click(last, Modifiers::none());
+    settle(cx);
+    assert_eq!(
+        view.read_with(cx, |strip, _| strip.asked.clone()),
+        [7],
+        "the eighth frame sits in view"
+    );
+}

src/forms/mod.rs

@@ -83,7 +83,7 @@ pub use multi::MultiSelect;
 pub use number::{NumberInput, ScrubInput};
 pub use options::Choice;
 pub(crate) use options::{
-    OnFlag, OnValue, OnValues, Pick, Run, float, float_height, step, surface,
+    OnFlag, OnValue, OnValues, Pick, Run, float, float_height, reveal, revealer, step, surface,
 };
 pub use pairs::{FieldArray, KeyValueInput, ListInput};
 pub use path::PathInput;

src/forms/options.rs

@@ -212,6 +212,20 @@ pub(crate) fn float_height(
         .into_any_element()
 }
 
+/// Scrolls child `ix` of `handle`'s box into view once the box has its bounds, asks one more frame, then runs `done`. Put it inside the box.
+pub(crate) fn revealer(handle: &ScrollHandle, ix: usize, done: Run) -> impl IntoElement + use<> {
+    let handle = handle.clone();
+    canvas(
+        move |_, window, cx| {
+            handle.scroll_to_item(ix);
+            window.request_animation_frame();
+            done(window, cx);
+        },
+        |_, _, _, _| {},
+    )
+    .absolute()
+}
+
 /// The row a list scrolls into view: `at` while `shown` and not yet revealed there. A hidden list forgets.
 pub(crate) fn reveal<T: 'static>(
     state: &Entity<T>,
@@ -288,20 +302,7 @@ impl Popup<'_> {
             .children(rows)
             .when_some(
                 self.scroll.zip(self.reveal),
-                |list, (handle, (ix, done))| {
-                    let handle = handle.clone();
-                    list.child(
-                        canvas(
-                            move |_, window, cx| {
-                                handle.scroll_to_item(ix);
-                                window.request_animation_frame();
-                                done(window, cx);
-                            },
-                            |_, _, _, _| {},
-                        )
-                        .absolute(),
-                    )
-                },
+                |list, (handle, (ix, done))| list.child(revealer(handle, ix, done)),
             );
         float(anchor, self.rows.len(), list, window, cx)
     }