From 6478dc424222744b3f8fcd821842661e22e2ae99 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 07:25:13 -0400 Subject: [PATCH] Forms: lists reveal their cursor once laid out - Select, Combobox and MultiSelect opened a long list at its top, the choice out of view: gpui skips a scroll asked before the list's first frame. Picker and Suggestions share forms::options::reveal, which Popup runs from a canvas inside the list - test: a_long_list_opens_with_its_choice_in_view - AGENTS: the scroll rule names the reveal --- AGENTS.md | 2 +- src/forms/combobox.rs | 3 +- src/forms/mention.rs | 25 +++++-------- src/forms/multi.rs | 3 +- src/forms/options.rs | 30 +++++++++++++-- src/forms/select.rs | 17 +++++++-- src/forms/tests/choices.rs | 77 ++++++++++++++++++++++++++++++++++++-- 7 files changed, 129 insertions(+), 28 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9f43da1..965e779 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 before its bounds are stored, so a first-frame call misfires. Call it from a child's prepaint once the handle has bounds, then request one frame (`navigation::editor_tabs`). 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 through `forms::options::reveal`. 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/src/forms/combobox.rs b/src/forms/combobox.rs index c65ef12..fc00206 100644 --- a/src/forms/combobox.rs +++ b/src/forms/combobox.rs @@ -165,6 +165,7 @@ impl RenderOnce for Combobox { step(&rows, rows.len().saturating_sub(1), 1) }; let (anchor, scroll) = (picker.read(cx).anchor, picker.read(cx).scroll.clone()); + let reveal = Picker::reveal(&picker, open, highlighted, cx); let pick: Pick = { let (id, rows, state, typing, on_change) = ( self.id.clone(), @@ -256,7 +257,7 @@ impl RenderOnce for Combobox { pick, dismiss: None, scroll: Some(&scroll), - reveal: None, + reveal, } .render(window, cx), ) diff --git a/src/forms/mention.rs b/src/forms/mention.rs index 89a2e52..e1d682d 100644 --- a/src/forms/mention.rs +++ b/src/forms/mention.rs @@ -7,7 +7,7 @@ use gpui::{ use super::{ Highlight, Input, TextInput, - options::{Choice, Pick, Popup, Run, step}, + options::{Choice, Pick, Popup, reveal, step}, text::{Down, Enter, Up}, }; use crate::theme::ActiveTheme; @@ -151,21 +151,16 @@ impl Suggestions { let rows: Rc<[Choice]> = self.rows.into(); let shown = open && anchor.is_some(); let highlighted = usable(&rows, picking.read(cx).highlighted); - let revealed = picking.read(cx).revealed.filter(|_| shown); - let stored = (picking.read(cx).highlighted, picking.read(cx).revealed); - if (highlighted, revealed) != stored { - picking.update(cx, |picking, _| { - picking.highlighted = highlighted; - picking.revealed = revealed; - }); + if highlighted != picking.read(cx).highlighted { + picking.update(cx, |picking, _| picking.highlighted = highlighted); } - let reveal = (shown && revealed != Some(highlighted)).then(|| { - let done = picking.clone(); - let mark: Run = Rc::new(move |_, cx| { - done.update(cx, |picking, _| picking.revealed = Some(highlighted)) - }); - (highlighted, mark) - }); + let reveal = reveal( + &picking, + |picking| &mut picking.revealed, + shown, + highlighted, + cx, + ); let scroll = picking.read(cx).scroll.clone(); log::debug!( "suggestions {:?}: open {open}, {} rows, anchor {anchor:?}", diff --git a/src/forms/multi.rs b/src/forms/multi.rs index 3856386..59e9dff 100644 --- a/src/forms/multi.rs +++ b/src/forms/multi.rs @@ -92,6 +92,7 @@ impl RenderOnce for MultiSelect { picker.scroll.clone(), ) }; + let reveal = Picker::reveal(&picker, open, highlighted, cx); let commit: OnValues = { let (id, on_change) = (self.id.clone(), self.on_change); Rc::new(move |next, window, cx| { @@ -222,7 +223,7 @@ impl RenderOnce for MultiSelect { Picker::show(&close, false, at, cx) })), scroll: Some(&scroll), - reveal: None, + reveal, } .render(window, cx), ) diff --git a/src/forms/options.rs b/src/forms/options.rs index 571cf5d..9f4e0e4 100644 --- a/src/forms/options.rs +++ b/src/forms/options.rs @@ -1,9 +1,10 @@ use std::rc::Rc; use gpui::{ - Animation, AnimationExt, AnyElement, App, Bounds, Corner, Div, ElementId, InteractiveElement, - IntoElement, MouseButton, ParentElement, Pixels, ScrollHandle, SharedString, Stateful, - StatefulInteractiveElement, Styled, Window, anchored, canvas, deferred, div, prelude::*, + Animation, AnimationExt, AnyElement, App, Bounds, Corner, Div, ElementId, Entity, + InteractiveElement, IntoElement, MouseButton, ParentElement, Pixels, ScrollHandle, + SharedString, Stateful, StatefulInteractiveElement, Styled, Window, anchored, canvas, deferred, + div, prelude::*, }; use crate::{ @@ -211,6 +212,29 @@ pub(crate) fn float_height( .into_any_element() } +/// 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, + revealed: fn(&mut T) -> &mut Option, + shown: bool, + at: usize, + cx: &mut App, +) -> Option<(usize, Run)> { + let last = state.update(cx, |state, _| { + let last = revealed(state); + if !shown { + *last = None; + } + *last + }); + (shown && last != Some(at)).then(|| { + let state = state.clone(); + let mark: Run = + Rc::new(move |_, cx| state.update(cx, |state, _| *revealed(state) = Some(at))); + (at, mark) + }) +} + /// A list floating under `anchor`, at least as wide. Rows leave focus where it was. pub(crate) struct Popup<'a> { pub id: ElementId, diff --git a/src/forms/select.rs b/src/forms/select.rs index 9631cd8..5de0360 100644 --- a/src/forms/select.rs +++ b/src/forms/select.rs @@ -9,7 +9,7 @@ use gpui::{ use super::{ Choice, input::text_size, - options::{OnValue, Pick, Popup, step}, + options::{OnValue, Pick, Popup, Run, reveal, step}, }; use crate::{ primitives::{Icon, IconName, tab_stop}, @@ -24,6 +24,7 @@ pub(crate) struct Picker { pub highlighted: usize, pub anchor: Bounds, pub scroll: ScrollHandle, + pub revealed: Option, } impl Picker { @@ -31,10 +32,19 @@ impl Picker { state.update(cx, |picker, cx| { picker.open = open; picker.highlighted = highlighted; - picker.scroll.scroll_to_item(highlighted); cx.notify(); }); } + + /// The row to scroll into view while the list shows `highlighted`. + pub fn reveal( + state: &Entity, + shown: bool, + highlighted: usize, + cx: &mut App, + ) -> Option<(usize, Run)> { + reveal(state, |picker| &mut picker.revealed, shown, highlighted, cx) + } } /// Sizes an invisible layer to its parent and records the bounds in `state`. @@ -163,6 +173,7 @@ pub(crate) fn listing( picker.scroll.clone(), ) }; + let reveal = Picker::reveal(&picker, open, highlighted, cx); let pick: Pick = { let (rows, picker) = (rows.clone(), picker.clone()); Rc::new(move |ix, window, cx| { @@ -224,7 +235,7 @@ pub(crate) fn listing( Picker::show(&close, false, at, cx) })), scroll: Some(&scroll), - reveal: None, + reveal, } .render(window, cx), ) diff --git a/src/forms/tests/choices.rs b/src/forms/tests/choices.rs index ceada08..a8029d1 100644 --- a/src/forms/tests/choices.rs +++ b/src/forms/tests/choices.rs @@ -1,11 +1,15 @@ use gpui::{ - AppContext as _, Context, Entity, IntoElement, KeyUpEvent, Keystroke, Render, SharedString, - TestAppContext, VisualTestContext, Window, + AppContext as _, Context, Entity, IntoElement, KeyUpEvent, Keystroke, Modifiers, ParentElement, + Render, SharedString, Styled, TestAppContext, VisualTestContext, Window, div, point, px, }; use super::setup; -use crate::forms::{ - Checkbox, Choice, Combobox, MultiSelect, RadioGroup, Rating, Select, Slider, TextInput, +use crate::{ + forms::{ + Checkbox, Choice, Combobox, MultiSelect, RadioGroup, Rating, Select, Slider, TextInput, + }, + motion, + theme::{ActiveTheme, ControlSize, Theme}, }; fn abc(middle_off: bool) -> [Choice; 3] { @@ -83,6 +87,71 @@ fn select_opens_moves_and_picks_from_the_keyboard(cx: &mut TestAppContext) { assert_eq!(view.read_with(cx, |view, _| view.chosen.clone()), "b"); } +/// A select of thirty rows, low in the window, and the rows it picked. +struct Long { + chosen: SharedString, + picks: Vec, +} + +impl Render for Long { + fn render(&mut self, _: &mut Window, cx: &mut Context) -> impl IntoElement { + let view = cx.entity(); + let rows = (0..30).map(|ix| Choice::new(format!("r{ix:02}"), format!("Row {ix:02}"))); + div().pt(px(200.0)).w(px(240.0)).child( + Select::new("long", rows) + .selected(self.chosen.clone()) + .on_change(move |value, _, cx| { + view.update(cx, |view, cx| { + view.chosen = value.clone(); + view.picks.push(value.clone()); + cx.notify(); + }) + }), + ) + } +} + +#[gpui::test] +fn a_long_list_opens_with_its_choice_in_view(cx: &mut TestAppContext) { + setup(cx); + cx.update(|cx| Theme::update(cx, |theme| theme.reduced_motion = true)); + let (view, cx) = cx.add_window_view(|_, _| Long { + chosen: "r25".into(), + picks: Vec::new(), + }); + let (trigger, list) = cx.update(|window, cx| { + let theme = cx.theme(); + ( + theme + .control_height(ControlSize::Md) + .to_pixels(window.rem_size()), + theme.list_max_height().to_pixels(window.rem_size()), + ) + }); + let field = point(px(40.0), px(200.0) + trigger / 2.0); + cx.simulate_mouse_move(field, None, Modifiers::none()); + cx.simulate_click(field, Modifiers::none()); + frames(cx); + let foot = point( + px(40.0), + px(200.0) + trigger + motion::NUDGE + list - px(8.0), + ); + cx.simulate_mouse_move(foot, None, Modifiers::none()); + cx.simulate_click(foot, Modifiers::none()); + frames(cx); + let picks = view.read_with(cx, |view, _| view.picks.clone()); + assert_eq!(picks, ["r25"], "the list opens with its choice at the foot"); +} + +/// Frames 2ms apart, past reduced motion's 1ms entrances. +fn frames(cx: &mut VisualTestContext) { + for _ in 0..3 { + std::thread::sleep(std::time::Duration::from_millis(2)); + cx.update(|window, _| window.refresh()); + cx.run_until_parked(); + } +} + struct Level { value: f64, }