From adfd4e1da2342e05fef65b90623b3c16cb02636d Mon Sep 17 00:00:00 2001 From: Brian Love Date: Thu, 3 Sep 2026 17:22:40 -0700 Subject: [PATCH 1/3] fix(react): menus size to their labels, and a disabled item looks disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every popover took `popoverStyle`'s fixed 240px width. That is the dialog width — `FilterMenu`'s operator select over its value input, whose controls stretch to their container — and it made the tool panel's pin menu ("Pin left" / "Pin right" / "Unpin" / "Auto width") a mostly empty 240px box anchored at a 20px kebab, spilling past the grid and over the page beside it. The header's ⋮ ("Group by this column") and `AddGroupMenu` had the same box. Split the module's one export in two over a shared `placement()`: `popoverStyle` keeps the fixed column for the dialog and the cell editors, and `menuPopoverStyle` sizes a menu to its content between a 160px floor and the dialog's 240px. The horizontal clamp stays bound to 240 for both — a content-sized menu can only be narrower, so the right edge is safe with no measure-then-reposition pass. Two things the pin menu's own semantics needed and the stylesheet never gave it. `[data-pretable-menu-item]` had no `:disabled` rule, so the placement the column is ALREADY in — the menu's way of saying where the column is — kept the enabled color, a pointer cursor and the `:hover` highlight, reading as the one item to click. It now takes the tool pane's standard disabled treatment and the hover rule carries `:not(:disabled)`. And a `role= "separator"` now divides the one-shot placement commands from the auto-width mode bit, which stays open when toggled; `useMenuKeyboard` roves over `[data-pretable-menu-item]` only, so it is not a focus stop. Verified in a browser on a production build, not just in jsdom: the pin menu measures 160px (was 240), the disabled item computes `--pretable-text-dim` with `cursor: default` and stays transparent under a real pointer hover while an enabled sibling still highlights, the separator paints a 1px rule, the filter dialog is untouched at 240px, and Pin right / Pin left still move the column between the pinned sections. Co-Authored-By: Claude Opus 5 --- .changeset/menu-popover-sizing.md | 27 ++++++++++ .../src/__tests__/popover-position.test.ts | 51 ++++++++++++++++++- .../react/src/__tests__/tool-panel.test.tsx | 21 ++++++++ .../react/src/overlay/popover-position.ts | 46 +++++++++++++++-- packages/react/src/pretable-surface.tsx | 4 +- .../react/src/tool-panel/ColumnRowMenu.tsx | 6 +++ .../react/src/tool-panel/ColumnsSection.tsx | 4 +- .../tool-panel/grouping/GroupingSection.tsx | 4 +- packages/ui/grid.css | 23 ++++++++- 9 files changed, 175 insertions(+), 11 deletions(-) create mode 100644 .changeset/menu-popover-sizing.md diff --git a/.changeset/menu-popover-sizing.md b/.changeset/menu-popover-sizing.md new file mode 100644 index 000000000..15625fcd0 --- /dev/null +++ b/.changeset/menu-popover-sizing.md @@ -0,0 +1,27 @@ +--- +"@pretable/react": patch +"@pretable/ui": patch +--- + +The grid's list-shaped menus — the tool panel's column kebab (pin placement + +auto width), the `+ Add group` menu and the header's `⋮` — now size to their +own labels, dim the item that is already the current state, and rule off the +mode bit from the commands. + +All three shared `popoverStyle`, which stamps a fixed 240px width: the right +call for `FilterMenu`, a dialog whose form controls stretch to their container, +and wrong for a menu of four short labels, which was drawn as a mostly empty +rectangle spilling well past the grid. Menus now take `menuPopoverStyle` — +content width between a 160px floor and the dialog's 240px, still clamped +horizontally against 240 so the right edge stays safe without measuring. + +The pin menu disables the placement the column is already in, but +`[data-pretable-menu-item]` had no disabled treatment at all: the disabled item +kept the enabled color, the pointer cursor, and the hover highlight, so the one +item that cannot be chosen read as the obvious one to click. It now takes the +tool pane's standard disabled treatment (`--pretable-text-dim`, default +cursor), and the hover rule skips it. + +`ColumnRowMenu` gained a `role="separator"` between the one-shot pin commands +(which close the menu) and the auto-width checkbox (which stays open) — +`[data-pretable-menu-separator]`, styled by `grid.css`, and not a focus stop. diff --git a/packages/react/src/__tests__/popover-position.test.ts b/packages/react/src/__tests__/popover-position.test.ts index dd6ab4481..1ac2a11f7 100644 --- a/packages/react/src/__tests__/popover-position.test.ts +++ b/packages/react/src/__tests__/popover-position.test.ts @@ -1,6 +1,9 @@ import { afterEach, describe, expect, it } from "vitest"; -import { popoverStyle } from "../overlay/popover-position"; +import { + menuPopoverStyle, + popoverStyle, +} from "../overlay/popover-position"; const originalWidth = window.innerWidth; const originalHeight = window.innerHeight; @@ -102,3 +105,49 @@ describe("popoverStyle", () => { expect(popoverStyle(rect(100, 200, 120, 300)).maxHeight).toBeUndefined(); }); }); + +describe("menuPopoverStyle", () => { + it("sizes to its content instead of the dialog's fixed column", () => { + setViewport(1024, 768); + const style = menuPopoverStyle(rect(100, 200, 120, 300)); + + // The defect this exists for: a four-item pin menu drawn 240px wide. + expect(style.width).toBe("max-content"); + expect(style.maxWidth).toBe(240); + expect(popoverStyle(rect(100, 200, 120, 300)).width).toBe(240); + }); + + it("keeps a floor, so a one-word menu is still menu-shaped", () => { + setViewport(1024, 768); + expect(menuPopoverStyle(rect(100, 200, 120, 300)).minWidth).toBe(160); + }); + + it("places itself exactly as a dialog does", () => { + setViewport(1024, 768); + const anchor = rect(100, 200, 120, 300); + const { width, minWidth, maxWidth, ...placement } = + menuPopoverStyle(anchor); + const { width: dialogWidth, ...dialogPlacement } = popoverStyle(anchor); + + expect(placement).toEqual(dialogPlacement); + expect(dialogWidth).toBe(240); + expect(width).toBe("max-content"); + expect(minWidth).toBe(160); + expect(maxWidth).toBe(240); + }); + + it("clamps against the widest it could be, never past the right edge", () => { + setViewport(400, 768); + // Same clamp as the dialog: a content-sized menu can only be narrower, + // so the bound holds without measuring the rendered menu. + expect(menuPopoverStyle(rect(100, 380, 120, 400)).left).toBe(152); + }); + + it("flips upward when there is no room below", () => { + setViewport(1024, 768); + const style = menuPopoverStyle(rect(720, 200, 740, 300)); + + expect(style.top).toBeUndefined(); + expect(style.bottom).toBe(768 - 720 + 4); + }); +}); diff --git a/packages/react/src/__tests__/tool-panel.test.tsx b/packages/react/src/__tests__/tool-panel.test.tsx index 1e209652e..c09b4889b 100644 --- a/packages/react/src/__tests__/tool-panel.test.tsx +++ b/packages/react/src/__tests__/tool-panel.test.tsx @@ -820,6 +820,27 @@ describe("columns section pin menu", () => { ]); }); + it("rules off the one-shot commands from the mode-bit checkbox", () => { + const h = mountColumnsSection(); + openKebab(h.kebabFor("Bravo")!); + + const menu = document.querySelector("[data-pretable-column-menu]")!; + const separators = menu.querySelectorAll("[data-pretable-menu-separator]"); + expect(separators).toHaveLength(1); + expect(separators[0]).toHaveAttribute("role", "separator"); + + // Between the last command and the checkbox, not anywhere in the list: + // the two kinds of item are what it divides. + const children = Array.from(menu.children); + const items = h.menuItems(); + expect(children.indexOf(separators[0]!)).toBe(children.indexOf(items[2]!) + 1); + expect(children.indexOf(items[3]!)).toBe(children.indexOf(separators[0]!) + 1); + + // A separator is not an item: the roving contract queries menu items, so + // it must not become a focus stop. + expect(separators[0]).not.toHaveAttribute("data-pretable-menu-item"); + }); + it("disables the matching pin item for an already-pinned column", () => { const h = mountColumnsSection(); openKebab(h.kebabFor("Alpha")!); diff --git a/packages/react/src/overlay/popover-position.ts b/packages/react/src/overlay/popover-position.ts index 53e782161..fc126f316 100644 --- a/packages/react/src/overlay/popover-position.ts +++ b/packages/react/src/overlay/popover-position.ts @@ -1,6 +1,15 @@ import type { CSSProperties } from "react"; +/** + * The DIALOG width — one column of form controls (`FilterMenu`'s operator + * select over its value input) and the cell editors' panels. It doubles as + * the horizontal-clamp bound for every popover, menus included: clamping + * against the widest a popover can be is what makes the right edge safe + * without measuring anything. + */ const WIDTH = 240; +/** A list-shaped menu narrower than this reads as a stray chip, not a menu. */ +const MENU_MIN_WIDTH = 160; /** Gap between the anchor and the popover. */ const GAP = 4; /** Minimum breathing room kept against every viewport edge. */ @@ -9,7 +18,7 @@ const MARGIN = 8; const MIN_SPACE = 160; /** - * Fixed-position style from the anchor rect. + * Where the popover sits: `position: fixed` coordinates from the anchor rect. * * Horizontally the popover is *clamped* into the viewport (never flipped). * Vertically it opens below the anchor, and flips above it when there is not @@ -18,10 +27,14 @@ const MIN_SPACE = 160; * never has to be measured. No `max-height` is set — each popover's CSS owns * its own height cap. */ -export function popoverStyle(rect: DOMRect): CSSProperties { +function placement(rect: DOMRect): CSSProperties { const vw = typeof window !== "undefined" ? window.innerWidth : 1024; const vh = typeof window !== "undefined" ? window.innerHeight : 768; + // Clamped against WIDTH even for a content-sized menu, which can only be + // narrower: the popover is then further from the right edge than it needed + // to be, never past it. Measuring the real width would mean a layout pass + // and a second paint at a corrected position. const left = Math.min(rect.left, vw - WIDTH - MARGIN); const spaceBelow = vh - rect.bottom - GAP - MARGIN; @@ -32,7 +45,34 @@ export function popoverStyle(rect: DOMRect): CSSProperties { position: "fixed", ...(flip ? { bottom: vh - rect.top + GAP } : { top: rect.bottom + GAP }), left: Math.max(MARGIN, left), - width: WIDTH, zIndex: 50, }; } + +/** + * A DIALOG-shaped popover: a fixed {@link WIDTH} column, because the form + * controls inside it stretch to their container and a shrink-wrapped one + * would be as narrow as its widest option string. + */ +export function popoverStyle(rect: DOMRect): CSSProperties { + return { ...placement(rect), width: WIDTH }; +} + +/** + * A MENU-shaped popover: sized to its own longest label instead of the + * dialog's column. `Pin right` and `Group by this column` are ~60px and + * ~150px of text; both were drawn in a 240px box, which left an item's click + * target and its words in different halves of a mostly empty rectangle. + * + * A floor and a cap rather than free-running content width — the labels are + * caller data (a column header, for `AddGroupMenu`), so neither end can be + * left to them. + */ +export function menuPopoverStyle(rect: DOMRect): CSSProperties { + return { + ...placement(rect), + width: "max-content", + minWidth: MENU_MIN_WIDTH, + maxWidth: WIDTH, + }; +} diff --git a/packages/react/src/pretable-surface.tsx b/packages/react/src/pretable-surface.tsx index 2918f8241..df7741338 100644 --- a/packages/react/src/pretable-surface.tsx +++ b/packages/react/src/pretable-surface.tsx @@ -166,7 +166,7 @@ import { MenuButton } from "./column-menu/MenuButton"; import { FilterMenu, FunnelButton } from "./filter-menu"; import { resolveColumnOptions } from "./filter-menu/filter-operators"; import { OverlayPortal } from "./overlay/OverlayPortal"; -import { popoverStyle } from "./overlay/popover-position"; +import { menuPopoverStyle, popoverStyle } from "./overlay/popover-position"; import { useHeaderPopover } from "./overlay/useHeaderPopover"; import { useHydrated } from "./use-hydrated"; import { @@ -7843,7 +7843,7 @@ export function PretableSurface< columnId={menuOpenState.columnId} grouped={snapshot.rowGroups.includes(menuOpenState.columnId)} label={col.header ?? menuOpenState.columnId} - style={popoverStyle(menuOpenState.rect)} + style={menuPopoverStyle(menuOpenState.rect)} onClose={closePopover} onSelect={selectColumnMenuAction} /> diff --git a/packages/react/src/tool-panel/ColumnRowMenu.tsx b/packages/react/src/tool-panel/ColumnRowMenu.tsx index f8cf50138..c8bb18e88 100644 --- a/packages/react/src/tool-panel/ColumnRowMenu.tsx +++ b/packages/react/src/tool-panel/ColumnRowMenu.tsx @@ -93,6 +93,12 @@ export function ColumnRowMenu({ {messages.toolPanelPinLabel({ pinned: item.pinned })} ))} + {/* Divides the one-shot placement COMMANDS above from the mode bit + below — two kinds of item with two activation behaviors, which + without a rule between them read as one flat list of four. A real + role="separator": `useMenuKeyboard` roves over + [data-pretable-menu-item] only, so it is skipped by construction. */} +
{/* "Let the grid manage this column's width" — a mode bit over the auto-width store, NOT a fit-to-content action (spec B1/Fact 2). The check glyph trails the label so the label's position is diff --git a/packages/react/src/tool-panel/ColumnsSection.tsx b/packages/react/src/tool-panel/ColumnsSection.tsx index 611206276..6c1b0ce89 100644 --- a/packages/react/src/tool-panel/ColumnsSection.tsx +++ b/packages/react/src/tool-panel/ColumnsSection.tsx @@ -14,7 +14,7 @@ import { GROUP_COLUMN_ID } from "@pretable/core"; import { ROW_SELECT_COLUMN_ID } from "../constants"; import { CheckIcon, GripIcon, OverflowIcon } from "../icons"; import type { AutoWidthSetReader } from "../pretable-model"; -import { popoverStyle } from "../overlay/popover-position"; +import { menuPopoverStyle } from "../overlay/popover-position"; import { useHeaderPopover } from "../overlay/useHeaderPopover"; import { ColumnRowMenu } from "./ColumnRowMenu"; import type { ToolPanelColumnsMessages } from "./messages"; @@ -559,7 +559,7 @@ export function ColumnsSection({ label={open.label} pinned={open.entry.pinned ?? null} messages={messages} - style={popoverStyle(menu.rect)} + style={menuPopoverStyle(menu.rect)} onClose={closeMenu} // The menu stays open (its comment carries the checkbox-vs- // command rationale), so no pending-focus arming here: the row diff --git a/packages/react/src/tool-panel/grouping/GroupingSection.tsx b/packages/react/src/tool-panel/grouping/GroupingSection.tsx index 521a39983..a54acb16a 100644 --- a/packages/react/src/tool-panel/grouping/GroupingSection.tsx +++ b/packages/react/src/tool-panel/grouping/GroupingSection.tsx @@ -10,7 +10,7 @@ import { import type { ColumnType } from "@pretable/core"; import { CloseIcon, GripIcon } from "../../icons"; -import { popoverStyle } from "../../overlay/popover-position"; +import { menuPopoverStyle } from "../../overlay/popover-position"; import { useHeaderPopover } from "../../overlay/useHeaderPopover"; import type { GroupingSectionMessages } from "../messages"; import type { ToolDropTarget, ToolRowRect } from "../tool-panel-drop-target"; @@ -459,7 +459,7 @@ export function GroupingSection({ { applyRowGroups([...groupedIds, columnId]); diff --git a/packages/ui/grid.css b/packages/ui/grid.css index 71a5d06ca..59f09a5f6 100644 --- a/packages/ui/grid.css +++ b/packages/ui/grid.css @@ -1111,9 +1111,19 @@ text-align: left; cursor: pointer; } - :where([data-pretable-menu-item]:hover) { + :where([data-pretable-menu-item]:hover:not(:disabled)) { background: var(--pretable-selection-bg); } + /* The standard disabled treatment — dims by token and drops the pointer + affordance, exactly as the tool pane's action buttons do. Load-bearing + here rather than cosmetic: a pin menu disables the placement the column + is ALREADY in, so with the enabled items' color, the hover highlight and + a pointer cursor, the item that says what is true reads as the one thing + to click. */ + :where([data-pretable-menu-item]:disabled) { + color: var(--pretable-text-dim); + cursor: default; + } :where([data-pretable-menu-item]:focus-visible) { outline: 2px solid var(--pretable-focus-ring); outline-offset: -2px; @@ -1129,6 +1139,17 @@ justify-content: space-between; gap: 8px; } + /* Divides KINDS of item, not groups of them: above it the one-shot + commands that close the menu, below it the mode bits that stay open. + A border on a role="separator" element — the menu's own accessible + divider — rather than a border-top on the item after it, so the rule + survives that item moving or a second one joining it. */ + :where([data-pretable-menu-separator]) { + height: 0; + margin: 4px 0; + border: 0; + border-top: 1px solid var(--pretable-rule); + } /* Enum combobox listbox (cell editor) */ :where([data-pretable-enum-editor]) { From b4ceebc6f839c1418284a2acb5e2d314c527c53d Mon Sep 17 00:00:00 2001 From: Brian Love Date: Thu, 3 Sep 2026 18:35:12 -0700 Subject: [PATCH 2/3] fix(ui): every portaled surface declares its own type, ring and disabled state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pin menu's sizing fix has four siblings, all the same shape: a surface portaled to cannot inherit anything the grid declares, and the ones that got it wrong took the CONSUMER's page instead. - [data-pretable-filter-menu] declared `font: inherit`. The shorthand reads as "keep the surrounding type" and does the opposite here — it pulls the host page's size and line-height in, so the dialog drew at 16px on this site while every other popover sat at --pretable-font-size-cell, and would draw at whatever any other consumer's body font is. It now declares family, size and colour outright; [data-pretable-column-menu], which pinned family and size, gained the colour. - That dialog's operator are a stacked pair and came out 28px and 33px: Chrome forces `line-height: normal` on a select and ignores what it is given, so equal padding does not mean equal height. Both now take one explicit block-size, which is what the tool pane's identical controls already do. - Those same fields had no focus-visible rule, so they kept the UA ring — which takes the consumer's accent-color. Tabbing between the pane's copy of these controls and the dialog's changed the ring's colour, width and offset. They now take --pretable-focus-ring at the shared -2px offset. - [data-pretable-reorder-ghost] draws a copy of a header cell but declared no size or weight and took --pretable-text-cell: 16px/400 in the cell colour against a 12.5px/500 header. The label under the cursor did not match the column it came from. It now mirrors the header's trio. - The date editor's month steppers disable at the calendar's min/max month, and `[data-pretable-date-header] button:hover` had no :not(:disabled) and no :disabled rule — the menu-item defect in a second place. The reason only half of this was caught already is the guard: "portaled popovers declare the sans font themselves" checked font-family alone, which is exactly the half the filter dialog had. It now checks size and colour and rejects the shorthand, and four more guards join it — the ghost's header type, the disabled treatment, the hover guards, and the dialog's ring. All five were mutation-tested: each fails when its declaration is removed. Verified in a browser on a production build: the dialog computes 14px with the cell colour, its select and input are both 28px, both take the blue 2px/-2px ring instead of the page's orange UA ring, and the ghost computes 12.5px/500/rgb(94,94,106) — identical to the header cell beside it. Behavior intact: typing NVDA into the dialog still filters to one row, and 66 Playwright specs over the popover, header-keyboard and smoke suites pass, column-reorder drags included. Co-Authored-By: Claude Opus 5 --- .changeset/menu-popover-sizing.md | 29 ++++++ packages/ui/grid.css | 55 +++++++++-- packages/ui/src/__tests__/css-cascade.test.ts | 96 ++++++++++++++++++- 3 files changed, 168 insertions(+), 12 deletions(-) diff --git a/.changeset/menu-popover-sizing.md b/.changeset/menu-popover-sizing.md index 15625fcd0..9fc57f063 100644 --- a/.changeset/menu-popover-sizing.md +++ b/.changeset/menu-popover-sizing.md @@ -25,3 +25,32 @@ cursor), and the hover rule skips it. `ColumnRowMenu` gained a `role="separator"` between the one-shot pin commands (which close the menu) and the auto-width checkbox (which stays open) — `[data-pretable-menu-separator]`, styled by `grid.css`, and not a focus stop. + +The same pass over the rest of the portaled surfaces, which cannot inherit +anything from the grid: + +- The header's filter dialog declared `font: inherit`, a shorthand that pulls + the HOST PAGE's size and line-height in — so it drew at the consumer's body + font (16px on our own site) while every other popover sat at + `--pretable-font-size-cell`. It now declares the whole trio, as the enum + listbox and date popover already did, and `[data-pretable-column-menu]` + gained the `color` it was missing. +- The dialog's operator `` were 28px and 33px in a + stacked pair, because Chrome forces `line-height: normal` on a select and + ignores what it is given. Both now take one explicit `block-size`, the way + the tool pane's identical controls already do. +- Those fields kept the UA focus ring, which takes the CONSUMER's + `accent-color` — a different colour, width and offset from the ring on the + same controls in the tool pane. They now take `--pretable-focus-ring`. +- The column-reorder ghost is a copy of a header cell but declared neither + size nor weight and took the cell colour, so the label under the cursor was + bigger and lighter than the column it came from. It now mirrors the header. +- The date editor's month steppers disable at the calendar's min/max month + with no disabled treatment and a live hover accent — the menu items' defect + in a second place. Fixed the same way. + +`grid.css`'s "portaled popovers declare the sans font themselves" guard is why +only half of this was caught: it checked `font-family` alone. It now checks +size and colour too, rejects the `font: inherit` shorthand, and is joined by +guards for the ghost's header type, the disabled treatment, the hover +guards, and the dialog's focus ring — each mutation-tested to fail. diff --git a/packages/ui/grid.css b/packages/ui/grid.css index 59f09a5f6..a6146b3db 100644 --- a/packages/ui/grid.css +++ b/packages/ui/grid.css @@ -784,10 +784,16 @@ opacity: 0.6; z-index: 10; user-select: none; - /* Portaled into — see the filter menu below. The font-family and - color declared on the scroll viewport are no longer inherited. */ + /* Portaled into — see the filter menu below. Nothing declared on + the scroll viewport is inherited, and what this draws is a copy of a + HEADER CELL, so it takes the header's trio rather than the cell's: the + ghost was rendering the host page's body font (16px/400 against the + header's 12.5px/500 on our own site) in the cell colour, so the label + under the cursor did not match the column it came from. */ font-family: var(--pretable-font-sans); - color: var(--pretable-text-cell); + font-size: var(--pretable-font-size-header); + font-weight: 500; + color: var(--pretable-text-header); } :where([data-pretable-reorder-drop-indicator]) { position: absolute; @@ -889,10 +895,16 @@ gap: 8px; padding: 10px; box-shadow: var(--pretable-shadow-overlay); - font: inherit; - /* Portaled into , so the font-family declared on the scroll - viewport is no longer inherited — declare it here. */ + /* Portaled into , so NOTHING declared on the scroll viewport is + inherited — the whole trio has to be declared here, as every other + portaled surface does. `font: inherit` used to stand in for this and + did the opposite of what it looks like: the shorthand pulled the HOST + PAGE's size and line-height in, so the dialog rendered at whatever the + consumer's body font is (16px on our own site) while every other + popover sat at the cell size. */ font-family: var(--pretable-font-sans); + font-size: var(--pretable-font-size-cell); + color: var(--pretable-text-cell); } :where([data-pretable-filter-menu]) select, :where([data-pretable-filter-menu]) input[type="text"], @@ -900,13 +912,32 @@ :where([data-pretable-filter-menu]) input:not([type]) { width: 100%; box-sizing: border-box; - padding: 5px 7px; + /* An explicit box, like the tool pane's identical controls: Chrome forces + `line-height: normal` on a