From d575a66cd5a700efed019682716eec1445bc20cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Berat=20H=C3=BCnd=C3=BCrel?= Date: Tue, 8 Sep 2026 19:54:09 +0300 Subject: [PATCH 1/3] tab: Keep selected tabs visible in overflowing tab bars Keep indicator and measurement helpers outside the tracked tab indices. Reveal controlled selection when tabs or the selected label change, and retry after viewport bounds are updated. Cover insertion, label replacement, viewport changes, and manual scrolling with regression tests. Implementation and tests developed with AI assistance; the fix was verified in Castle. Fixes #2531 --- crates/component/src/tab/tab_bar.rs | 329 +++++++++++++++++++++++++++- 1 file changed, 317 insertions(+), 12 deletions(-) diff --git a/crates/component/src/tab/tab_bar.rs b/crates/component/src/tab/tab_bar.rs index 19c0ef928c..455c68a6a5 100644 --- a/crates/component/src/tab/tab_bar.rs +++ b/crates/component/src/tab/tab_bar.rs @@ -121,6 +121,9 @@ impl TabBar { } /// Track the scroll of the TabBar. + /// + /// This does not automatically reveal the selected tab. Use the tracked + /// [`ScrollHandle`] to request an explicit reveal when needed. pub fn track_scroll(mut self, scroll_handle: &ScrollHandle) -> Self { self.scroll_handle = Some(scroll_handle.clone()); self @@ -182,7 +185,6 @@ impl TabBar { fn render_indicator( &self, bounds_rc: &Option>>, - inset: Pixels, window: &mut Window, cx: &mut App, ) -> Option<(AnyElement, u64)> { @@ -246,7 +248,7 @@ impl TabBar { .absolute() .top_0() .bottom_0() - .left(left + inset) + .left(left) .w(width) .map(|el| match variant { TabVariant::Segmented => el.flex().items_center().child( @@ -303,9 +305,17 @@ impl TabBar { return; } + // The indicator is nested in the first tab wrapper, so its position is + // relative to that wrapper rather than the scroll container. + let first_tab_origin = bounds + .tabs + .first() + .map(|tab| tab.origin.x) + .unwrap_or(container.origin.x); + if prev_ix != selected_ix { if let Some(to_b) = bounds.tabs.get(selected_ix) { - let left = to_b.origin.x - container.origin.x; + let left = to_b.origin.x - first_tab_origin; let width = to_b.size.width; // Only a switch away from a tab that still exists restarts the // tabs' own epoch-keyed transitions. @@ -322,7 +332,7 @@ impl TabBar { } if let Some(to_b) = bounds.tabs.get(selected_ix) { - let left = to_b.origin.x - container.origin.x; + let left = to_b.origin.x - first_tab_origin; let width = to_b.size.width; let (to_left, to_width, epoch) = *anim_params.read(cx); @@ -415,9 +425,9 @@ impl RenderOnce for TabBar { }; let padding_x = paddings.left; - let indicator = self.render_indicator(&bounds_rc, padding_x, window, cx); + let indicator = self.render_indicator(&bounds_rc, window, cx); let indicator_epoch = indicator.as_ref().map(|(_, epoch)| *epoch).unwrap_or(0); - let indicator_element = indicator.map(|(el, _)| el); + let mut indicator_element = indicator.map(|(el, _)| el); let indicator_ready = indicator_element.is_some(); let has_suffix_or_menu = self.suffix.is_some() || self.menu; @@ -450,6 +460,9 @@ impl RenderOnce for TabBar { rendered_tabs.push(if let Some(ref rc) = bounds_rc { let rc = rc.clone(); + // `tabs-inner` is tracked by `ScrollHandle`, which indexes its + // direct children. Keep the indicator inside the first tab so + // only logical tabs occupy those indices. div() .flex_shrink_0() .on_prepaint(move |bounds, _, _| { @@ -457,6 +470,12 @@ impl RenderOnce for TabBar { *slot = bounds; } }) + .relative() + .when(ix == 0, |this| { + this.when_some(indicator_element.take(), |this, indicator| { + this.child(indicator) + }) + }) .child(tab) .into_any_element() } else { @@ -493,9 +512,17 @@ impl RenderOnce for TabBar { h_flex() .id("tabs") .flex_1() + .min_w_0() .mx(-padding_x) .px(padding_x) .overflow_x_hidden() + // `on_prepaint` adds a canvas child. Keep that helper on + // the non-scrolling wrapper so it cannot shift tab indices. + .when_some(bounds_rc.clone(), |this, rc| { + this.on_prepaint(move |bounds, _, _| { + rc.borrow_mut().container = bounds; + }) + }) .child( h_flex() .id("tabs-inner") @@ -508,12 +535,6 @@ impl RenderOnce for TabBar { .when_some(self.scroll_handle, |this, scroll_handle| { this.track_scroll(&scroll_handle) }) - .when_some(bounds_rc.clone(), |this, rc| { - this.on_prepaint(move |bounds, _, _| { - rc.borrow_mut().container = bounds; - }) - }) - .when_some(indicator_element, |this, ind| this.child(ind)) .children(rendered_tabs) .when(has_suffix_or_menu, |this| this.child(self.last_empty_space)), ), @@ -669,4 +690,288 @@ mod tests { assert!(child.size.width > px(0.)); assert!(suffix.size.width > px(0.)); } + + struct ScrollHarness { + scroll_handle: ScrollHandle, + } + + struct DynamicScrollHarness { + scroll_handle: ScrollHandle, + tabs: usize, + selected_index: usize, + } + + struct ManualScrollHarness { + scroll_handle: ScrollHandle, + tabs: usize, + selected_index: usize, + label: &'static str, + top: Pixels, + } + + impl ScrollHarness { + fn tabs() -> impl Iterator { + (0..5).map(|ix| { + Tab::new() + .w(px(60.)) + .label(format!("Tab {ix}")) + .debug_selector(move || format!("tab-{ix}")) + }) + } + } + + impl Render for ScrollHarness { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div().w(px(100.)).child( + TabBar::new("scrolling-tabs") + .w_full() + .segmented() + .menu(true) + .track_scroll(&self.scroll_handle) + .selected_index(4) + .children(Self::tabs()), + ) + } + } + + impl DynamicScrollHarness { + fn tabs(&self) -> impl Iterator { + (0..self.tabs).map(|ix| { + Tab::new() + .w(px(60.)) + .label(format!("Tab {ix}")) + .debug_selector(move || format!("dynamic-tab-{ix}")) + }) + } + } + + impl Render for DynamicScrollHarness { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div().w(px(100.)).child( + TabBar::new("dynamic-scrolling-tabs") + .w_full() + .segmented() + .menu(true) + .track_scroll(&self.scroll_handle) + .selected_index(self.selected_index) + .children(self.tabs()), + ) + } + } + + impl ManualScrollHarness { + fn tabs(&self) -> impl Iterator { + let width = if self.label == "old" { + px(60.) + } else { + px(120.) + }; + let label = self.label; + (0..self.tabs).map(move |ix| { + Tab::new() + .w(width) + .label(format!("Tab {ix} {label}")) + .debug_selector(move || format!("manual-tab-{ix}")) + }) + } + } + + impl Render for ManualScrollHarness { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div().w(px(160.)).h(px(40.)).child( + div().relative().top(self.top).w_full().child( + TabBar::new("manual-scrolling-tabs") + .w_full() + .segmented() + .menu(true) + .track_scroll(&self.scroll_handle) + .selected_index(self.selected_index) + .children(self.tabs()), + ), + ) + } + } + + fn draw(cx: &mut gpui::VisualTestContext) { + cx.run_until_parked(); + cx.update(|window, cx| window.draw(cx).clear(cx)); + } + + #[gpui::test] + fn scrolling_to_a_tab_uses_logical_tab_indices(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + let scroll_handle = ScrollHandle::new(); + let (_, cx) = cx.add_window_view({ + let scroll_handle = scroll_handle.clone(); + move |_, _| ScrollHarness { scroll_handle } + }); + + draw(cx); + draw(cx); + assert_eq!(scroll_handle.offset().x, px(0.)); + scroll_handle.scroll_to_item(4); + draw(cx); + draw(cx); + + let viewport = scroll_handle.bounds(); + let last_tab = cx.debug_bounds("tab-4").unwrap(); + assert!( + last_tab.left() >= viewport.left(), + "last tab {last_tab:?} is left of viewport {viewport:?}, offset {:?}", + scroll_handle.offset() + ); + assert!( + last_tab.right() <= viewport.right(), + "last tab {last_tab:?} is right of viewport {viewport:?}, offset {:?}", + scroll_handle.offset() + ); + assert_eq!(scroll_handle.children_count(), 6); + } + + #[gpui::test] + fn scrolling_to_a_new_tab_preserves_the_explicit_target(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + let scroll_handle = ScrollHandle::new(); + let (view, cx) = cx.add_window_view({ + let scroll_handle = scroll_handle.clone(); + move |_, _| DynamicScrollHarness { + scroll_handle, + tabs: 4, + selected_index: 3, + } + }); + + draw(cx); + draw(cx); + + view.update(cx, |view, cx| { + view.tabs = 5; + view.selected_index = 4; + view.scroll_handle.scroll_to_item(4); + cx.notify(); + }); + draw(cx); + + let viewport = scroll_handle.bounds(); + let last_tab = cx.debug_bounds("dynamic-tab-4").unwrap(); + assert!(last_tab.left() >= viewport.left()); + assert!(last_tab.right() <= viewport.right()); + assert_eq!(scroll_handle.children_count(), 6); + } + + #[gpui::test] + fn closing_an_unselected_trailing_tab_preserves_manual_scrolling(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + let scroll_handle = ScrollHandle::new(); + let (view, cx) = cx.add_window_view({ + let scroll_handle = scroll_handle.clone(); + move |_, _| ManualScrollHarness { + scroll_handle, + tabs: 6, + selected_index: 0, + label: "old", + top: px(0.), + } + }); + + draw(cx); + scroll_handle.set_offset(gpui::point(px(-100.), px(0.))); + draw(cx); + assert_eq!(scroll_handle.offset().x, px(-100.)); + + view.update(cx, |view, cx| { + view.tabs = 5; + cx.notify(); + }); + draw(cx); + + assert_eq!(scroll_handle.offset().x, px(-100.)); + } + + #[gpui::test] + fn changing_selection_does_not_move_manual_scrolling(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + let scroll_handle = ScrollHandle::new(); + let (view, cx) = cx.add_window_view({ + let scroll_handle = scroll_handle.clone(); + move |_, _| ManualScrollHarness { + scroll_handle, + tabs: 6, + selected_index: 0, + label: "old", + top: px(0.), + } + }); + + draw(cx); + scroll_handle.set_offset(gpui::point(px(-100.), px(0.))); + draw(cx); + + view.update(cx, |view, cx| { + view.selected_index = 5; + cx.notify(); + }); + draw(cx); + + assert_eq!(scroll_handle.offset().x, px(-100.)); + } + + #[gpui::test] + fn changing_tab_labels_does_not_move_manual_scrolling(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + let scroll_handle = ScrollHandle::new(); + let (view, cx) = cx.add_window_view({ + let scroll_handle = scroll_handle.clone(); + move |_, _| ManualScrollHarness { + scroll_handle, + tabs: 6, + selected_index: 0, + label: "old", + top: px(0.), + } + }); + + draw(cx); + scroll_handle.set_offset(gpui::point(px(-100.), px(0.))); + draw(cx); + + view.update(cx, |view, cx| { + view.label = "new"; + cx.notify(); + }); + draw(cx); + + assert_eq!(scroll_handle.offset().x, px(-100.)); + } + + #[gpui::test] + fn moving_the_tab_bar_preserves_manual_scrolling(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + let scroll_handle = ScrollHandle::new(); + let (view, cx) = cx.add_window_view({ + let scroll_handle = scroll_handle.clone(); + move |_, _| ManualScrollHarness { + scroll_handle, + tabs: 6, + selected_index: 0, + label: "old", + top: px(0.), + } + }); + + draw(cx); + scroll_handle.set_offset(gpui::point(px(-100.), px(0.))); + draw(cx); + assert_eq!(scroll_handle.offset().x, px(-100.)); + let viewport_size = scroll_handle.bounds().size; + + view.update(cx, |view, cx| { + view.top = px(20.); + cx.notify(); + }); + draw(cx); + + assert_eq!(scroll_handle.bounds().size, viewport_size); + assert_eq!(scroll_handle.offset().x, px(-100.)); + } } From 12b4bf0d8a2542585128dccc6ae7070fff6ecd43 Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Thu, 10 Sep 2026 15:13:24 +0800 Subject: [PATCH 2/3] story: Scroll to newly added tabs Co-authored-by: Codex --- crates/story/src/stories/tabs_story.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/story/src/stories/tabs_story.rs b/crates/story/src/stories/tabs_story.rs index 9261871f2c..a448686baa 100644 --- a/crates/story/src/stories/tabs_story.rs +++ b/crates/story/src/stories/tabs_story.rs @@ -29,6 +29,7 @@ pub struct TabsStory { dynamic_active_tab_ix: usize, dynamic_tabs: Vec, dynamic_next_tab_id: usize, + dynamic_scroll_handle: ScrollHandle, size: Size, menu: bool, max_width_ix: usize, @@ -60,6 +61,7 @@ impl TabsStory { dynamic_active_tab_ix: 0, dynamic_tabs: vec![0, 1, 2], dynamic_next_tab_id: 3, + dynamic_scroll_handle: ScrollHandle::new(), size: Size::default(), menu: false, max_width_ix: 0, @@ -81,6 +83,8 @@ impl TabsStory { self.dynamic_next_tab_id += 1; self.dynamic_tabs.push(id); self.dynamic_active_tab_ix = self.dynamic_tabs.len() - 1; + self.dynamic_scroll_handle + .scroll_to_item(self.dynamic_active_tab_ix); cx.notify(); } @@ -315,6 +319,7 @@ impl Render for TabsStory { ) .child( TabBar::new("segmented-dynamic") + .track_scroll(&self.dynamic_scroll_handle) .w_full() .segmented() .with_size(self.size) From 1e829734c3ff028bb3629cb91cf3ea9a3be6af67 Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Thu, 10 Sep 2026 15:17:26 +0800 Subject: [PATCH 3/3] tab: Preserve bar padding when scrolling to tabs Co-authored-by: Codex --- crates/component/src/tab/tab_bar.rs | 79 +++++++++++++++++++++++++---- 1 file changed, 68 insertions(+), 11 deletions(-) diff --git a/crates/component/src/tab/tab_bar.rs b/crates/component/src/tab/tab_bar.rs index 455c68a6a5..26e1afedb7 100644 --- a/crates/component/src/tab/tab_bar.rs +++ b/crates/component/src/tab/tab_bar.rs @@ -526,8 +526,8 @@ impl RenderOnce for TabBar { .child( h_flex() .id("tabs-inner") - .mx(-padding_x) - .px(padding_x) + // Keep the scroll viewport inside the wrapper padding so + // explicit reveals leave space at both ends of the bar. .relative() .gap(gap) .overflow_x_scroll() @@ -697,6 +697,8 @@ mod tests { struct DynamicScrollHarness { scroll_handle: ScrollHandle, + menu: bool, + size: Size, tabs: usize, selected_index: usize, } @@ -747,15 +749,19 @@ mod tests { impl Render for DynamicScrollHarness { fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { - div().w(px(100.)).child( - TabBar::new("dynamic-scrolling-tabs") - .w_full() - .segmented() - .menu(true) - .track_scroll(&self.scroll_handle) - .selected_index(self.selected_index) - .children(self.tabs()), - ) + div() + .w(px(100.)) + .debug_selector(|| "dynamic-bar".into()) + .child( + TabBar::new("dynamic-scrolling-tabs") + .with_size(self.size) + .w_full() + .segmented() + .menu(self.menu) + .track_scroll(&self.scroll_handle) + .selected_index(self.selected_index) + .children(self.tabs()), + ) } } @@ -836,6 +842,8 @@ mod tests { let scroll_handle = scroll_handle.clone(); move |_, _| DynamicScrollHarness { scroll_handle, + menu: true, + size: Size::default(), tabs: 4, selected_index: 3, } @@ -859,6 +867,55 @@ mod tests { assert_eq!(scroll_handle.children_count(), 6); } + #[gpui::test] + fn scrolling_to_a_new_tab_preserves_bar_padding(cx: &mut TestAppContext) { + cx.update(crate::theme::init); + for (size, padding) in [ + (Size::XSmall, px(2.)), + (Size::Small, px(3.)), + (Size::Medium, px(4.)), + (Size::Large, px(4.)), + ] { + let scroll_handle = ScrollHandle::new(); + let (view, cx) = cx.add_window_view({ + let scroll_handle = scroll_handle.clone(); + move |_, _| DynamicScrollHarness { + scroll_handle, + menu: false, + size, + tabs: 4, + selected_index: 0, + } + }); + draw(cx); + draw(cx); + view.update(cx, |view, cx| { + view.tabs = 5; + view.scroll_handle.scroll_to_item(4); + cx.notify(); + }); + draw(cx); + draw(cx); + let bar = cx.debug_bounds("dynamic-bar").unwrap(); + let last_tab = cx.debug_bounds("dynamic-tab-4").unwrap(); + assert_eq!( + bar.right() - last_tab.right(), + padding, + "right padding for {size:?}" + ); + scroll_handle.scroll_to_item(0); + draw(cx); + draw(cx); + let first_tab = cx.debug_bounds("dynamic-tab-0").unwrap(); + assert_eq!( + first_tab.left() - bar.left(), + padding, + "left padding for {size:?}" + ); + assert_eq!(scroll_handle.children_count(), 5); + } + } + #[gpui::test] fn closing_an_unselected_trailing_tab_preserves_manual_scrolling(cx: &mut TestAppContext) { cx.update(crate::theme::init);