From eb5c8a91fbce94bdad58fe7345b30da2b7de9a99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 7 Aug 2026 16:23:31 +0200 Subject: [PATCH] fix(ui): keep date/time pickers inside their dialog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- e2e/dialog-horizontal-overflow.spec.ts | 255 +++++++++++++++++++++++++ src/components/ui/date-field.tsx | 16 +- src/components/ui/date-time-field.tsx | 18 +- src/components/ui/dialog.tsx | 19 +- src/components/ui/responsive-sheet.tsx | 20 +- src/components/ui/sheet-section.tsx | 10 +- src/components/ui/time-field.tsx | 10 +- 7 files changed, 333 insertions(+), 15 deletions(-) create mode 100644 e2e/dialog-horizontal-overflow.spec.ts diff --git a/e2e/dialog-horizontal-overflow.spec.ts b/e2e/dialog-horizontal-overflow.spec.ts new file mode 100644 index 000000000..4382596e7 --- /dev/null +++ b/e2e/dialog-horizontal-overflow.spec.ts @@ -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. `` / `` wrap a text ``. 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 — ``'s trigger row, ``'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 — `` + `` + * for the editor, `` + `` 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 { + const boxes = await page.evaluate( + ([surfaceSel, bodySel]) => { + const nodes = [ + ...document.querySelectorAll(surfaceSel), + ...document.querySelectorAll(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 { + const result = await page.evaluate((surfaceSel) => { + const surface = document.querySelector(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( + "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(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/, + ); + }); +}); diff --git a/src/components/ui/date-field.tsx b/src/components/ui/date-field.tsx index 35e8d9971..9eaa01f59 100644 --- a/src/components/ui/date-field.tsx +++ b/src/components/ui/date-field.tsx @@ -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 +// ``, 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( 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} diff --git a/src/components/ui/date-time-field.tsx b/src/components/ui/date-time-field.tsx index cc34611c5..6cb58f308 100644 --- a/src/components/ui/date-time-field.tsx +++ b/src/components/ui/date-time-field.tsx @@ -110,13 +110,17 @@ export const DateTimeField = React.forwardRef<
` 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 `` 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" diff --git a/src/components/ui/dialog.tsx b/src/components/ui/dialog.tsx index 08472023c..c51604155 100644 --- a/src/components/ui/dialog.tsx +++ b/src/components/ui/dialog.tsx @@ -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 — `` 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} diff --git a/src/components/ui/responsive-sheet.tsx b/src/components/ui/responsive-sheet.tsx index 743470921..9cb7a24a7 100644 --- a/src/components/ui/responsive-sheet.tsx +++ b/src/components/ui/responsive-sheet.tsx @@ -220,7 +220,10 @@ export function ResponsiveSheet({
@@ -304,7 +307,20 @@ export function ResponsiveSheet({
`'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, )} > diff --git a/src/components/ui/sheet-section.tsx b/src/components/ui/sheet-section.tsx index bd865124a..b93246092 100644 --- a/src/components/ui/sheet-section.tsx +++ b/src/components/ui/sheet-section.tsx @@ -60,7 +60,15 @@ export function SheetSection({ > {icon ? ( ` 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( 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}