From cab449cdcff0f17dcabd024374c25ac9202b82fe Mon Sep 17 00:00:00 2001 From: KlausUllrich <33103541+KlausUllrich@users.noreply.github.com> Date: Tue, 12 May 2026 21:17:38 +0200 Subject: [PATCH] gpui: Fix scrollbar drag position calculation in list (#53378) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes scroll position inversion when content height changes during a scrollbar drag (e.g. in the agent panel while streaming responses). ### Root cause `set_offset_from_scrollbar` used the **live** `items.summary().height` for `content_height` and computed a `drag_offset` correction (`live − frozen`). However, `max_offset_for_scrollbar()` → `max_scroll_offset()` already uses the **frozen** `scrollbar_drag_start_height` during drag. This mismatch means: 1. The scrollbar computes thumb position using the frozen height range 2. `set_offset_from_scrollbar` converts that position using the live height range 3. As content grows during drag, `drag_offset` increases 4. `(point.y - drag_offset).abs()` produces an incorrectly large value, overshooting `scroll_max` The `.abs()` call also masked the sign convention: `compute_click_offset` (in `scrollbar.rs`) returns `-max_offset * percentage` — a negative value — but `.abs()` converted it regardless of sign, hiding the mismatch. ### Fix - Use `scrollbar_drag_start_height` (frozen during drag) for `content_height` in `set_offset_from_scrollbar`, matching `max_scroll_offset()`. Both sides of the scrollbar mapping now use the same height reference. - Replace `(point.y - drag_offset).abs()` with `(-point.y)` to correctly convert the negative scroll offset. - Remove the symmetric `drag_offset` from `scroll_px_offset_for_scrollbar`, which reported position back to the scrollbar and suffered from the same inconsistency. - Update existing tests to use negative offsets, matching the actual values produced by the scrollbar component. - Add a regression test that freezes height, grows content, drags the scrollbar, and asserts the position is correct. ## Test plan - [x] Existing tests updated to use correct sign convention (negative offsets) - [x] New regression test `test_scrollbar_drag_with_growing_content`: verifies scroll position remains correct when content grows during a scrollbar drag - [ ] Manual: open agent panel, start a long generation, drag scrollbar while content is streaming — position should track the thumb correctly Release Notes: - Fixed scrollbar position jumping or inverting when content height changes during a scrollbar drag (e.g. in the agent panel while streaming). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Smit Barmase --- crates/gpui/src/elements/list.rs | 157 ++++++++++++++++++++++++++++--- 1 file changed, 142 insertions(+), 15 deletions(-) diff --git a/crates/gpui/src/elements/list.rs b/crates/gpui/src/elements/list.rs index 54404fa7b2..7f80d78892 100644 --- a/crates/gpui/src/elements/list.rs +++ b/crates/gpui/src/elements/list.rs @@ -716,7 +716,10 @@ impl ListState { point(Pixels::ZERO, state.max_scroll_offset()) } - /// Returns the current scroll offset adjusted for the scrollbar + /// Returns the current scroll offset adjusted for the scrollbar. + /// + /// The returned offset has a negative `y` component representing + /// how far the content has scrolled. pub fn scroll_px_offset_for_scrollbar(&self) -> Point { let state = &self.0.borrow(); @@ -729,11 +732,7 @@ impl ListState { let mut cursor = state.items.cursor::(()); let summary: ListItemSummary = cursor.summary(&Count(logical_scroll_top.item_ix), Bias::Right); - let content_height = state.items.summary().height; - let drag_offset = - // if dragging the scrollbar, we want to offset the point if the height changed - content_height - state.scrollbar_drag_start_height.unwrap_or(content_height); - let offset = summary.height + logical_scroll_top.offset_in_item - drag_offset; + let offset = summary.height + logical_scroll_top.offset_in_item; Point::new(px(0.), -offset) } @@ -1212,12 +1211,27 @@ impl StateInner { let height = bounds.size.height; let padding = self.last_padding.unwrap_or_default(); - let content_height = self.items.summary().height; + // Scrollbar drag positions are computed from the content height + // captured at drag start, so map them back using the same height. + let content_height = self + .scrollbar_drag_start_height + .unwrap_or_else(|| self.items.summary().height); let scroll_max = (content_height + padding.top + padding.bottom - height).max(px(0.)); - let drag_offset = - // if dragging the scrollbar, we want to offset the point if the height changed - content_height - self.scrollbar_drag_start_height.unwrap_or(content_height); - let new_scroll_top = (point.y - drag_offset).abs().max(px(0.)).min(scroll_max); + let new_scroll_top = (-point.y).max(px(0.)).min(scroll_max); + + // If content grew during the drag, the frozen bottom is below the + // live bottom. Treat dragging to the frozen end as resuming tail follow. + let dragged_to_end = + scroll_max > px(0.) && new_scroll_top >= (scroll_max - px(1.0)).max(px(0.)); + if dragged_to_end && matches!(self.follow_state, FollowState::Tail { .. }) { + self.follow_state = FollowState::Tail { is_following: true }; + let item_count = self.items.summary().count; + self.logical_scroll_top = Some(ListOffset { + item_ix: item_count, + offset_in_item: px(0.), + }); + return; + } self.follow_state.stop_following(); @@ -1923,8 +1937,7 @@ mod test { assert!(state.is_following_tail()); // Simulate the scrollbar moving the viewport to the middle. - // `set_offset_from_scrollbar` accepts a positive distance from the start. - state.set_offset_from_scrollbar(point(px(0.), px(150.))); + state.set_offset_from_scrollbar(point(px(0.), px(-150.))); let offset = state.logical_scroll_top(); assert_eq!(offset.item_ix, 3); @@ -1945,6 +1958,71 @@ mod test { assert_eq!(offset.offset_in_item, px(0.)); } + #[gpui::test] + fn test_scrollbar_drag_with_growing_content(cx: &mut TestAppContext) { + let cx = cx.add_empty_window(); + + let last_item_height = Rc::new(Cell::new(50usize)); + let state = ListState::new(10, crate::ListAlignment::Top, px(0.)).measure_all(); + + struct TestView { + state: ListState, + last_item_height: Rc>, + } + impl Render for TestView { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + let last_item_height = self.last_item_height.clone(); + list(self.state.clone(), move |index, _, _| { + let height = if index == 9 { + last_item_height.get() + } else { + 50 + }; + div().h(px(height as f32)).w_full().into_any() + }) + .w_full() + .h_full() + } + } + + let view = cx.update(|_, cx| { + cx.new(|_| TestView { + state: state.clone(), + last_item_height: last_item_height.clone(), + }) + }); + + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.clone().into_any_element() + }); + + state.scrollbar_drag_started(); + + state.set_offset_from_scrollbar(point(px(0.), px(-150.))); + let scrollbar_offset_before_growth = state.scroll_px_offset_for_scrollbar(); + + let offset = state.logical_scroll_top(); + assert_eq!(offset.item_ix, 3); + assert_eq!(offset.offset_in_item, px(0.)); + + last_item_height.set(550); + state.remeasure_items(9..10); + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.clone().into_any_element() + }); + + assert_eq!(state.max_offset_for_scrollbar().y, px(300.)); + assert_eq!( + state.scroll_px_offset_for_scrollbar(), + scrollbar_offset_before_growth + ); + + state.set_offset_from_scrollbar(point(px(0.), px(-150.))); + let offset = state.logical_scroll_top(); + assert_eq!(offset.item_ix, 3); + assert_eq!(offset.offset_in_item, px(0.)); + } + #[gpui::test] fn test_set_follow_tail_snaps_to_bottom(cx: &mut TestAppContext) { let cx = cx.add_empty_window(); @@ -2182,12 +2260,12 @@ mod test { assert!(state.is_following_tail()); // Drag the scrollbar up to the middle — follow_tail should suspend. - state.set_offset_from_scrollbar(point(px(0.), px(150.))); + state.set_offset_from_scrollbar(point(px(0.), px(-150.))); assert!(!state.is_following_tail()); // Drag the scrollbar back to the bottom — follow_tail should re-engage // on the next paint. - state.set_offset_from_scrollbar(point(px(0.), px(300.))); + state.set_offset_from_scrollbar(point(px(0.), px(-300.))); cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { view.into_any_element() }); @@ -2196,4 +2274,53 @@ mod test { "follow_tail should re-engage after scrolling back to the bottom via the scrollbar" ); } + + #[gpui::test] + fn test_follow_tail_reengages_after_scrollbar_drag_to_bottom_while_growing( + cx: &mut TestAppContext, + ) { + let cx = cx.add_empty_window(); + + let state = ListState::new(10, crate::ListAlignment::Top, px(0.)).measure_all(); + + struct TestView(ListState); + impl Render for TestView { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + list(self.0.clone(), |_, _, _| { + div().h(px(50.)).w_full().into_any() + }) + .w_full() + .h_full() + } + } + + let view = cx.update(|_, cx| cx.new(|_| TestView(state.clone()))); + + state.set_follow_mode(FollowMode::Tail); + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.clone().into_any_element() + }); + assert!(state.is_following_tail()); + + state.scrollbar_drag_started(); + + state.splice(10..10, 10); + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.clone().into_any_element() + }); + + state.set_offset_from_scrollbar(point(px(0.), px(-300.))); + state.scrollbar_drag_ended(); + + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.into_any_element() + }); + + assert!( + state.is_following_tail(), + "follow_tail should re-engage when the user drags the scrollbar to \ + the bottom of its track, even when content has grown during the drag \ + (so frozen_bottom < live_bottom)" + ); + } }