From aeb870ba9de7d9e3b5e6adae4d1fe1829ce04d4c Mon Sep 17 00:00:00 2001 From: kirillDevPro <113171057+kirillDevPro@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:15:01 +0200 Subject: [PATCH] fix(data-table): keep the last row clear of the horizontal scrollbar MoonDataTable draws its horizontal scrollbar as an absolute overlay pinned to the root's bottom edge, while the rows container is pinned to that same edge. The track therefore paints over the bottom of whichever row sits at the viewport bottom; mid-list the user scrolls past it, but at the end of the scroll range the last row can never be moved out from under the track and stays clipped. The rows container now insets by the track's own thickness whenever the track is drawn, under one shared predicate that both the overlay and the gutter call, so the two can never disagree about when the gutter exists. The track thickness becomes a single constant instead of a duplicated literal. Rendered-geometry tests pin both cases: rows end above the track when there is horizontal overflow, and reach the root bottom when there is none. --- .../moon-ui-components/src/moon/data_table.rs | 27 ++- .../src/moon/data_table/tests.rs | 186 ++++++++++++++++++ crates/moon-ui-components/src/moon/mod.rs | 1 + .../src/moon/scroll_area.rs | 31 ++- docs/component-api-baseline.json | 8 + 5 files changed, 248 insertions(+), 5 deletions(-) diff --git a/crates/moon-ui-components/src/moon/data_table.rs b/crates/moon-ui-components/src/moon/data_table.rs index 9887864..998d55d 100644 --- a/crates/moon-ui-components/src/moon/data_table.rs +++ b/crates/moon-ui-components/src/moon/data_table.rs @@ -10,7 +10,10 @@ use super::{ context_menu::MoonContextMenuWindowExt as _, dropdown::MoonMenuItem, foundation::selected_background, - scroll_area::{MoonScrollAxis, MoonScrollbarVisibility, moon_scrollbar_overlay_with_palette}, + scroll_area::{ + MOON_SCROLLBAR_TRACK, MoonScrollAxis, MoonScrollbarVisibility, + moon_horizontal_track_is_drawn, moon_scrollbar_overlay_with_palette, + }, table::{MoonTableAlign, MoonTableCell, MoonTableColumn, MoonTableRow, MoonTableStyle}, theme::MoonTheme, tokens::{MoonPalette, MoonRect, MoonTone, rgba_from}, @@ -957,6 +960,26 @@ impl RenderOnce for MoonDataTable { let viewport_width = viewport_from_scroll.max(viewport_from_state); let width_policy = self.width_policy; let horizontal_scrollbar_visibility = self.horizontal_scrollbar_visibility; + // Height the pinned horizontal track occupies, or 0.0 when no track is drawn. + // + // The track is an absolute overlay at the root's bottom edge and reserves no layout + // height (`scroll_area.rs`, the `h-track` child), so a rows area pinned to `bottom(0)` + // ends UNDER it. Mid-list that is invisible — the user scrolls past it — but at the end + // of the scroll range the last row can never be moved out from under the track, which is + // the whole bug this inset fixes. + // + // The condition is not restated here: it is the SAME function the overlay itself asks, + // on the SAME handle it is handed below, so the gutter and the track appear and vanish + // together and cannot drift apart. A table that does not overflow keeps its rows flush + // with the root bottom and gains no permanent dead band. + let horizontal_track_gutter = if moon_horizontal_track_is_drawn( + &horizontal_scroll_handle, + horizontal_scrollbar_visibility, + ) { + tokens.ui(MOON_SCROLLBAR_TRACK) + } else { + 0.0 + }; // Original column order (by key) BEFORE reordering — `render_row` returns cells in // this order. After columns are reordered (drag), the body cells must be permuted to // match, otherwise only the header moves while cell content stays in place. @@ -1437,7 +1460,7 @@ impl RenderOnce for MoonDataTable { .left(px(0.0)) .top(px(header_height)) .right(px(0.0)) - .bottom(px(0.0)) + .bottom(px(horizontal_track_gutter)) .child(rows_list), ); diff --git a/crates/moon-ui-components/src/moon/data_table/tests.rs b/crates/moon-ui-components/src/moon/data_table/tests.rs index d66b517..330e85e 100644 --- a/crates/moon-ui-components/src/moon/data_table/tests.rs +++ b/crates/moon-ui-components/src/moon/data_table/tests.rs @@ -6,7 +6,11 @@ use super::{ MIN_COLUMN_WIDTH, MoonDataCell, MoonDataRow, MoonDataTable, MoonDataTableColumn, MoonDataTableWidthPolicy, is_select_all_shortcut, }; +use crate::moon::{ + MOON_SCROLLBAR_TRACK, MoonPalette, MoonRect, MoonScrollbarVisibility, MoonTheme, +}; use gpui::{Modifiers, div}; +use std::{cell::RefCell, rc::Rc}; /// Build a test column with matching key and label. fn col(key: &str, width: f32) -> MoonDataTableColumn { @@ -281,3 +285,185 @@ fn data_row_conversion_preserves_banner_presence() { "a MoonDataRow without a banner must not create one during conversion" ); } + +/// Stable identifiers for the table roots measured by the gutter probes. +const OVERFLOW_TABLE_ID: &str = "data-table-gutter-overflow"; +const FITTED_TABLE_ID: &str = "data-table-gutter-fitted"; +const HIDDEN_TABLE_ID: &str = "data-table-gutter-hidden"; + +/// Choose the column geometry that either exceeds or fits the fixed table viewport. +#[derive(Clone, Copy)] +enum TableWidthCase { + /// Declared columns require horizontal scrolling in the 200-unit viewport. + Overflow, + /// Declared columns stay inside the same 200-unit viewport. + Fit, +} + +/// Render one fixed-size data table and retain its root box through the in-flow prepaint hook. +struct DataTableGutterHarness { + table_id: &'static str, + width_case: TableWidthCase, + visibility: MoonScrollbarVisibility, + root_bounds: Rc>>>, + rows_scroll_handle: gpui::UniformListScrollHandle, +} + +impl gpui::Render for DataTableGutterHarness { + /// Render the configured data-table geometry probe. + /// + /// Args: + /// _window: Test window hosting the table. + /// _cx: Test view context. + /// + /// Returns: + /// A fixed-size table whose root is captured after its children lay out. + fn render( + &mut self, + _window: &mut gpui::Window, + _cx: &mut gpui::Context, + ) -> impl gpui::IntoElement { + use gpui::{ParentElement as _, Styled as _}; + + let columns = match self.width_case { + TableWidthCase::Overflow => vec![col("first", 150.0), col("second", 150.0)], + TableWidthCase::Fit => vec![col("first", 80.0), col("second", 80.0)], + }; + let sink = self.root_bounds.clone(); + gpui::div() + .size_full() + .on_children_prepainted(move |bounds, _, _| *sink.borrow_mut() = bounds) + .child( + MoonDataTable::new(self.table_id, 8, |ix, _, _| { + MoonDataRow::new([ + MoonDataCell::text(format!("left-{ix}")), + MoonDataCell::text(format!("right-{ix}")), + ]) + }) + .bounds(MoonRect::new(0.0, 0.0, 200.0, 160.0)) + .columns(columns) + .track_scroll(&self.rows_scroll_handle) + .width_policy(MoonDataTableWidthPolicy::Preserve) + .horizontal_scrollbar_visibility(self.visibility), + ) + } +} + +/// Lay out a table through enough frames for its horizontal scroll handle to learn the viewport. +/// +/// Args: +/// cx: GPUI test context whose active palette has already been selected. +/// table_id: Stable table identity for the probe. +/// width_case: Whether the table columns overflow its fixed viewport. +/// visibility: Horizontal scrollbar visibility policy to exercise. +/// +/// Returns: +/// The measured root and rows-list bounds after layout has settled. +fn laid_out_data_table_bounds( + cx: &mut gpui::TestAppContext, + table_id: &'static str, + width_case: TableWidthCase, + visibility: MoonScrollbarVisibility, +) -> (gpui::Bounds, gpui::Bounds) { + let root_bounds = Rc::new(RefCell::new(Vec::new())); + let sink = root_bounds.clone(); + let rows_scroll_handle = gpui::UniformListScrollHandle::new(); + let handle_for_view = rows_scroll_handle.clone(); + let window = cx.add_window(move |_, _| DataTableGutterHarness { + table_id, + width_case, + visibility, + root_bounds: sink, + rows_scroll_handle: handle_for_view, + }); + let mut visual = gpui::VisualTestContext::from_window(window.into(), cx); + + for _ in 0..8 { + visual.update(|window, _| window.refresh()); + visual.run_until_parked(); + } + let rows = rows_scroll_handle.0.borrow().base_handle.bounds(); + assert!( + rows.size.height > gpui::px(0.0), + "data table rows must lay out before their bottom inset is measured" + ); + let roots = root_bounds.borrow(); + assert_eq!( + roots.len(), + 1, + "the geometry harness must have one table child" + ); + (roots[0], rows) +} + +/// Return the unoccupied space between the laid-out rows list and its table root's bottom edge. +/// +/// Args: +/// root: Measured table-root bounds. +/// rows: Measured virtual rows-list bounds. +/// +/// Returns: +/// The bottom inset the rows list leaves for an overlay track. +fn rows_bottom_inset( + root: gpui::Bounds, + rows: gpui::Bounds, +) -> gpui::Pixels { + (root.origin.y + root.size.height) - (rows.origin.y + rows.size.height) +} + +/// Catches restoring `.bottom(px(0.0))` in `data_table.rs:MoonDataTable::render`, which would +/// leave the final row under the visible horizontal scrollbar at the end of an overflowing table. +#[gpui::test] +fn overflowing_table_reserves_the_rendered_horizontal_track_in_both_themes( + cx: &mut gpui::TestAppContext, +) { + cx.update(crate::init); + for palette in [MoonPalette::TERMINAL, MoonPalette::LIGHT] { + cx.update(|cx| MoonTheme::global_mut(cx).palette = palette); + let expected_track = + cx.update(|cx| gpui::px(MoonTheme::active_tokens(cx).ui(MOON_SCROLLBAR_TRACK))); + let (root, rows) = laid_out_data_table_bounds( + cx, + OVERFLOW_TABLE_ID, + TableWidthCase::Overflow, + MoonScrollbarVisibility::Always, + ); + + assert_eq!( + rows_bottom_inset(root, rows), + expected_track, + "overflowing rows must stop one shared scrollbar-track height above the table bottom in {palette:?}" + ); + } +} + +/// Catches making `horizontal_track_gutter` unconditional in `data_table.rs:MoonDataTable::render`, +/// which would leave an eight-unit dead band under fitting or Hidden-scrollbar tables. +#[gpui::test] +fn tables_without_a_drawn_horizontal_track_keep_rows_flush_in_both_themes( + cx: &mut gpui::TestAppContext, +) { + cx.update(crate::init); + for palette in [MoonPalette::TERMINAL, MoonPalette::LIGHT] { + cx.update(|cx| MoonTheme::global_mut(cx).palette = palette); + for (table_id, width_case, visibility) in [ + ( + FITTED_TABLE_ID, + TableWidthCase::Fit, + MoonScrollbarVisibility::Always, + ), + ( + HIDDEN_TABLE_ID, + TableWidthCase::Overflow, + MoonScrollbarVisibility::Hidden, + ), + ] { + let (root, rows) = laid_out_data_table_bounds(cx, table_id, width_case, visibility); + assert_eq!( + rows_bottom_inset(root, rows), + gpui::px(0.0), + "rows must reach the table bottom without a drawn horizontal track in {palette:?}" + ); + } + } +} diff --git a/crates/moon-ui-components/src/moon/mod.rs b/crates/moon-ui-components/src/moon/mod.rs index 415ec75..71db466 100644 --- a/crates/moon-ui-components/src/moon/mod.rs +++ b/crates/moon-ui-components/src/moon/mod.rs @@ -155,6 +155,7 @@ pub use resizable::{ moon_h_resizable, moon_resizable_panel, moon_v_resizable, }; pub use root::{MoonRoot, MoonRoot as Root}; +pub use scroll_area::MOON_SCROLLBAR_TRACK; pub use scroll_area::{ MoonScrollAxis, MoonScrollbarVisibility, moon_scrollbar_overlay_with_palette, }; diff --git a/crates/moon-ui-components/src/moon/scroll_area.rs b/crates/moon-ui-components/src/moon/scroll_area.rs index 65e46e3..d429d1e 100644 --- a/crates/moon-ui-components/src/moon/scroll_area.rs +++ b/crates/moon-ui-components/src/moon/scroll_area.rs @@ -7,6 +7,14 @@ use super::{ tokens::{MoonPalette, rgba_from}, }; +/// Thickness of a Moon scrollbar track, in design-reference (unscaled) units. +/// +/// The single home for the number: both tracks below take their cross-axis size from it, and a +/// surface that has to keep content clear of a track — `MoonDataTable`'s rows area — insets by +/// the same value instead of restating it. Feed it to `MoonTheme::active_tokens(cx).ui(..)`, +/// never to `px()` directly, or it stops following the UI scale. +pub const MOON_SCROLLBAR_TRACK: f32 = 8.0; + #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum MoonScrollAxis { Vertical, @@ -28,6 +36,21 @@ impl MoonScrollbarVisibility { } } +/// Whether the pinned horizontal track is actually drawn for this handle. +/// +/// The overlay below asks it to decide whether to build the track; a surface that must keep its +/// content clear of that track — `MoonDataTable`'s rows area, which the track would otherwise +/// paint over at the end of the scroll range — asks the SAME function to decide whether to +/// reserve `MOON_SCROLLBAR_TRACK`. Hand-copying the predicate is what lets the two drift: a +/// third condition added here would silently stop matching a copy that lives in another file, +/// and the content would go back under the track with nothing failing. +pub(crate) fn moon_horizontal_track_is_drawn( + scroll_handle: &ScrollHandle, + visibility: MoonScrollbarVisibility, +) -> bool { + visibility.is_visible() && f32::from(scroll_handle.max_offset().x) > 0.0 +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum MoonScrollbarDragAxis { Vertical, @@ -226,7 +249,7 @@ pub fn moon_scrollbar_overlay_with_palette( .right(px(0.0)) .top(px(0.0)) .bottom(px(0.0)) - .w(px(tokens.ui(8.0))) + .w(px(tokens.ui(MOON_SCROLLBAR_TRACK))) .bg(track_alpha(runtime.vertical_alpha)) .cursor(CursorStyle::Arrow) .on_hover(move |hovered, _window, cx| { @@ -341,7 +364,9 @@ pub fn moon_scrollbar_overlay_with_palette( ); } - if matches!(axis, MoonScrollAxis::Horizontal | MoonScrollAxis::Both) && f32::from(max.x) > 0.0 { + if matches!(axis, MoonScrollAxis::Horizontal | MoonScrollAxis::Both) + && moon_horizontal_track_is_drawn(scroll_handle, visibility) + { let viewport = f32::from(bounds.size.width).max(1.0); let content = viewport + f32::from(max.x); let thumb_w = (viewport / content * viewport).clamp(18.0_f32.min(viewport), viewport); @@ -369,7 +394,7 @@ pub fn moon_scrollbar_overlay_with_palette( .left(px(0.0)) .right(px(0.0)) .bottom(px(0.0)) - .h(px(tokens.ui(8.0))) + .h(px(tokens.ui(MOON_SCROLLBAR_TRACK))) .bg(track_alpha(runtime.horizontal_alpha)) .cursor(CursorStyle::Arrow) .on_hover(move |hovered, _window, cx| { diff --git a/docs/component-api-baseline.json b/docs/component-api-baseline.json index 95e491a..0952891 100644 --- a/docs/component-api-baseline.json +++ b/docs/component-api-baseline.json @@ -2581,6 +2581,10 @@ "file": "crates/moon-ui-components/src/moon/mod.rs", "signature": "pub use root::{MoonRoot, MoonRoot as Root};" }, + { + "file": "crates/moon-ui-components/src/moon/mod.rs", + "signature": "pub use scroll_area::MOON_SCROLLBAR_TRACK;" + }, { "file": "crates/moon-ui-components/src/moon/mod.rs", "signature": "pub use scroll_area::{ MoonScrollAxis, MoonScrollbarVisibility, moon_scrollbar_overlay_with_palette, };" @@ -3093,6 +3097,10 @@ "file": "crates/moon-ui-components/src/moon/root.rs", "signature": "pub use crate::MoonRoot;" }, + { + "file": "crates/moon-ui-components/src/moon/scroll_area.rs", + "signature": "pub const MOON_SCROLLBAR_TRACK: f32 = 8.0;" + }, { "file": "crates/moon-ui-components/src/moon/scroll_area.rs", "signature": "pub enum MoonScrollAxis"