diff --git a/crates/component/src/tab/tab_bar.rs b/crates/component/src/tab/tab_bar.rs index 19c0ef928c..26e1afedb7 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,14 +512,22 @@ 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") - .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() @@ -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,345 @@ mod tests { assert!(child.size.width > px(0.)); assert!(suffix.size.width > px(0.)); } + + struct ScrollHarness { + scroll_handle: ScrollHandle, + } + + struct DynamicScrollHarness { + scroll_handle: ScrollHandle, + menu: bool, + size: Size, + 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.)) + .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()), + ) + } + } + + 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, + menu: true, + size: Size::default(), + 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 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); + 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.)); + } } 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)