Skip to content

Commit ef59816

Browse files
bloveclaude
andauthored
fix(react): make the row-height floor follow the theme (#325)
Row heights were floored at a hard-coded 44px, written in April before the token contract existed and never revisited. Every theme states a row height per density tier, and three of the nine shipped tiers are shorter than that floor: Excel is 20/24/32 and both other themes are 40 at compact. Under those the floor won every time — an Excel grid rendered its rows at more than twice the height the theme asked for, and no override could reach it, because the number was in JavaScript rather than in the stylesheet. The number lived in four places, which is why the two halves of the system could disagree without anyone noticing: - react/row-height.ts, the floor for a MEASURED row; - react/rendering.ts, re-applied at the measure call site; - renderer-dom/row-layout-controller.ts, the default for an UNMEASURED one; - renderer-dom/create-renderer.ts, a second floor under that default, so a controller told 20 still estimated max(20, 44). Now: the surface resolves --pretable-row-height through the same store that already drives header height and density, passes it as the measured floor, and passes it to the controller as the estimate default; the estimator takes its base height from the controller instead of owning a constant; and the duplicate constant in renderer-dom is one exported definition. Two consequences worth stating: The floor is part of the row measurement key now. A density flip changes no attribute the key otherwise reads — same rows, same text, same classes — so without it every already-measured row kept its old height and only rows scrolled into view afterwards adopted the new density. The bench pins --pretable-row-height to 48px. The three comparator adapters hard-code ROW_HEIGHT = 48, and row height decides how many rows a viewport holds; pretable was quietly running at 44, and with the theme honored it would have run the Excel theme's 20 against their 48 — 2.4x the rows, in its favour on some metrics and against it on others. The pin states the control where a reader will find it. Unthemed grids are unchanged at 44px, and there is a test for that. Evidence: paired replace runs, alternating this tree and origin/main in one window (this Mac is loaded, so only the pairing is trustworthy): 8.3/8.2/8.3 vs 8.4/8.4/8.6 ms against a 20ms budget, with rendered_rows_peak, rendered_cells_peak and dom_nodes_peak identical across all six. A Chromium test asserts rows land on the theme's height and that the scroll extent agrees with them; it fails with "Expected 48, Received 44" against the old floor. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 94a44d5 commit ef59816

12 files changed

Lines changed: 399 additions & 19 deletions

File tree

‎apps/bench/src/app.css‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,23 @@
1010
@import "./styles/marketing-components.css";
1111
@import "@pretable/ui/themes/excel.css";
1212
@import "@pretable/ui/grid.css";
13+
14+
/* Row height is a comparability control, not a style choice.
15+
The three comparator adapters hard-code ROW_HEIGHT = 48 (see
16+
tanstack-adapter.tsx, mui-adapter.tsx, ag-grid-adapter.tsx), so pretable has
17+
to render at 48 too or the runs are not measuring the same work: row height
18+
decides how many rows fit a viewport, and a shorter row means MORE rows
19+
rendered, scrolled, and re-rendered per frame.
20+
Until row heights became theme-aware, pretable ignored the theme and floored
21+
every row at a hard-coded 44 — off by 4px from the comparators, and nobody
22+
could see it in the stylesheet. Now the Excel theme this app imports would
23+
genuinely apply, and Excel's rows are 20px: pretable would render 2.4x the
24+
rows of every comparator in the same viewport, in its own favour on some
25+
metrics and against it on others. Pinning the token states the control in
26+
the one place a reader of this file will look. */
27+
:root {
28+
--pretable-row-height: 48px;
29+
}
1330
@import "tailwindcss";
1431

1532
@theme inline {
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import { expect, test } from "@playwright/test";
2+
3+
/**
4+
* The unit tests for the row-height floor run in jsdom, which has no layout
5+
* engine: there the floor is the only thing that can decide a row's height,
6+
* which makes them precise about the floor and blind to everything else.
7+
*
8+
* This is the other half — a real engine, real fonts, real content — asserting
9+
* the two properties that only a browser can show:
10+
*
11+
* 1. rows land on the theme's --pretable-row-height rather than the old
12+
* hard-coded 44, and
13+
* 2. the scroll extent agrees with them, so the scrollbar is not sized from a
14+
* different number than the rows are drawn at.
15+
*/
16+
test("rows render at the theme's row height, and the scroll extent agrees", async ({
17+
page,
18+
}) => {
19+
await page.goto("/?adapter=pretable&scenario=S1&scale=dev");
20+
21+
const row = page.locator("[data-pretable-row]").first();
22+
await expect(row).toBeVisible();
23+
24+
const themeRowHeight = await page.evaluate(() =>
25+
parseFloat(
26+
getComputedStyle(document.documentElement).getPropertyValue(
27+
"--pretable-row-height",
28+
),
29+
),
30+
);
31+
// The bench pins this to match the comparators; if that pin is ever removed
32+
// the assertions below still hold, they just hold against a different number.
33+
expect(themeRowHeight).toBeGreaterThan(0);
34+
35+
const heights = await page.evaluate(() =>
36+
[...document.querySelectorAll("[data-pretable-row]")].map(
37+
(node) => node.getBoundingClientRect().height,
38+
),
39+
);
40+
expect(heights.length).toBeGreaterThan(3);
41+
// Every row, not just the first: the floor is applied per row, and a bug
42+
// that reached only the initially-measured window would still pass a
43+
// single-row check.
44+
for (const height of heights) {
45+
expect(height).toBeCloseTo(themeRowHeight, 1);
46+
}
47+
48+
// The scroll extent is built from estimates for rows nobody has measured.
49+
// It used to come from a separate constant, so a themed grid claimed one
50+
// height for its scrollbar and drew another in its rows.
51+
const { contentHeight, rowCount } = await page.evaluate(() => {
52+
const content = document.querySelector<HTMLElement>(
53+
"[data-pretable-scroll-content]",
54+
);
55+
const viewport = document.querySelector<HTMLElement>(
56+
"[data-pretable-scroll-viewport]",
57+
);
58+
return {
59+
contentHeight: content ? parseFloat(content.style.height) : NaN,
60+
rowCount: Number(viewport?.getAttribute("aria-rowcount") ?? NaN),
61+
};
62+
});
63+
expect(Number.isFinite(contentHeight)).toBe(true);
64+
expect(Number.isFinite(rowCount)).toBe(true);
65+
// aria-rowcount includes the header row; the body holds the rest.
66+
expect(contentHeight).toBeCloseTo((rowCount - 1) * themeRowHeight, 0);
67+
});

‎apps/website/content/docs/theming/density.mdx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,16 @@ Wrap your app with a `<DensityPicker density={...} onChange={...} />` and hold t
5555

5656
Three density tokens are read by the engine in JavaScript, not just by CSS:
5757

58-
- `--pretable-row-height` — used by the row virtualizer to compute `top` positions.
58+
- `--pretable-row-height` — the height every row is drawn at, and the floor a measured row is clamped to.
5959
- `--pretable-header-height` — used to position the sticky header and compute body viewport height.
6060
- `--pretable-group-panel-height` — used to lay out the drag-to-group strip above the header.
6161

6262
The engine reads all three from `<html>`'s computed style and subscribes to attribute changes on `<html>` with a `MutationObserver`. When you flip `data-density`, the grid re-renders with new heights automatically — this is internal to `<Pretable>` and `<PretableSurface>`, and needs no wiring from you.
6363

64+
> `--pretable-row-height` is a floor, not a cap. A row whose content is taller — a wrapped text column, a two-line cell presentation — is measured and drawn at its content height, and the token decides only what a row that fits gets. That is what makes a mixed grid work: uniform rows at the density you asked for, taller ones exactly where the content needs them.
65+
>
66+
> With no theme imported at all, rows fall back to 44px. That is not a fourth density tier, it is the historical default for an unthemed grid, and it differs from `getDensityHeights`'s documented 32 because the two answer different questions — one is what to draw, the other what a caller reading density into their own layout should assume. Import a theme and neither number is reachable.
67+
6468
To read the row and header heights in your own code, `@pretable/ui` exports `getDensityHeights`. See [Density helpers](/docs/grid/density-helpers).
6569

6670
## Composition with light/dark

‎packages/react/react.api.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2443,7 +2443,7 @@ export type UsePretableRowsWithIdOptions<TColumns, TRowId extends PretableRowId>
24432443
// Warning: (ae-internal-missing-underscore) The name "ɵmeasureRenderedRowHeight" should be prefixed with an underscore because the declaration is marked as @internal
24442444
//
24452445
// @internal
2446-
export function ɵmeasureRenderedRowHeight(row: HTMLElement): number;
2446+
export function ɵmeasureRenderedRowHeight(row: HTMLElement, minRowHeight?: number): number;
24472447

24482448
// Warning: (ae-internal-missing-underscore) The name "ɵROW_SELECT_COLUMN_ID" should be prefixed with an underscore because the declaration is marked as @internal
24492449
//
Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
// @vitest-environment jsdom
2+
import "@testing-library/jest-dom/vitest";
3+
import { act, cleanup, render, waitFor } from "@testing-library/react";
4+
import * as React from "react";
5+
import { afterEach, describe, expect, test } from "vitest";
6+
7+
import { createColumnHelper } from "@pretable/core";
8+
9+
import { PretableSurface } from "../pretable-surface";
10+
import { measureRenderedRowHeight } from "../row-height";
11+
12+
/**
13+
* A row's height has a floor, and until now that floor was the literal 44 —
14+
* written in April, before the token contract existed, and never revisited.
15+
* Every shipped theme states a row height per density tier, and three of the
16+
* nine tiers are shorter than 44: Excel is 20 / 24 / 32, and both pretable and
17+
* Material are 40 at compact. Under those the floor won every time, so the
18+
* theme's stated height was unreachable and Excel's rows rendered at more than
19+
* twice their intended size.
20+
*
21+
* jsdom has no layout engine — every element measures zero — so the floor is
22+
* the ONLY thing deciding a row's height here. That makes it exactly the right
23+
* environment to pin the floor, and useless for anything about content.
24+
*/
25+
26+
type Row = { id: number; name: string };
27+
28+
const column = createColumnHelper<Row>();
29+
const columns = [column.accessor("name", { type: "text" })] as const;
30+
const rows: readonly Row[] = [
31+
{ id: 1, name: "a" },
32+
{ id: 2, name: "b" },
33+
];
34+
35+
function setThemeRowHeight(value: string | null) {
36+
if (value === null) {
37+
document.documentElement.style.removeProperty("--pretable-row-height");
38+
return;
39+
}
40+
document.documentElement.style.setProperty("--pretable-row-height", value);
41+
}
42+
43+
function renderedRowHeights(container: HTMLElement): number[] {
44+
return [
45+
...container.querySelectorAll<HTMLElement>("[data-pretable-row]"),
46+
].map((row) => Number(row.getAttribute("data-pretable-row-height")));
47+
}
48+
49+
afterEach(() => {
50+
cleanup();
51+
setThemeRowHeight(null);
52+
});
53+
54+
describe("row height floor", () => {
55+
test("measureRenderedRowHeight floors at the height it is given", () => {
56+
const row = document.createElement("div");
57+
const cell = document.createElement("div");
58+
cell.setAttribute("data-pretable-cell", "");
59+
row.appendChild(cell);
60+
document.body.appendChild(row);
61+
62+
// Same empty row, two themes. A single hard-coded floor cannot produce
63+
// both of these numbers, which is the whole point.
64+
expect(measureRenderedRowHeight(row, 20)).toBe(20);
65+
expect(measureRenderedRowHeight(row, 56)).toBe(56);
66+
});
67+
68+
test("content taller than the floor still wins", () => {
69+
// The floor is a floor, not a height. Regressing it into an assignment
70+
// would clip every wrapped row, and jsdom cannot see clipping — so assert
71+
// it here, where scrollHeight is the one dimension jsdom does report.
72+
const row = document.createElement("div");
73+
const cell = document.createElement("div");
74+
cell.setAttribute("data-pretable-cell", "");
75+
Object.defineProperty(cell, "scrollHeight", { value: 90 });
76+
row.appendChild(cell);
77+
document.body.appendChild(row);
78+
79+
expect(measureRenderedRowHeight(row, 20)).toBe(90);
80+
});
81+
82+
test("a surface renders its rows at the theme's row height", async () => {
83+
setThemeRowHeight("20px");
84+
const { container } = render(
85+
<PretableSurface
86+
ariaLabel="Excel-density grid"
87+
columns={columns}
88+
getRowId={(row) => row.id}
89+
overscan={0}
90+
rows={rows}
91+
viewportHeight={200}
92+
/>,
93+
);
94+
95+
await waitFor(() => {
96+
expect(renderedRowHeights(container).length).toBeGreaterThan(0);
97+
});
98+
expect(renderedRowHeights(container)).toEqual(
99+
renderedRowHeights(container).map(() => 20),
100+
);
101+
});
102+
103+
test("with no theme, rows keep the historical 44px default", async () => {
104+
// The fallback is unchanged on purpose: an app that never imports a theme
105+
// file — and every jsdom test in this package — must render exactly as it
106+
// did before. This is the check that keeps the change surgical.
107+
const { container } = render(
108+
<PretableSurface
109+
ariaLabel="Unthemed grid"
110+
columns={columns}
111+
getRowId={(row) => row.id}
112+
overscan={0}
113+
rows={rows}
114+
viewportHeight={200}
115+
/>,
116+
);
117+
118+
await waitFor(() => {
119+
expect(renderedRowHeights(container).length).toBeGreaterThan(0);
120+
});
121+
expect(renderedRowHeights(container)).toEqual(
122+
renderedRowHeights(container).map(() => 44),
123+
);
124+
});
125+
126+
test("rows nobody has measured yet are estimated at the theme's height", async () => {
127+
// The floor governs rows the DOM has rendered. Everything below the
128+
// viewport is a guess, and the guess has its own constant — renderer-dom's
129+
// defaultRowHeight, also 44. Fixing only the floor would leave a themed
130+
// grid estimating its scroll extent at one height and measuring at
131+
// another: under Excel, a 500-row grid would claim 22000px of scroll for
132+
// 10000px of rows, and the scrollbar would visibly shrink as you scrolled.
133+
setThemeRowHeight("20px");
134+
const many: readonly Row[] = Array.from({ length: 500 }, (_, i) => ({
135+
id: i,
136+
name: `row ${i}`,
137+
}));
138+
const { container } = render(
139+
<PretableSurface
140+
ariaLabel="Estimated grid"
141+
columns={columns}
142+
getRowId={(row) => row.id}
143+
overscan={0}
144+
rows={many}
145+
viewportHeight={200}
146+
/>,
147+
);
148+
149+
await waitFor(() => {
150+
expect(renderedRowHeights(container).length).toBeGreaterThan(0);
151+
});
152+
const content = container.querySelector<HTMLElement>(
153+
"[data-pretable-scroll-content]",
154+
);
155+
expect(content).not.toBeNull();
156+
expect(content!.style.height).toBe(`${500 * 20}px`);
157+
});
158+
159+
test("a density flip re-measures rows that did not otherwise change", async () => {
160+
// The subtle one. Rows are re-measured only when their measurement key
161+
// changes, and that key is built from the row's own markup — class, style,
162+
// state attributes, cell text. A density switch changes NONE of those in
163+
// jsdom: same rows, same text, same classes. If the floor is not part of
164+
// the key, every already-measured row keeps its old height and only rows
165+
// scrolled into view afterwards pick up the new density.
166+
setThemeRowHeight("20px");
167+
const { container } = render(
168+
<PretableSurface
169+
ariaLabel="Density-switching grid"
170+
columns={columns}
171+
getRowId={(row) => row.id}
172+
overscan={0}
173+
rows={rows}
174+
viewportHeight={200}
175+
/>,
176+
);
177+
178+
await waitFor(() => {
179+
expect(renderedRowHeights(container)[0]).toBe(20);
180+
});
181+
182+
await act(async () => {
183+
setThemeRowHeight("56px");
184+
});
185+
186+
await waitFor(() => {
187+
expect(renderedRowHeights(container)).toEqual(
188+
renderedRowHeights(container).map(() => 56),
189+
);
190+
});
191+
});
192+
});

‎packages/react/src/density.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import { useCallback, useRef, useSyncExternalStore } from "react";
22

33
import { type DensityHeights, getDensityHeights } from "@pretable/ui";
44

5+
import { DEFAULT_ROW_HEIGHT } from "./rendering";
6+
57
const FALLBACK_ROW_HEIGHT = 32;
68
const FALLBACK_HEADER_HEIGHT = 36;
79

@@ -86,6 +88,23 @@ function noopSubscribe(): () => void {
8688
return () => {};
8789
}
8890

91+
/**
92+
* One-shot read of the active theme's row height, for the places that need it
93+
* before there is a component to hook from — the row-layout controller is
94+
* constructed once per row model and takes its estimate default up front.
95+
*
96+
* Falls back to {@link DEFAULT_ROW_HEIGHT}, matching the surface's measured
97+
* floor, so the estimate for an unmeasured row and the floor for a measured
98+
* one agree in every case including no-theme. They disagreed by construction
99+
* before: both were 44, so a themed grid estimated its scroll extent at one
100+
* height and measured rows at another.
101+
*
102+
* @internal
103+
*/
104+
export function getThemeRowHeight(): number {
105+
return readPx("--pretable-row-height", DEFAULT_ROW_HEIGHT);
106+
}
107+
89108
/**
90109
* Reactive resolved pixel value of one `--pretable-*` CSS variable on
91110
* `document.documentElement`, falling back when it is unset or is not a

‎packages/react/src/pretable-model.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ import {
2727
useSyncExternalStore,
2828
} from "react";
2929

30+
import { getThemeRowHeight } from "./density";
31+
3032
/** Inclusive symbolic data-row span exposed by the indexed React grid. @public */
3133
export interface PretableReactRowRange<TRowId extends PretableRowId> {
3234
readonly startRowId: TRowId;
@@ -400,6 +402,9 @@ export function usePretableModelInternal<
400402
const controller = createRowLayoutController<TRow, TRowId, TColumns>({
401403
model: internalModel,
402404
columns: initialRenderColumns,
405+
// Estimates for rows the DOM has not rendered yet. Same value the
406+
// surface floors measured rows at, so the two agree under every theme.
407+
defaultRowHeight: getThemeRowHeight(),
403408
deferActivation: true,
404409
eagerInitialRowLimit: 32,
405410
viewport: {

0 commit comments

Comments
 (0)