fix(ui): keep date/time pickers inside their dialog

The add and edit surfaces could be scrolled sideways and the time picker
sat past the right edge: measurements, mood, the medication intake editor
and the cycle log sheet all showed it, so the cause is in what they share.

Two mechanisms, both in shared primitives.

DateField and TimeField wrap a text input. A flex item's min-width:auto
resolves to a content-based minimum, and for a text input that is its
intrinsic size attribute — 20 characters, about 180px. Neither field could
render below ~230px whatever width the parent offered, so the date + time
row demanded ~470px inside a 400px dialog body and pushed the clock button
58px past the edge. min-w-0 on the field box and on the overlay input
releases that floor; the row now fits at every width from 360px up.

The scrolling body only declares overflow-y. CSS promotes the other axis
from visible to auto, so every deliberate outward bleed in the tree turned
the form into a pannable surface — SheetSection's trigger row was 8px wider
than its own container for no benefit (a focus ring already draws outside
the box), and Switch's hit-target pseudo reaches 13px past the track. The
bodies and the dialog surface now state overflow-x explicitly, the section
trigger no longer bleeds, and the dialog's grid column is pinned so an
over-wide child cannot drag the surface out with it.

Measured before and after on the four reported surfaces at 360 / 390 / 430
/ 768 / 1280 / 1440: the date/time row spilled 83px on every width from
640px up and none of it remains.

An e2e spec asserts the two harms directly — the surface is not a
horizontal scroll container, and no focusable control lies outside its
visible box — with a canary that proves the reachability check can fail.
This commit is contained in:
Marc-André Bombeck committed 2026-08-07 16:23:31 +02:00
1 parent d5f176ac4d
commit eb5c8a91fb
7 files changed
+333 -15

No files matched your search

