From 7fd5ea4bf3d97826d96fb8e31426d18c459e8eb6 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Thu, 11 Jun 2026 00:15:10 -0400 Subject: [PATCH] gpui: Fix list scroll events being reverted by pending scroll (#59002) This PR fixes some cases where scrolling is choppy due to a user inputting a scroll event while a list state is going through a remeasure. Currently, the list state would ignore the user's scroll and fall back to the pending scroll position, this PR fixes this by rebasing the pending scroll onto the user's new scroll position. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - agent_panel: Improve scroll smoothness while a response is streaming --- crates/gpui/src/elements/list.rs | 192 ++++++++++++++++++++++++++++--- 1 file changed, 178 insertions(+), 14 deletions(-) diff --git a/crates/gpui/src/elements/list.rs b/crates/gpui/src/elements/list.rs index 725f517a12..28f47a6b7b 100644 --- a/crates/gpui/src/elements/list.rs +++ b/crates/gpui/src/elements/list.rs @@ -347,6 +347,7 @@ impl ListState { state.reset = true; state.measuring_behavior.reset(); state.logical_scroll_top = None; + state.pending_scroll = None; state.scrollbar_drag_start_height = None; state.items.summary().count }; @@ -546,10 +547,13 @@ impl ListState { cursor.seek(&Height(new_pixel_offset), Bias::Right); } - state.logical_scroll_top = Some(ListOffset { + let scroll_top = ListOffset { item_ix: cursor.start().count, offset_in_item: new_pixel_offset - cursor.start().height, - }); + }; + drop(cursor); + state.rebase_pending_scroll(scroll_top); + state.logical_scroll_top = Some(scroll_top); } /// Scroll the list to the very end (past the last item). @@ -561,6 +565,7 @@ impl ListState { pub fn scroll_to_end(&self) { let state = &mut *self.0.borrow_mut(); let item_count = state.items.summary().count; + state.pending_scroll = None; state.logical_scroll_top = Some(ListOffset { item_ix: item_count, offset_in_item: px(0.), @@ -613,6 +618,7 @@ impl ListState { state.follow_state.stop_following(); } + state.rebase_pending_scroll(scroll_top); state.logical_scroll_top = Some(scroll_top); } @@ -645,6 +651,7 @@ impl ListState { } } + state.rebase_pending_scroll(scroll_top); state.logical_scroll_top = Some(scroll_top); } @@ -784,6 +791,39 @@ impl ListState { } impl StateInner { + /// Re-anchor a pending scroll adjustment from a remeasure onto a newly set + /// scroll position, so it clamps to the remeasured item's new height on + /// the next layout instead of reverting the scroll. + fn rebase_pending_scroll(&mut self, scroll_top: ListOffset) { + let Some(pending) = self.pending_scroll.take() else { + return; + }; + if scroll_top.item_ix >= self.items.summary().count { + return; + } + + self.pending_scroll = match pending { + PendingScroll::Absolute { .. } => Some(PendingScroll::Absolute { + item_ix: scroll_top.item_ix, + offset: scroll_top.offset_in_item, + }), + PendingScroll::Proportional(_) => { + let mut cursor = self.items.cursor::(()); + cursor.seek(&Count(scroll_top.item_ix), Bias::Right); + cursor + .item() + .and_then(|item| item.size_hint()) + .filter(|size| size.height.0 > 0.0) + .map(|size| { + PendingScroll::Proportional(PendingScrollFraction { + item_ix: scroll_top.item_ix, + fraction: (scroll_top.offset_in_item.0 / size.height.0).clamp(0.0, 1.0), + }) + }) + } + }; + } + fn max_scroll_offset(&self) -> Pixels { let bounds = self.last_layout_bounds.unwrap_or_default(); let height = self @@ -827,17 +867,21 @@ impl StateInner { .min(scroll_max); if self.alignment == ListAlignment::Bottom && new_scroll_top == scroll_max { + self.pending_scroll = None; self.logical_scroll_top = None; } else { let (start, ..) = self.items .find::((), &Height(new_scroll_top), Bias::Right); - let item_ix = start.count; - let offset_in_item = new_scroll_top - start.height; - self.logical_scroll_top = Some(ListOffset { - item_ix, - offset_in_item, - }); + let scroll_top = ListOffset { + item_ix: start.count, + offset_in_item: new_scroll_top - start.height, + }; + // The user's scroll supersedes the position stashed by a + // remeasure; re-anchor the pending adjustment so it doesn't revert + // this scroll on the next layout. + self.rebase_pending_scroll(scroll_top); + self.logical_scroll_top = Some(scroll_top); } if delta.y > px(0.) { @@ -1266,6 +1310,7 @@ impl StateInner { 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.pending_scroll = None; self.logical_scroll_top = Some(ListOffset { item_ix: item_count, offset_in_item: px(0.), @@ -1276,18 +1321,19 @@ impl StateInner { self.follow_state.stop_following(); if self.alignment == ListAlignment::Bottom && new_scroll_top == scroll_max { + self.pending_scroll = None; self.logical_scroll_top = None; } else { let (start, _, _) = self.items .find::((), &Height(new_scroll_top), Bias::Right); - let item_ix = start.count; - let offset_in_item = new_scroll_top - start.height; - self.logical_scroll_top = Some(ListOffset { - item_ix, - offset_in_item, - }); + let scroll_top = ListOffset { + item_ix: start.count, + offset_in_item: new_scroll_top - start.height, + }; + self.rebase_pending_scroll(scroll_top); + self.logical_scroll_top = Some(scroll_top); } } } @@ -1978,6 +2024,124 @@ mod test { assert_eq!(offset.offset_in_item, px(40.)); } + #[gpui::test] + fn test_remeasure_then_scroll_does_not_revert_scroll_position(cx: &mut TestAppContext) { + let cx = cx.add_empty_window(); + + let state = ListState::new(20, crate::ListAlignment::Top, px(10.)); + + struct TestView(ListState); + impl Render for TestView { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + list(self.0.clone(), |_, _, _| { + div().h(px(100.)).w_full().into_any() + }) + .w_full() + .h_full() + } + } + + let view = { + let state = state.clone(); + cx.update(|_, cx| cx.new(|_| TestView(state))) + }; + + state.scroll_to(gpui::ListOffset { + item_ix: 5, + offset_in_item: px(40.), + }); + + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.clone().into_any_element() + }); + + state.remeasure_items(5..6); + + cx.simulate_event(ScrollWheelEvent { + position: point(px(50.), px(100.)), + delta: ScrollDelta::Pixels(point(px(0.), px(-30.))), + ..Default::default() + }); + + let offset = state.logical_scroll_top(); + assert_eq!(offset.item_ix, 5); + assert_eq!(offset.offset_in_item, px(70.)); + + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.into_any_element() + }); + + let offset = state.logical_scroll_top(); + assert_eq!(offset.item_ix, 5); + assert_eq!( + offset.offset_in_item, + px(70.), + "scrolling after a remeasure should not be reverted by the stale pending scroll" + ); + } + + #[gpui::test] + fn test_scroll_after_remeasure_clamps_to_shrunk_item_height(cx: &mut TestAppContext) { + let cx = cx.add_empty_window(); + + let item_height = Rc::new(Cell::new(100usize)); + let state = ListState::new(20, crate::ListAlignment::Top, px(10.)); + + struct TestView { + state: ListState, + item_height: Rc>, + } + + impl Render for TestView { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + let height = self.item_height.get(); + list(self.state.clone(), move |index, _, _| { + let height = if index == 5 { height } else { 100 }; + div().h(px(height as f32)).w_full().into_any() + }) + .w_full() + .h_full() + } + } + + let view = { + let state = state.clone(); + let item_height = item_height.clone(); + cx.update(|_, cx| cx.new(|_| TestView { state, item_height })) + }; + + state.scroll_to(gpui::ListOffset { + item_ix: 5, + offset_in_item: px(40.), + }); + + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.clone().into_any_element() + }); + + // Item 5 shrinks from 100px to 50px and is remeasured... + item_height.set(50); + state.remeasure_items(5..6); + + // ...and then the user scrolls down by 30px before the next frame, + // landing at offset 70. + cx.simulate_event(ScrollWheelEvent { + position: point(px(50.), px(100.)), + delta: ScrollDelta::Pixels(point(px(0.), px(-30.))), + ..Default::default() + }); + + cx.draw(point(px(0.), px(0.)), size(px(100.), px(200.)), |_, _| { + view.into_any_element() + }); + + // The rebased pending scroll clamps the user's offset to the item's + // new height instead of leaving it pointing past the end of the item. + let offset = state.logical_scroll_top(); + assert_eq!(offset.item_ix, 5); + assert_eq!(offset.offset_in_item, px(50.)); + } + #[gpui::test] fn test_follow_tail_stays_at_bottom_as_items_grow(cx: &mut TestAppContext) { let cx = cx.add_empty_window();