From 49335d54be1b90e22a9e889eeaae86e12b93fe70 Mon Sep 17 00:00:00 2001 From: hrou0003 <54772688+hrou0003@users.noreply.github.com> Date: Tue, 30 Sep 2025 19:04:34 +1000 Subject: [PATCH] Pane tabs: Scroll entire new tab into view (#36827) The state of the child bounds is not up-to-date when `scroll_to_item` gets triggered, causing the new tab to not scroll completely into view. Closes #36317 Release Notes: - Fix an issue where a new tab is only partially visible on creation. --- crates/collab/src/tests/integration_tests.rs | 6 -- crates/gpui/src/elements/div.rs | 60 +++++++++++------- crates/workspace/src/pane.rs | 64 +++++++++++++++++--- 3 files changed, 95 insertions(+), 35 deletions(-) diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index 98acecb1088b75aaec6b07d75d95c67517b56779..d3cd87ad6b9bf81bb3804a88b943703dba8f19be 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/crates/collab/src/tests/integration_tests.rs @@ -6514,14 +6514,8 @@ async fn test_right_click_menu_behind_collab_panel(cx: &mut TestAppContext) { cx.simulate_keystrokes("cmd-n cmd-n cmd-n"); cx.update(|window, _cx| window.refresh()); - let tab_bounds = cx.debug_bounds("TAB-2").unwrap(); let new_tab_button_bounds = cx.debug_bounds("ICON-Plus").unwrap(); - assert!( - tab_bounds.intersects(&new_tab_button_bounds), - "Tab should overlap with the new tab button, if this is failing check if there's been a redesign!" - ); - cx.simulate_event(MouseDownEvent { button: MouseButton::Right, position: new_tab_button_bounds.center(), diff --git a/crates/gpui/src/elements/div.rs b/crates/gpui/src/elements/div.rs index 9e747d864b06391f7c72721918b1c35a2eb7ce62..1a578cd14b015a5b40ee7502b7787f3e3082623e 100644 --- a/crates/gpui/src/elements/div.rs +++ b/crates/gpui/src/elements/div.rs @@ -1384,6 +1384,10 @@ impl Element for Div { (child_max - child_min).into() }; + if let Some(scroll_handle) = self.interactivity.tracked_scroll_handle.as_ref() { + scroll_handle.scroll_to_active_item(); + } + self.interactivity.prepaint( global_id, inspector_id, @@ -2986,8 +2990,7 @@ where } /// Represents an element that can be scrolled *to* in its parent element. -/// -/// Contrary to [ScrollHandle::scroll_to_item], an anchored element does not have to be an immediate child of the parent. +/// Contrary to [ScrollHandle::scroll_to_active_item], an anchored element does not have to be an immediate child of the parent. #[derive(Clone)] pub struct ScrollAnchor { handle: ScrollHandle, @@ -3022,6 +3025,7 @@ struct ScrollHandleState { child_bounds: Vec>, scroll_to_bottom: bool, overflow: Point, + active_item: Option, } /// A handle to the scrollable aspects of an element. @@ -3081,32 +3085,44 @@ impl ScrollHandle { self.0.borrow().child_bounds.get(ix).cloned() } - /// scroll_to_item scrolls the minimal amount to ensure that the child is - /// fully visible + /// Update [ScrollHandleState]'s active item for scrolling to in prepaint pub fn scroll_to_item(&self, ix: usize) { - let state = self.0.borrow(); + let mut state = self.0.borrow_mut(); + state.active_item = Some(ix); + } + + /// Scrolls the minimal amount to ensure that the child is + /// fully visible + fn scroll_to_active_item(&self) { + let mut state = self.0.borrow_mut(); - let Some(bounds) = state.child_bounds.get(ix) else { + let Some(active_item_index) = state.active_item else { return; }; + let active_item = match state.child_bounds.get(active_item_index) { + Some(bounds) => { + let mut scroll_offset = state.offset.borrow_mut(); + + if state.overflow.y == Overflow::Scroll { + if bounds.top() + scroll_offset.y < state.bounds.top() { + scroll_offset.y = state.bounds.top() - bounds.top(); + } else if bounds.bottom() + scroll_offset.y > state.bounds.bottom() { + scroll_offset.y = state.bounds.bottom() - bounds.bottom(); + } + } - let mut scroll_offset = state.offset.borrow_mut(); - - if state.overflow.y == Overflow::Scroll { - if bounds.top() + scroll_offset.y < state.bounds.top() { - scroll_offset.y = state.bounds.top() - bounds.top(); - } else if bounds.bottom() + scroll_offset.y > state.bounds.bottom() { - scroll_offset.y = state.bounds.bottom() - bounds.bottom(); - } - } - - if state.overflow.x == Overflow::Scroll { - if bounds.left() + scroll_offset.x < state.bounds.left() { - scroll_offset.x = state.bounds.left() - bounds.left(); - } else if bounds.right() + scroll_offset.x > state.bounds.right() { - scroll_offset.x = state.bounds.right() - bounds.right(); + if state.overflow.x == Overflow::Scroll { + if bounds.left() + scroll_offset.x < state.bounds.left() { + scroll_offset.x = state.bounds.left() - bounds.left(); + } else if bounds.right() + scroll_offset.x > state.bounds.right() { + scroll_offset.x = state.bounds.right() - bounds.right(); + } + } + None } - } + None => Some(active_item_index), + }; + state.active_item = active_item; } /// Scrolls to the bottom. diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index 726488a2dda52b63f294d16d0434709d1ab4c985..5f36c8e83fa4d0eb454786ae5c5c5470eff12072 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -367,6 +367,9 @@ pub struct Pane { max_tabs: Option, _subscriptions: Vec, tab_bar_scroll_handle: ScrollHandle, + /// This is set to true if a user scroll has occurred more recently than a system scroll + /// We want to suppress certain system scrolls when the user has intentionally scrolled + suppress_scroll: bool, /// Is None if navigation buttons are permanently turned off (and should not react to setting changes). /// Otherwise, when `display_nav_history_buttons` is Some, it determines whether nav buttons should be displayed. display_nav_history_buttons: Option, @@ -497,6 +500,7 @@ impl Pane { }))), toolbar: cx.new(|_| Toolbar::new()), tab_bar_scroll_handle: ScrollHandle::new(), + suppress_scroll: false, drag_split_direction: None, workspace, project: project.downgrade(), @@ -573,6 +577,9 @@ impl Pane { if !self.was_focused { self.was_focused = true; self.update_history(self.active_item_index); + if !self.suppress_scroll && self.items.get(self.active_item_index).is_some() { + self.update_active_tab(self.active_item_index); + } cx.emit(Event::Focus); cx.notify(); } @@ -618,6 +625,7 @@ impl Pane { self.toolbar.update(cx, |toolbar, cx| { toolbar.focus_changed(false, window, cx); }); + cx.notify(); } @@ -1124,6 +1132,7 @@ impl Pane { } } else { self.items.insert(insertion_index, item.clone()); + cx.notify(); if activate { if insertion_index <= self.active_item_index @@ -1134,7 +1143,6 @@ impl Pane { self.activate_item(insertion_index, activate_pane, focus_item, window, cx); } - cx.notify(); } cx.emit(Event::AddItem { item }); @@ -1272,15 +1280,18 @@ impl Pane { focus_changed: focus_item, }); - if !self.is_tab_pinned(index) { - self.tab_bar_scroll_handle - .scroll_to_item(index - self.pinned_tab_count); - } - + self.update_active_tab(index); cx.notify(); } } + fn update_active_tab(&mut self, index: usize) { + if !self.is_tab_pinned(index) { + self.suppress_scroll = false; + self.tab_bar_scroll_handle.scroll_to_item(index); + } + } + fn update_history(&mut self, index: usize) { if let Some(newly_active_item) = self.items.get(index) { self.activation_history @@ -3028,6 +3039,9 @@ impl Pane { .overflow_x_scroll() .w_full() .track_scroll(&self.tab_bar_scroll_handle) + .on_scroll_wheel(cx.listener(|this, _, _, _| { + this.suppress_scroll = true; + })) .children(unpinned_tabs) .child( div() @@ -4095,7 +4109,7 @@ mod tests { use super::*; use crate::item::test::{TestItem, TestProjectItem}; - use gpui::{TestAppContext, VisualTestContext}; + use gpui::{TestAppContext, VisualTestContext, size}; use project::FakeFs; use settings::SettingsStore; use theme::LoadThemes; @@ -6310,6 +6324,42 @@ mod tests { }); } + #[gpui::test] + async fn test_new_tab_scrolls_into_view_completely(cx: &mut TestAppContext) { + // Arrange + init_test(cx); + let fs = FakeFs::new(cx.executor()); + + let project = Project::test(fs, None, cx).await; + let (workspace, cx) = + cx.add_window_view(|window, cx| Workspace::test_new(project, window, cx)); + let pane = workspace.read_with(cx, |workspace, _| workspace.active_pane().clone()); + + cx.simulate_resize(size(px(300.), px(300.))); + + add_labeled_item(&pane, "untitled", false, cx); + add_labeled_item(&pane, "untitled", false, cx); + add_labeled_item(&pane, "untitled", false, cx); + add_labeled_item(&pane, "untitled", false, cx); + // Act: this should trigger a scroll + add_labeled_item(&pane, "untitled", false, cx); + // Assert + let tab_bar_scroll_handle = + pane.update_in(cx, |pane, _window, _cx| pane.tab_bar_scroll_handle.clone()); + assert_eq!(tab_bar_scroll_handle.children_count(), 6); + let tab_bounds = cx.debug_bounds("TAB-3").unwrap(); + let new_tab_button_bounds = cx.debug_bounds("ICON-Plus").unwrap(); + let scroll_bounds = tab_bar_scroll_handle.bounds(); + let scroll_offset = tab_bar_scroll_handle.offset(); + assert!(tab_bounds.right() <= scroll_bounds.right() + scroll_offset.x); + // -39.75 is the magic number for this setup + assert_eq!(scroll_offset.x, px(-39.75)); + assert!( + !tab_bounds.intersects(&new_tab_button_bounds), + "Tab should not overlap with the new tab button, if this is failing check if there's been a redesign!" + ); + } + #[gpui::test] async fn test_close_all_items_including_pinned(cx: &mut TestAppContext) { init_test(cx);