From c013747b7ccf4c8b8f0179ddc5a81034cefe0aca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Sat, 26 Sep 2026 09:16:01 -0400 Subject: [PATCH] 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 | 2 +- TASKS.md | 1 + src/agent/previews.rs | 48 +++++++++++++++++++++++++++++++------ src/agent/tests/previews.rs | 32 +++++++++++++++++++++++++ src/forms/mod.rs | 2 +- src/forms/options.rs | 29 +++++++++++----------- 6 files changed, 91 insertions(+), 23 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cf30d73..02a277e 100644 --- a/AGENTS.md +++ b/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. diff --git a/TASKS.md b/TASKS.md index 17c10a3..6a91cc3 100644 --- a/TASKS.md +++ b/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 | diff --git a/src/agent/previews.rs b/src/agent/previews.rs index 71dd75b..bbfb4b1 100644 --- a/src/agent/previews.rs +++ b/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, +} + 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 = (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)) diff --git a/src/agent/tests/previews.rs b/src/agent/tests/previews.rs index f5423e6..81a2761 100644 --- a/src/agent/tests/previews.rs +++ b/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" + ); +} diff --git a/src/forms/mod.rs b/src/forms/mod.rs index 04fd1c5..5852b0d 100644 --- a/src/forms/mod.rs +++ b/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; diff --git a/src/forms/options.rs b/src/forms/options.rs index 9f4e0e4..4361da1 100644 --- a/src/forms/options.rs +++ b/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( state: &Entity, @@ -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) }