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"