gpui: Fix uniform list scroll to offset for Top and Bottom strategies (#39938)

Smit Barmase created 3 weeks ago

Closes #39863

Regressed in https://github.com/zed-industries/zed/pull/36653

Release Notes:

- Fixed an issue where clicking a sticky item in the project panel
wouldn’t correctly scroll the view to show its start.

Change summary

crates/gpui/src/elements/uniform_list.rs  | 49 +++++++++++++++++++++---
crates/project_panel/src/project_panel.rs |  2 
2 files changed, 44 insertions(+), 7 deletions(-)

Detailed changes

crates/gpui/src/elements/uniform_list.rs

@@ -138,7 +138,11 @@ impl UniformListScrollHandle {
         })))
     }
 
-    /// Scroll the list so that the given item index is onscreen.
+    /// Scroll the list so that the given item index is visible.
+    ///
+    /// This uses non-strict scrolling: if the item is already fully visible, no scrolling occurs.
+    /// If the item is out of view, it scrolls the minimum amount to bring it into view according
+    /// to the strategy.
     pub fn scroll_to_item(&self, ix: usize, strategy: ScrollStrategy) {
         self.0.borrow_mut().deferred_scroll_to_item = Some(DeferredScrollToItem {
             item_index: ix,
@@ -149,6 +153,9 @@ impl UniformListScrollHandle {
     }
 
     /// Scroll the list so that the given item index is at scroll strategy position.
+    ///
+    /// This uses strict scrolling: the item will always be scrolled to match the strategy position,
+    /// even if it's already visible. Use this when you need precise positioning.
     pub fn scroll_to_item_strict(&self, ix: usize, strategy: ScrollStrategy) {
         self.0.borrow_mut().deferred_scroll_to_item = Some(DeferredScrollToItem {
             item_index: ix,
@@ -158,11 +165,16 @@ impl UniformListScrollHandle {
         });
     }
 
-    /// Scroll the list to the given item index with an offset.
+    /// Scroll the list to the given item index with an offset in number of items.
     ///
-    /// For ScrollStrategy::Top, the item will be placed at the offset position from the top.
+    /// This uses non-strict scrolling: if the item is already visible within the offset region,
+    /// no scrolling occurs.
     ///
-    /// For ScrollStrategy::Center, the item will be centered between offset and the last visible item.
+    /// The offset parameter shrinks the effective viewport by the specified number of items
+    /// from the corresponding edge, then applies the scroll strategy within that reduced viewport:
+    /// - `ScrollStrategy::Top`: Shrinks from top, positions item at the new top
+    /// - `ScrollStrategy::Center`: Shrinks from top, centers item in the reduced viewport
+    /// - `ScrollStrategy::Bottom`: Shrinks from bottom, positions item at the new bottom
     pub fn scroll_to_item_with_offset(&self, ix: usize, strategy: ScrollStrategy, offset: usize) {
         self.0.borrow_mut().deferred_scroll_to_item = Some(DeferredScrollToItem {
             item_index: ix,
@@ -172,6 +184,30 @@ impl UniformListScrollHandle {
         });
     }
 
+    /// Scroll the list so that the given item index is at the exact scroll strategy position with an offset.
+    ///
+    /// This uses strict scrolling: the item will always be scrolled to match the strategy position,
+    /// even if it's already visible.
+    ///
+    /// The offset parameter shrinks the effective viewport by the specified number of items
+    /// from the corresponding edge, then applies the scroll strategy within that reduced viewport:
+    /// - `ScrollStrategy::Top`: Shrinks from top, positions item at the new top
+    /// - `ScrollStrategy::Center`: Shrinks from top, centers item in the reduced viewport
+    /// - `ScrollStrategy::Bottom`: Shrinks from bottom, positions item at the new bottom
+    pub fn scroll_to_item_strict_with_offset(
+        &self,
+        ix: usize,
+        strategy: ScrollStrategy,
+        offset: usize,
+    ) {
+        self.0.borrow_mut().deferred_scroll_to_item = Some(DeferredScrollToItem {
+            item_index: ix,
+            strategy,
+            offset,
+            scroll_strict: true,
+        });
+    }
+
     /// Check if the list is flipped vertically.
     pub fn y_flipped(&self) -> bool {
         self.0.borrow().y_flipped
@@ -392,7 +428,7 @@ impl Element for UniformList {
                         {
                             match deferred_scroll.strategy {
                                 ScrollStrategy::Top => {
-                                    updated_scroll_offset.y = -item_top
+                                    updated_scroll_offset.y = -(item_top - offset_pixels)
                                         .max(Pixels::ZERO)
                                         .min(content_height - list_height)
                                         .max(Pixels::ZERO);
@@ -410,7 +446,8 @@ impl Element for UniformList {
                                         .max(Pixels::ZERO);
                                 }
                                 ScrollStrategy::Bottom => {
-                                    updated_scroll_offset.y = -(item_bottom - list_height)
+                                    updated_scroll_offset.y = -(item_bottom - list_height
+                                        + offset_pixels)
                                         .max(Pixels::ZERO)
                                         .min(content_height - list_height)
                                         .max(Pixels::ZERO);

crates/project_panel/src/project_panel.rs

@@ -4624,7 +4624,7 @@ impl ProjectPanel {
                         project_panel.marked_entries.clear();
                         if is_sticky
                             && let Some((_, _, index)) = project_panel.index_for_entry(entry_id, worktree_id) {
-                                project_panel.scroll_handle.scroll_to_item_with_offset(index, ScrollStrategy::Top, sticky_index.unwrap_or(0));
+                                project_panel.scroll_handle.scroll_to_item_strict_with_offset(index, ScrollStrategy::Top, sticky_index.unwrap_or(0));
                                 cx.notify();
                                 // move down by 1px so that clicked item
                                 // don't count as sticky anymore