+255
View File
@@ -0,0 +1,255 @@
import { expect, test } from "./setup/test";
import { STORAGE_STATE_PATH } from "./setup/global-setup";
/**
* An add / edit dialog must not pan sideways, and nothing you have to operate
* may sit outside its visible box.
*
* A report named four surfaces — measurements add, mood add, the medication
* intake editor, the cycle log sheet — and four surfaces sharing one symptom
* is one defect in something shared, not four. Two shared mechanisms were
* behind it, and this spec pins the outcome of both rather than either
* implementation:
*
* 1. `<DateField>` / `<TimeField>` wrap a text `<input>`. A flex item's
* `min-width: auto` resolves to a content-based minimum, and for a text
* input that minimum is its intrinsic `size` (20 characters, ~180 px).
* The date + time row therefore demanded ~470 px inside a 400 px dialog
* body and put the clock button ~58 px past the right edge — the picker
* was not merely ugly, it was unreachable without scrolling sideways.
*
* 2. The body scrolls (`overflow-y-auto`), and CSS promotes the OTHER axis
* from `visible` to `auto`. So every intentional outward bleed in the
* tree — `<SheetSection>`'s trigger row, `<Switch>`'s `inset-[-13px]`
* hit-target pseudo — turned the whole form into a horizontally
* scrollable surface. That is why the desktop dialog showed two
* scrollbars while the phone sheet, whose body has padding, looked fine.
*
* The two assertions map onto the two harms and neither is a restatement of a
* class name:
*
* - `expectNoSidewaysScroll` — the surface is not a horizontal scroll
* container. Fails against mechanism 2 (the axis was promoted to `auto`).
* - `expectControlsInsideBox` — every focusable control is within the
* surface's visible content box. Fails against mechanism 1 (the clock
* button was outside it), and keeps failing if a future change clips a
* control out of sight instead of fitting it.
*
* Both widths run: the defect was desktop-only for the date/time row, and the
* phone sheet is the branch where a regression would be least visible.
*
* NOT covered here: the medication intake editor and the cycle log sheet, the
* other two reported surfaces. Both need account state the shared fixture does
* not carry (a medication with a logged dose; a profile with cycle tracking
* on), and a spec that silently measures an empty page proves nothing. They
* share the primitives asserted below — `<ResponsiveSheet>` + `<DateTimeField>`
* for the editor, `<ResponsiveSheet>` + `<SheetSection>` for the log sheet —
* so a regression in either mechanism surfaces here first.
*/
const SURFACE =
'[data-slot="responsive-sheet-content"], [data-slot="dialog-content"]';
const BODY = '[data-slot="responsive-sheet-body"]';
interface DialogCase {
name: string;
path: string;
/** Accessible name of the control that opens the dialog. */
trigger: RegExp;
/** Something inside the dialog that proves it rendered its form, not a shell. */
ready: string;
}
const DIALOGS: DialogCase[] = [
{
name: "measurements add",
path: "/measurements",
trigger: /^add$/i,
ready: '[data-slot="date-time-field"]',
},
{
name: "mood add",
path: "/mood",
trigger: /^add$/i,
ready: '[data-slot="date-time-field"]',
},
{
name: "labs add",
path: "/labs",
trigger: /^add$/i,
ready: '[data-slot="date-time-field"]',
},
];
const VIEWPORTS = [
{ label: "desktop 1280", width: 1280, height: 800 },
{ label: "phone 390", width: 390, height: 844 },
];
/**
* The surface and its scrolling body must not be horizontal scroll
* containers. Read from the COMPUTED style, because the promotion that caused
* the defect happens in the cascade and is invisible in the class list.
*/
async function expectNoSidewaysScroll(
page: import("@playwright/test").Page,
label: string,
): Promise<void> {
const boxes = await page.evaluate(
([surfaceSel, bodySel]) => {
const nodes = [
...document.querySelectorAll<HTMLElement>(surfaceSel),
...document.querySelectorAll<HTMLElement>(bodySel),
];
return nodes.map((el) => ({
slot: el.dataset.slot ?? el.tagName.toLowerCase(),
overflowX: getComputedStyle(el).overflowX,
}));
},
[SURFACE, BODY] as const,
);
expect(
boxes.length,
`${label}: no dialog surface found to measure`,
).toBeGreaterThan(0);
for (const box of boxes) {
expect(
["auto", "scroll"].includes(box.overflowX),
`${label}: ${box.slot} computes overflow-x:${box.overflowX} — the surface can be panned sideways`,
).toBe(false);
}
}
/**
* No focusable control may lie outside the surface's visible content box.
* This is the reachability claim: a picker pushed past the edge (or clipped
* out of sight by a later "fix") fails here regardless of how the layout got
* there.
*/
async function expectControlsInsideBox(
page: import("@playwright/test").Page,
label: string,
): Promise<void> {
const result = await page.evaluate((surfaceSel) => {
const surface = document.querySelector<HTMLElement>(surfaceSel);
if (!surface) return null;
const box = surface.getBoundingClientRect();
// The visible content box: `clientWidth` excludes a vertical scrollbar,
// so a control hidden behind one counts as outside.
const visibleRight = box.left + surface.clientWidth;
const controls = [
...surface.querySelectorAll<HTMLElement>(
"button, input, select, textarea, a[href], [tabindex]:not([tabindex='-1'])",
),
];
const outside = controls
.map((el) => {
const r = el.getBoundingClientRect();
return { el, r };
})
.filter(({ r }) => r.width > 0 && r.height > 0)
.filter(({ r }) => r.right > visibleRight + 1 || r.left < box.left - 1)
.map(({ el, r }) => ({
tag: el.tagName.toLowerCase(),
slot: el.dataset.slot ?? null,
label:
el.getAttribute("aria-label") ??
el.textContent?.trim().slice(0, 30) ??
"",
left: Math.round(r.left),
right: Math.round(r.right),
}));
return {
controlCount: controls.length,
surfaceLeft: Math.round(box.left),
visibleRight: Math.round(visibleRight),
outside,
};
}, SURFACE);
expect(result, `${label}: dialog surface not found`).not.toBeNull();
// Guard the guard: a dialog with no controls would pass vacuously.
expect(
result!.controlCount,
`${label}: the dialog rendered no focusable controls, so this assertion proves nothing`,
).toBeGreaterThan(2);
expect(
result!.outside,
`${label}: control(s) outside the dialog's visible box [${result!.surfaceLeft}..${result!.visibleRight}]`,
).toEqual([]);
}
test.describe("add/edit dialogs do not scroll sideways", () => {
test.use({ storageState: STORAGE_STATE_PATH });
for (const vp of VIEWPORTS) {
for (const dialog of DIALOGS) {
test(`${dialog.name} fits its surface at ${vp.label}`, async ({
page,
}) => {
await page.setViewportSize({ width: vp.width, height: vp.height });
await page.goto(dialog.path, { waitUntil: "domcontentloaded" });
await page
.getByRole("button", { name: dialog.trigger })
.first()
.click();
const surface = page.locator(SURFACE).first();
await expect(surface).toBeVisible();
// Gate on the form's own content, not the shell: the surface mounts
// before the fields do, and measuring the empty frame would pass
// against exactly the layout this spec exists to catch.
await expect(surface.locator(dialog.ready).first()).toBeVisible();
const label = `${dialog.name} @${vp.label}`;
await expectNoSidewaysScroll(page, label);
await expectControlsInsideBox(page, label);
});
}
}
/**
* The reachability assertion has to be able to FAIL, or the six tests above
* are decoration. Inject a control that deliberately sits past the right
* edge and assert the helper reports it — the same shape the time-field
* button had before the fix.
*/
test("the reachability check detects a control pushed past the edge", async ({
page,
}) => {
await page.setViewportSize({ width: 1280, height: 800 });
await page.goto("/measurements", { waitUntil: "domcontentloaded" });
await page.getByRole("button", { name: /^add$/i }).first().click();
const surface = page.locator(SURFACE).first();
await expect(surface).toBeVisible();
await expect(
surface.locator('[data-slot="date-time-field"]').first(),
).toBeVisible();
await page.evaluate((surfaceSel) => {
const el = document.querySelector<HTMLElement>(surfaceSel)!;
const probe = document.createElement("button");
probe.id = "reach-canary";
probe.textContent = "canary";
probe.style.position = "absolute";
probe.style.left = `${el.clientWidth + 40}px`;
probe.style.top = "0";
probe.style.width = "30px";
probe.style.height = "30px";
el.appendChild(probe);
}, SURFACE);
await expect(
page.locator(`${SURFACE} #reach-canary`).first(),
"the canary is not inside the dialog — the fixture failed, not the helper",
).toBeAttached();
await expect(expectControlsInsideBox(page, "canary probe")).rejects.toThrow(
/outside the dialog's visible box/,
);
});
});
+14 -2
View File
@@ -48,8 +48,16 @@ import {
// only applies to the native value text — here no native input is rendered).
const FIELD_HEIGHT_CLASSES = "min-h-11 h-11 sm:min-h-10 sm:h-10";
// `min-w-0` is load-bearing, not cosmetic. A field is almost always a flex item
// (the date/time row, a form grid cell), and a flex item's `min-width: auto`
// resolves to its content-based minimum. The content here contains a text
// `<input>`, whose intrinsic width comes from the `size` attribute's default of
// 20 characters (~180 px) — so without this the field refuses to render below
// ~230 px no matter what width the parent hands it, and the surplus spills out
// of whatever dialog or sheet it sits in. See the matching note on the overlay
// input below; both ends are needed, one alone still floors the box.
const FIELD_BASE_CLASSES =
"border-input bg-background text-foreground ring-offset-background placeholder:text-muted-foreground focus-within:ring-ring relative flex w-full items-center rounded-md border ps-3 pe-2 text-sm shadow-xs transition-[color,box-shadow] focus-within:ring-2 focus-within:ring-offset-2 focus-within:outline-none";
"border-input bg-background text-foreground ring-offset-background placeholder:text-muted-foreground focus-within:ring-ring relative flex w-full min-w-0 items-center rounded-md border ps-3 pe-2 text-sm shadow-xs transition-[color,box-shadow] focus-within:ring-2 focus-within:ring-offset-2 focus-within:outline-none";
export interface DateFieldProps {
id?: string;
@@ -182,7 +190,11 @@ export const DateField = React.forwardRef<HTMLInputElement, DateFieldProps>(
inputMode="numeric"
autoComplete="off"
data-testid={dataTestId}
className="placeholder:text-muted-foreground h-full flex-1 bg-transparent py-1 outline-none disabled:cursor-not-allowed"
// `min-w-0` releases the input's own automatic minimum size (the
// 20-character intrinsic width of a text input). Without it the
// input is the widest thing in the field and the field cannot
// shrink to the width its parent offers.
className="placeholder:text-muted-foreground h-full min-w-0 flex-1 bg-transparent py-1 outline-none disabled:cursor-not-allowed"
value={overlayValue}
placeholder={placeholder ?? formatPlaceholder(dateFormat, locale)}
disabled={disabled}
+11 -7
View File
@@ -110,13 +110,17 @@ export const DateTimeField = React.forwardRef<
<div
className={cn(
// v1.25.10 — stack the date and time halves on a narrow viewport and
// only sit them side-by-side from `sm:` up. A native `<input
// type="date">` keeps its intrinsic width on mobile WebKit and won't
// shrink below it, so the `date + w-32 time` row overran a phone-width
// sheet (the time field + clock icon spilled ~70px past the edge,
// forcing a sideways scroll on every add form). Stacked, each half
// takes the full sheet width and nothing overflows.
"flex flex-col gap-2 sm:flex-row sm:items-stretch",
// only sit them side-by-side from `sm:` up. Stacked, each half takes
// the full sheet width, which is what a phone-width sheet can give.
//
// The side-by-side row above `sm:` needs `min-w-0` on both halves as
// well, and that lives in the field primitives themselves: each field
// wraps a text `<input>` whose 20-character intrinsic width was the
// field's automatic minimum size, so `date + w-32 time` demanded
// ~470 px inside a 400 px dialog body and spilled past the edge on
// every add / edit form. `min-w-0` here lets the row itself shrink
// inside whatever grid or flex cell hosts it.
"flex min-w-0 flex-col gap-2 sm:flex-row sm:items-stretch",
className,
)}
data-slot="date-time-field"
+18 -1
View File
@@ -69,7 +69,24 @@ function DialogContent({
// viewport on short / soft-keyboard-collapsed mobile screens.
// Consumers that need a different ceiling can override via
// `className`.
"bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 fixed top-[50%] left-[50%] z-50 grid max-h-[calc(100dvh-2rem)] w-full max-w-[calc(100%-2rem)] translate-x-[-50%] translate-y-[-50%] gap-4 overflow-y-auto rounded-lg border p-6 shadow-lg duration-200 outline-none sm:max-w-lg",
//
// `grid-cols-[minmax(0,1fr)]` pins the implicit column. A bare
// `grid` gives its single column an `auto` track, which sizes to the
// widest child's max-content and grows the box past the dialog's own
// `max-w-*` — and because `overflow-y:auto` makes `overflow-x`
// compute to `auto`, that surplus becomes a horizontal scrollbar
// rather than a visible spill. `minmax(0,1fr)` caps the track at the
// dialog width so an over-wide child is contained instead of
// dragging the surface out with it.
//
// `overflow-x-clip` pairs with it: `overflow-y-auto` alone promotes
// the other axis from `visible` to `auto`, so a single over-wide
// child made the whole dialog pannable sideways. A dialog scrolls
// vertically; anything that genuinely needs sideways scroll brings
// its own `overflow-x-auto` wrapper. Consumers that set `overflow-*`
// themselves still win — `<ResponsiveSheet>` passes
// `overflow-hidden` and scrolls its body instead.
"bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 fixed top-[50%] left-[50%] z-50 grid max-h-[calc(100dvh-2rem)] w-full max-w-[calc(100%-2rem)] translate-x-[-50%] translate-y-[-50%] grid-cols-[minmax(0,1fr)] gap-4 overflow-x-clip overflow-y-auto rounded-lg border p-6 shadow-lg duration-200 outline-none sm:max-w-lg",
className,
)}
{...props}
+18 -2
View File
@@ -220,7 +220,10 @@ export function ResponsiveSheet({
<div
data-slot="responsive-sheet-body"
className={cn(
"flex min-h-0 flex-1 flex-col gap-4 overflow-y-auto p-4",
// Vertical-only scroll port — see the note on the Dialog
// branch's body below for why `overflow-y-auto` alone does not
// say that.
"flex min-h-0 flex-1 flex-col gap-4 overflow-x-clip overflow-y-auto p-4",
bodyClassName,
)}
>
@@ -304,7 +307,20 @@ export function ResponsiveSheet({
<div
data-slot="responsive-sheet-body"
className={cn(
"flex min-h-0 flex-1 flex-col gap-4 overflow-y-auto",
// `overflow-x-clip` states the invariant the surrounding comment
// already claims: this body scrolls VERTICALLY. `overflow-y-auto`
// alone does not say that — CSS promotes the other axis from
// `visible` to `auto`, so any child wider than the port turned the
// whole form into a sideways-pannable surface. That is how the
// date/time row and `<SheetSection>`'s deliberate 4 px hover bleed
// (`-mx-1 w-[calc(100%+0.5rem)]`) both produced a horizontal
// scrollbar on the desktop dialog. `clip` is not `visible`, so the
// promotion does not happen and the vertical scroll is kept.
//
// Content that genuinely needs to scroll sideways (a wide table)
// owns its own `overflow-x-auto` wrapper — that is the correct
// place for it, and it still works inside a clipped port.
"flex min-h-0 flex-1 flex-col gap-4 overflow-x-clip overflow-y-auto",
bodyClassName,
)}
>
+9 -1
View File
@@ -60,7 +60,15 @@ export function SheetSection({
>
<CollapsiblePrimitive.Trigger
data-slot="sheet-section-trigger"
className="group focus-visible:ring-ring/50 -mx-1 flex w-[calc(100%+0.5rem)] items-center gap-2 rounded-md px-1 py-1.5 text-left transition-colors focus-visible:ring-2 focus-visible:outline-none"
// `w-full`, not `-mx-1 w-[calc(100%+0.5rem)] px-1`. The old pair held
// the row's text at the section's own left edge while pushing the box
// 4 px outward on each side, purely to hold the focus ring off the
// label — but a Tailwind ring is drawn OUTSIDE the border box anyway,
// so the bleed bought nothing and made every row 8 px wider than the
// sheet body that scrolls it. In the desktop dialog, whose body has no
// horizontal padding to absorb it, that surfaced as a sideways-
// scrollable form on the cycle and mood sheets.
className="group focus-visible:ring-ring/50 flex w-full items-center gap-2 rounded-md py-1.5 text-left transition-colors focus-visible:ring-2 focus-visible:outline-none"
>
{icon ? (
<span
+8 -2
View File
@@ -38,8 +38,12 @@ import { hourCycleOptions } from "@/lib/format-locale";
const FIELD_HEIGHT_CLASSES = "min-h-11 h-11 sm:min-h-10 sm:h-10";
// `min-w-0` mirrors `<DateField>` and carries the same reason: a flex item's
// automatic minimum size would otherwise be the 20-character intrinsic width of
// the text input inside, which floors the whole field at ~230 px and overflows
// the dialog that hosts it. See the note in `date-field.tsx`.
const FIELD_BASE_CLASSES =
"border-input bg-background text-foreground ring-offset-background placeholder:text-muted-foreground focus-within:ring-ring relative flex w-full items-center rounded-md border ps-3 pe-2 text-sm shadow-xs transition-[color,box-shadow] focus-within:ring-2 focus-within:ring-offset-2 focus-within:outline-none";
"border-input bg-background text-foreground ring-offset-background placeholder:text-muted-foreground focus-within:ring-ring relative flex w-full min-w-0 items-center rounded-md border ps-3 pe-2 text-sm shadow-xs transition-[color,box-shadow] focus-within:ring-2 focus-within:ring-offset-2 focus-within:outline-none";
const TIME_RE = /^([01]\d|2[0-3]):([0-5]\d)$/;
@@ -153,7 +157,9 @@ export const TimeField = React.forwardRef<HTMLInputElement, TimeFieldProps>(
inputMode="numeric"
autoComplete="off"
data-testid={dataTestId}
className="placeholder:text-muted-foreground h-full flex-1 bg-transparent py-1 outline-none disabled:cursor-not-allowed"
// See `date-field.tsx`: without `min-w-0` the input's intrinsic
// 20-character width becomes the field's floor.
className="placeholder:text-muted-foreground h-full min-w-0 flex-1 bg-transparent py-1 outline-none disabled:cursor-not-allowed"
value={overlayValue}
placeholder={placeholder ?? (use12h ? "--:-- --" : "--:--")}
disabled={disabled}