From 50e7fa2a47d42061ae283413136e187ba8c70e09 Mon Sep 17 00:00:00 2001 From: Paul DeVito Date: Wed, 28 Oct 2026 01:16:28 +0200 Subject: [PATCH 1/9] test(frontend): cover the dialog layout, the header separator, the landmarks, and wide content These tests fail on main. A long ResponsiveDialog puts the title and the scrolled content under the close button, and its footer scrolls out of view. The header separator sits 6 px above the sidebar trigger. The router devtools add a contentinfo landmark. The sidebar links have no navigation landmark. A wide child at 1024 px makes the page scroll 256 px sideways. A test also tabs through a long form in the dialog and checks that the field in focus is not hidden. It passes on main and guards the sticky header and footer. --- .../components/ui/responsive-dialog.test.tsx | 157 ++++++++++++++++++ .../features/app-shell/app-shell.locators.ts | 6 + .../features/app-shell/app-shell.spec.ts | 45 +++++ .../features/app-shell/app-shell.tasks.ts | 24 ++- 4 files changed, 231 insertions(+), 1 deletion(-) create mode 100644 frontend/src/components/ui/responsive-dialog.test.tsx diff --git a/frontend/src/components/ui/responsive-dialog.test.tsx b/frontend/src/components/ui/responsive-dialog.test.tsx new file mode 100644 index 00000000..e302ed18 --- /dev/null +++ b/frontend/src/components/ui/responsive-dialog.test.tsx @@ -0,0 +1,157 @@ +import { afterEach, expect, test } from 'vitest' +import { page, userEvent } from 'vitest/browser' +import { render } from 'vitest-browser-react' + +import { + ResponsiveDialog, + ResponsiveDialogClose, + ResponsiveDialogDescription, + ResponsiveDialogFooter, + ResponsiveDialogHeader, + ResponsiveDialogTitle, +} from '@/components/ui/responsive-dialog' + +const longTitle = 'Close work order WO-2026-00042 for the north depot vehicle' +const longDescription = + 'Closing this work order records the final odometer reading, marks every open task as done, and sends a notice to ' + + 'the fleet manager. You cannot reopen a closed work order later, so check each field first.' + +afterEach(async () => { + await page.viewport(414, 896) +}) + +function LongDialog({ title, description }: { title: string; description?: string }) { + return ( + + + {title} + {description && {description}} + + {Array.from({ length: 20 }, (_, index) => ( +

+ Field {index + 1} +

+ ))} + + Close work order + + + ) +} + +function FormDialog() { + return ( + + + {longTitle} + {longDescription} + + {Array.from({ length: 20 }, (_, index) => ( + + ))} + + Cancel + Save + + + ) +} + +function isHidden(element: Element): boolean { + const { left, right, top, bottom } = element.getBoundingClientRect() + return [ + [left + 1, top + 1], + [right - 1, bottom - 1], + ].some(([x, y]) => !element.contains(document.elementFromPoint(x, y))) +} + +function closeButton(): Element { + return page.getByRole('button', { name: 'Close', exact: true }).element() +} + +function scrollParent(element: Element): Element { + let parent = element.parentElement + while (parent && getComputedStyle(parent).overflowY !== 'auto') parent = parent.parentElement + if (!parent) throw new Error('The content has no scroll container') + return parent +} + +async function openAnimationsFinished(): Promise { + await expect.element(page.getByRole('dialog')).toBeVisible() + // The modal starts its animations after it paints. + await new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(resolve))) + await Promise.all(document.getAnimations().map((animation) => animation.finished)) +} + +function paragraphTextUnderCloseButton(x: number, y: number): string | undefined { + const button = closeButton() + const shown = document.elementsFromPoint(x, y).find((element) => !button.contains(element)) + return shown?.closest('p')?.textContent ?? undefined +} + +for (const [width, height] of [ + [1280, 720], + [390, 844], +]) { + test(`at ${width}x${height} the title of a long dialog does not run under the close button`, async () => { + await page.viewport(width, height) + render() + await openAnimationsFinished() + + const button = closeButton().getBoundingClientRect() + const range = document.createRange() + range.selectNodeContents(page.getByRole('heading', { name: longTitle }).element()) + const linesUnderButton = Array.from(range.getClientRects()).filter( + (line) => line.left < button.right && button.left < line.right && line.top < button.bottom && button.top < line.bottom, + ) + expect(linesUnderButton).toEqual([]) + }) + + test(`at ${width}x${height} the content of a long dialog does not scroll under the close button`, async () => { + await page.viewport(width, height) + render() + await openAnimationsFinished() + + scrollParent(page.getByText('Field 1', { exact: true }).element()).scrollTop = 120 + + await expect.poll(() => { + const { left, right, top, bottom } = closeButton().getBoundingClientRect() + expect(closeButton().contains(document.elementFromPoint((left + right) / 2, (top + bottom) / 2))).toBe(true) + return [ + [left + 1, top + 1], + [right - 1, top + 1], + [left + 1, bottom - 1], + [right - 1, bottom - 1], + ].map(([x, y]) => paragraphTextUnderCloseButton(x, y)).filter((text) => text?.startsWith('Field ')) + }).toEqual([]) + }) + + test(`at ${width}x${height} the header and footer of a long form do not hide the field in focus`, async () => { + await page.viewport(width, height) + render() + await openAnimationsFinished() + + const hidden: string[] = [] + page.getByRole('textbox', { name: 'Field 1', exact: true }).element().focus() + for (let index = 2; index <= 20; index++) { + await userEvent.keyboard('{Tab}') + if (isHidden(document.activeElement!)) hidden.push(document.activeElement!.ariaLabel!) + } + for (let index = 19; index >= 1; index--) { + await userEvent.keyboard('{Shift>}{Tab}{/Shift}') + if (isHidden(document.activeElement!)) hidden.push(document.activeElement!.ariaLabel!) + } + expect(hidden).toEqual([]) + }) + + test(`at ${width}x${height} the footer of a long dialog stays in view`, async () => { + await page.viewport(width, height) + render() + + await expect.poll(() => { + const action = page.getByRole('button', { name: 'Close work order' }).element() + const { left, top, width: w, height: h } = action.getBoundingClientRect() + return action.contains(document.elementFromPoint(left + w / 2, top + h / 2)) + }).toBe(true) + }) +} diff --git a/frontend/tests/features/app-shell/app-shell.locators.ts b/frontend/tests/features/app-shell/app-shell.locators.ts index 77391500..cdb7bb71 100644 --- a/frontend/tests/features/app-shell/app-shell.locators.ts +++ b/frontend/tests/features/app-shell/app-shell.locators.ts @@ -4,6 +4,12 @@ export const appShell = (page: Page) => ({ aboutHeading: page.getByRole('heading', { level: 1, name: 'About' }), aboutText: page.getByText('This is a demo application built with .NET Aspire and React'), currentPage: page.getByRole('navigation', { name: 'breadcrumb' }).getByRole('link'), + contentInfoLandmarks: page.getByRole('contentinfo'), + // The About page has no separator of its own, so the one separator in main is the one of the layout header. + headerSeparator: page.getByRole('main').getByRole('separator'), + routerDevtoolsButton: page.getByRole('button', { name: 'Open TanStack Router Devtools' }), + mainNavigation: page.getByRole('navigation', { name: 'Main' }), + sidebarTrigger: page.getByRole('main').getByRole('button', { name: 'Toggle Sidebar' }), mainLandmarks: page.getByRole('main'), notFoundHeading: page.getByRole('heading', { name: 'Page not found' }), homeLink: page.getByRole('link', { name: 'Go to the home page' }), diff --git a/frontend/tests/features/app-shell/app-shell.spec.ts b/frontend/tests/features/app-shell/app-shell.spec.ts index 5959056b..9240e3ce 100644 --- a/frontend/tests/features/app-shell/app-shell.spec.ts +++ b/frontend/tests/features/app-shell/app-shell.spec.ts @@ -52,6 +52,51 @@ test.describe('the landmarks of a page', () => { }) }) +test.describe('the layout header', () => { + test.use({ viewport: { width: 1280, height: 720 } }) + + test('centers the separator on the sidebar trigger', async ({ page }) => { + const shell = appShellPage(page) + + await shell.openAbout() + + expect(await shell.verticalCenter(shell.at.headerSeparator)).toBeCloseTo(await shell.verticalCenter(shell.at.sidebarTrigger), 0) + }) +}) + +test.describe('the landmarks of the layout', () => { + test('the router devtools add no contentinfo landmark', async ({ page }) => { + const shell = appShellPage(page) + + await shell.openAbout() + + await expect(shell.at.routerDevtoolsButton).toBeVisible() + expect(await shell.at.contentInfoLandmarks.count()).toBe(0) + }) + + test('the sidebar links are in a main navigation landmark', async ({ page }) => { + const shell = appShellPage(page) + + await shell.openAbout() + + await expect(shell.at.mainNavigation.getByRole('link', { name: 'Home' })).toBeVisible() + await expect(shell.at.mainNavigation.getByRole('link', { name: 'About' })).toBeVisible() + }) +}) + +test.describe('wide content next to the open sidebar', () => { + test.use({ viewport: { width: 1024, height: 720 } }) + + test('scrolls in its own box, and the page does not scroll sideways', async ({ page }) => { + const shell = appShellPage(page) + await shell.openAbout() + + await shell.addWideContent(1600) + + expect(await shell.pageSideScrollRange()).toBe(0) + }) +}) + test.describe('an unknown address', () => { test('shows a page not found message in the app layout', async ({ page }) => { const shell = appShellPage(page) diff --git a/frontend/tests/features/app-shell/app-shell.tasks.ts b/frontend/tests/features/app-shell/app-shell.tasks.ts index ea6572d0..c1110c57 100644 --- a/frontend/tests/features/app-shell/app-shell.tasks.ts +++ b/frontend/tests/features/app-shell/app-shell.tasks.ts @@ -1,4 +1,4 @@ -import { expect, type Page } from '@playwright/test' +import { expect, type Locator, type Page } from '@playwright/test' import { weatherPaths } from '@tests/support/api/weather-api' import { appShell } from './app-shell.locators' @@ -31,6 +31,28 @@ export function appShellPage(page: Page) { await settled }, + async addWideContent(width: number) { + await at.mainLandmarks.evaluate((main, width) => { + const scroller = document.createElement('div') + scroller.className = 'overflow-x-auto' + const content = document.createElement('div') + content.style.width = `${width}px` + content.textContent = 'Wide content' + scroller.append(content) + main.append(scroller) + }, width) + }, + + async pageSideScrollRange() { + return page.evaluate(() => document.documentElement.scrollWidth - document.documentElement.clientWidth) + }, + + async verticalCenter(locator: Locator) { + const box = await locator.boundingBox() + if (!box) throw new Error('The element has no box') + return box.y + box.height / 2 + }, + async pageScrollRange() { return page.evaluate(() => document.documentElement.scrollHeight - document.documentElement.clientHeight) }, From 233059d72d95980612cddb36231128f7cb9f6f21 Mon Sep 17 00:00:00 2001 From: Paul DeVito Date: Wed, 28 Oct 2026 01:16:48 +0200 Subject: [PATCH 2/9] fix(frontend): correct the header separator, the layout landmarks, and the inset width The vertical separator had self-stretch, so in the centered header row it started at the top. It now centers on the sidebar trigger. The router devtools rendered in a footer, which is a contentinfo landmark. They now render in a div. The sidebar links had no navigation landmark. They are now in a nav named Main. SidebarInset had no min-w-0. A wide child, such as a table in a box that scrolls, made the inset grow and the page scroll sideways. The inset now shrinks to the room next to the sidebar. Closes #160 Closes #161 Closes #162 --- frontend/src/components/app-sidebar.tsx | 92 +++++++++++++------------ frontend/src/components/ui/sidebar.tsx | 2 +- frontend/src/routes/__root.tsx | 5 +- 3 files changed, 51 insertions(+), 48 deletions(-) diff --git a/frontend/src/components/app-sidebar.tsx b/frontend/src/components/app-sidebar.tsx index 5c942253..cdc25092 100644 --- a/frontend/src/components/app-sidebar.tsx +++ b/frontend/src/components/app-sidebar.tsx @@ -93,52 +93,54 @@ export function AppSidebar() { - - Navigation - - - {navItems.map((item) => { - const isActive = currentPath === item.url - return ( - - - - {item.title} - - - ) - })} + diff --git a/frontend/src/components/ui/sidebar.tsx b/frontend/src/components/ui/sidebar.tsx index f1204ea4..8a9e6feb 100644 --- a/frontend/src/components/ui/sidebar.tsx +++ b/frontend/src/components/ui/sidebar.tsx @@ -308,7 +308,7 @@ function SidebarInset({ className, ...props }: React.ComponentProps<"main">) {
@@ -76,7 +76,8 @@ function RootComponent() { - + {/* The default container is a footer, which is a contentinfo landmark. */} + ) From ba9b5947d21ebfefb904b7df902468ab9088bed4 Mon Sep 17 00:00:00 2001 From: Paul DeVito Date: Wed, 28 Oct 2026 01:16:49 +0200 Subject: [PATCH 3/9] fix(frontend): name the breadcrumb page from the route path, not the URL The breadcrumb read the page name from a map keyed by the URL path. A route with params, such as /users/$id, never matched and showed "Page". The map is now keyed by the full path of the matched route. The keys are typed by the generated route paths, so a wrong key fails the build. Refs #159 --- frontend/src/routes/__root.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frontend/src/routes/__root.tsx b/frontend/src/routes/__root.tsx index 9537ff5e..1b277c03 100644 --- a/frontend/src/routes/__root.tsx +++ b/frontend/src/routes/__root.tsx @@ -15,6 +15,7 @@ import { } from '@/components/ui/sidebar' import { AppSidebar } from '@/components/app-sidebar' import { Separator } from '@/components/ui/separator' +import type { FileRoutesByFullPath } from '@/routeTree.gen' import { Breadcrumb, BreadcrumbItem, @@ -34,7 +35,7 @@ export const Route = createRootRoute({ component: RootComponent, }) -const routeNames: Record = { +const pageNameByRoutePath: Partial> = { '/': 'Home', '/about': 'About', '/components': 'Components', @@ -44,9 +45,9 @@ const routeNames: Record = { function RootComponent() { const router = useRouter() const routerState = useRouterState() - const currentPath = routerState.location.pathname + const routePath = routerState.matches[routerState.matches.length - 1]?.fullPath ?? '' const isNotFound = routerState.matches.some((match) => match.globalNotFound) - const pageName = isNotFound ? 'Not Found' : (routeNames[currentPath] ?? 'Page') + const pageName = isNotFound ? 'Not Found' : (pageNameByRoutePath[routePath] ?? 'Page') return ( Date: Wed, 28 Oct 2026 01:16:49 +0200 Subject: [PATCH 4/9] fix(frontend): keep the ResponsiveDialog close button and footer clear of the content The close button was absolute over the content that scrolls. A long title ran under it, and the content scrolled under it. The header now sticks to the top of the content that scrolls, covers the close button, and the title leaves room for the button. The footer scrolled with the content, so a tall dialog hid its buttons. The footer now sticks to the bottom of the content that scrolls. When a field gets focus, the scroll box gets scroll padding of the header and footer heights, so the browser does not scroll the field under them. The dialog keeps its height. A close button row above the content would add 12 px on a phone and push Add Filter of the 320 x 780 date filter off the screen. Closes #152 --- .../src/components/ui/responsive-dialog.tsx | 27 ++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/ui/responsive-dialog.tsx b/frontend/src/components/ui/responsive-dialog.tsx index f0068f3c..6e28c811 100644 --- a/frontend/src/components/ui/responsive-dialog.tsx +++ b/frontend/src/components/ui/responsive-dialog.tsx @@ -90,11 +90,12 @@ function ResponsiveDialog({ {/* Drag handle for mobile */}
-
+
{children} @@ -102,7 +103,7 @@ function ResponsiveDialog({ {showCloseButton && ( @@ -115,6 +116,16 @@ function ResponsiveDialog({ ) } +// The header and the footer stick over the content that scrolls. A field that gets focus scrolls only to the edge of +// the scroll box, so the box needs scroll padding of their heights. The browser scrolls after the focus event. +function padScrollForStickyParts(event: React.FocusEvent) { + const box = event.currentTarget + const header = box.querySelector('[data-slot="responsive-dialog-header"]') + const footer = box.querySelector('[data-slot="responsive-dialog-footer"]') + box.style.scrollPaddingTop = `${header?.offsetHeight ?? 0}px` + box.style.scrollPaddingBottom = `${footer?.offsetHeight ?? 0}px` +} + function ResponsiveDialogHeader({ className, ...props @@ -122,7 +133,12 @@ function ResponsiveDialogHeader({ return (
) @@ -136,7 +152,9 @@ function ResponsiveDialogFooter({
Date: Wed, 28 Oct 2026 01:43:46 +0200 Subject: [PATCH 5/9] fix(frontend): keep the field in focus clear of the sticky dialog header and footer The scroll padding was set in the focus event. Chromium decides the scroll before that padding applies, so the first field that got focus after a scroll stayed under the header. The header and the footer now report their heights to the dialog with a ResizeObserver, and the scroll box always has scroll padding of those heights. The test now scrolls before the first Tab. The rule to put the header first and the footer last is now in their JSDoc. --- .../components/ui/responsive-dialog.test.tsx | 19 +++--- .../src/components/ui/responsive-dialog.tsx | 60 +++++++++++++------ 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/frontend/src/components/ui/responsive-dialog.test.tsx b/frontend/src/components/ui/responsive-dialog.test.tsx index e302ed18..02fbad89 100644 --- a/frontend/src/components/ui/responsive-dialog.test.tsx +++ b/frontend/src/components/ui/responsive-dialog.test.tsx @@ -131,16 +131,19 @@ for (const [width, height] of [ render() await openAnimationsFinished() + scrollParent(page.getByRole('textbox', { name: 'Field 1', exact: true }).element()).scrollTop = 40 + const focused: string[] = [] const hidden: string[] = [] - page.getByRole('textbox', { name: 'Field 1', exact: true }).element().focus() - for (let index = 2; index <= 20; index++) { - await userEvent.keyboard('{Tab}') - if (isHidden(document.activeElement!)) hidden.push(document.activeElement!.ariaLabel!) - } - for (let index = 19; index >= 1; index--) { - await userEvent.keyboard('{Shift>}{Tab}{/Shift}') - if (isHidden(document.activeElement!)) hidden.push(document.activeElement!.ariaLabel!) + async function press(keys: string) { + await userEvent.keyboard(keys) + const field = document.activeElement! + focused.push(field.ariaLabel!) + if (isHidden(field)) hidden.push(field.ariaLabel!) } + for (let index = 1; index <= 20; index++) await press('{Tab}') + for (let index = 19; index >= 1; index--) await press('{Shift>}{Tab}{/Shift}') + + expect(focused[0]).toBe('Field 1') expect(hidden).toEqual([]) }) diff --git a/frontend/src/components/ui/responsive-dialog.tsx b/frontend/src/components/ui/responsive-dialog.tsx index 6e28c811..0fba98ef 100644 --- a/frontend/src/components/ui/responsive-dialog.tsx +++ b/frontend/src/components/ui/responsive-dialog.tsx @@ -45,6 +45,32 @@ function ResponsiveDialogClose({ ) } +type ScrollPadding = { top: number; bottom: number } + +const ScrollPaddingContext = React.createContext> | null>(null) + +// The header and the footer stick over the content that scrolls. The scroll box gets scroll padding of their heights, +// so the browser does not scroll a field that gets focus under them. +function useStickyHeight(side: keyof ScrollPadding) { + const setScrollPadding = React.useContext(ScrollPaddingContext) + const ref = React.useRef(null) + + React.useLayoutEffect(() => { + const element = ref.current + if (!element || !setScrollPadding) return + const setHeight = (height: number) => + setScrollPadding((padding) => (padding[side] === height ? padding : { ...padding, [side]: height })) + const observer = new ResizeObserver(() => setHeight(element.offsetHeight)) + observer.observe(element) + return () => { + observer.disconnect() + setHeight(0) + } + }, [setScrollPadding, side]) + + return ref +} + /** * A dialog that is a bottom sheet on mobile and a centered modal on desktop. * Put it inside a `ResponsiveDialogTrigger`, or control it with `isOpen`. @@ -62,6 +88,7 @@ function ResponsiveDialog({ }) { const isMobile = useIsMobile() const [descriptionId, setDescriptionId] = React.useState() + const [scrollPadding, setScrollPadding] = React.useState({ top: 0, bottom: 0 }) return ( {/* Drag handle for mobile */}
-
+
- {children} + + {children} +
{showCloseButton && ( @@ -116,26 +148,17 @@ function ResponsiveDialog({ ) } -// The header and the footer stick over the content that scrolls. A field that gets focus scrolls only to the edge of -// the scroll box, so the box needs scroll padding of their heights. The browser scrolls after the focus event. -function padScrollForStickyParts(event: React.FocusEvent) { - const box = event.currentTarget - const header = box.querySelector('[data-slot="responsive-dialog-header"]') - const footer = box.querySelector('[data-slot="responsive-dialog-footer"]') - box.style.scrollPaddingTop = `${header?.offsetHeight ?? 0}px` - box.style.scrollPaddingBottom = `${footer?.offsetHeight ?? 0}px` -} - +/** Sticks to the top of the content that scrolls. Put it first in the dialog. */ function ResponsiveDialogHeader({ className, ...props -}: React.ComponentProps<"div">) { +}: Omit, "ref">) { return (
) { +}: Omit, "ref">) { return (
Date: Wed, 28 Oct 2026 01:43:46 +0200 Subject: [PATCH 6/9] test(frontend): check that wide content scrolls in its own box The test name said that the wide content scrolls in its own box, but the test only checked that the page does not scroll sideways. --- frontend/tests/features/app-shell/app-shell.locators.ts | 1 + frontend/tests/features/app-shell/app-shell.spec.ts | 1 + frontend/tests/features/app-shell/app-shell.tasks.ts | 6 ++++++ 3 files changed, 8 insertions(+) diff --git a/frontend/tests/features/app-shell/app-shell.locators.ts b/frontend/tests/features/app-shell/app-shell.locators.ts index cdb7bb71..f323fa08 100644 --- a/frontend/tests/features/app-shell/app-shell.locators.ts +++ b/frontend/tests/features/app-shell/app-shell.locators.ts @@ -11,6 +11,7 @@ export const appShell = (page: Page) => ({ mainNavigation: page.getByRole('navigation', { name: 'Main' }), sidebarTrigger: page.getByRole('main').getByRole('button', { name: 'Toggle Sidebar' }), mainLandmarks: page.getByRole('main'), + wideContent: page.getByRole('region', { name: 'Wide content' }), notFoundHeading: page.getByRole('heading', { name: 'Page not found' }), homeLink: page.getByRole('link', { name: 'Go to the home page' }), weatherHeading: page.getByRole('heading', { name: 'Weather Forecast' }), diff --git a/frontend/tests/features/app-shell/app-shell.spec.ts b/frontend/tests/features/app-shell/app-shell.spec.ts index 9240e3ce..bcf41ea4 100644 --- a/frontend/tests/features/app-shell/app-shell.spec.ts +++ b/frontend/tests/features/app-shell/app-shell.spec.ts @@ -93,6 +93,7 @@ test.describe('wide content next to the open sidebar', () => { await shell.addWideContent(1600) + expect(await shell.sideScrollRange(shell.at.wideContent)).toBeGreaterThan(0) expect(await shell.pageSideScrollRange()).toBe(0) }) }) diff --git a/frontend/tests/features/app-shell/app-shell.tasks.ts b/frontend/tests/features/app-shell/app-shell.tasks.ts index c1110c57..790f8483 100644 --- a/frontend/tests/features/app-shell/app-shell.tasks.ts +++ b/frontend/tests/features/app-shell/app-shell.tasks.ts @@ -35,6 +35,8 @@ export function appShellPage(page: Page) { await at.mainLandmarks.evaluate((main, width) => { const scroller = document.createElement('div') scroller.className = 'overflow-x-auto' + scroller.role = 'region' + scroller.ariaLabel = 'Wide content' const content = document.createElement('div') content.style.width = `${width}px` content.textContent = 'Wide content' @@ -47,6 +49,10 @@ export function appShellPage(page: Page) { return page.evaluate(() => document.documentElement.scrollWidth - document.documentElement.clientWidth) }, + async sideScrollRange(locator: Locator) { + return locator.evaluate((element) => element.scrollWidth - element.clientWidth) + }, + async verticalCenter(locator: Locator) { const box = await locator.boundingBox() if (!box) throw new Error('The element has no box') From 4a722e6d9a0cb268920afa7ffb969f0c4f9745f0 Mon Sep 17 00:00:00 2001 From: Paul DeVito Date: Wed, 28 Oct 2026 02:24:16 +0200 Subject: [PATCH 7/9] test(frontend): check that the dialog header hides a field that scrolls under it At 320 px, the Created Date filter showed the border and focus ring of the date field around the title after a scroll. The header covered only the content box, so the ring showed in the side gutter. On a phone the header also ended at the bottom of the title box, and the text reaches about 3 px below it. The new test scrolls a field in focus under a short title and checks that only the header shows in the row of the title. It fails until the next commit. --- .../components/ui/responsive-dialog.test.tsx | 38 +++++++++++++++++-- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/ui/responsive-dialog.test.tsx b/frontend/src/components/ui/responsive-dialog.test.tsx index 02fbad89..7db2e9c3 100644 --- a/frontend/src/components/ui/responsive-dialog.test.tsx +++ b/frontend/src/components/ui/responsive-dialog.test.tsx @@ -39,12 +39,12 @@ function LongDialog({ title, description }: { title: string; description?: strin ) } -function FormDialog() { +function FormDialog({ title, description }: { title: string; description?: string }) { return ( - {longTitle} - {longDescription} + {title} + {description && {description}} {Array.from({ length: 20 }, (_, index) => ( @@ -128,7 +128,7 @@ for (const [width, height] of [ test(`at ${width}x${height} the header and footer of a long form do not hide the field in focus`, async () => { await page.viewport(width, height) - render() + render() await openAnimationsFinished() scrollParent(page.getByRole('textbox', { name: 'Field 1', exact: true }).element()).scrollTop = 40 @@ -147,6 +147,36 @@ for (const [width, height] of [ expect(hidden).toEqual([]) }) + test(`at ${width}x${height} a field in focus that scrolls under the header does not show around the title`, async () => { + await page.viewport(width, height) + render() + await openAnimationsFinished() + + const field = page.getByRole('textbox', { name: 'Field 4', exact: true }).element() as HTMLElement + field.focus() + const box = scrollParent(field) + const title = page.getByRole('heading', { name: 'Edit filter' }).element() + box.scrollTop += field.getBoundingClientRect().top - title.getBoundingClientRect().top + expect(field.getBoundingClientRect().top).toBeCloseTo(title.getBoundingClientRect().top, 0) + + // A focus ring is 3 px wide, and the text of the title reaches about 3 px below its box. + await expect.poll(() => { + const header = title.closest('[data-slot="responsive-dialog-header"]')! + const { left } = box.getBoundingClientRect() + const right = left + box.clientWidth + const { top, bottom } = title.getBoundingClientRect() + const shown = new Set() + for (let y = top; y <= bottom + 3; y++) { + for (let x = left + 1; x < right; x += 2) { + const hit = document.elementFromPoint(x, y) + if (!hit || header.contains(hit) || closeButton().contains(hit)) continue + shown.add(hit === field ? 'the field' : hit.getAttribute('class') ?? hit.tagName) + } + } + return [...shown] + }).toEqual([]) + }) + test(`at ${width}x${height} the footer of a long dialog stays in view`, async () => { await page.viewport(width, height) render() From 9659931910ad45c5a6908b466af51441e127ef44 Mon Sep 17 00:00:00 2001 From: Paul DeVito Date: Wed, 28 Oct 2026 02:24:16 +0200 Subject: [PATCH 8/9] fix(frontend): make the sticky dialog header cover its whole row, with the same height as main The header and the footer now cover the side gutters of the content that scrolls, so a focus ring there does not show next to them. The scroll box has the rounded corners of the dialog, so the header and the footer do not cover them. The header gets 0.5rem of padding below the title, and a negative margin of the same size, so the padding does not take space. It covers the close button and the content just below the title. The min height for the close button is gone, so a short desktop dialog has the height and the title position of main again. It was 6 px taller. The filter sheet spacing test measured from the bottom of the header box. It now measures from the title, as its name says. On main the two were the same. --- .../src/components/ui/responsive-dialog.tsx | 19 +++++++++++++------ .../features/filter-demo/filter-demo.tasks.ts | 6 +++--- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/frontend/src/components/ui/responsive-dialog.tsx b/frontend/src/components/ui/responsive-dialog.tsx index 0fba98ef..24d4c606 100644 --- a/frontend/src/components/ui/responsive-dialog.tsx +++ b/frontend/src/components/ui/responsive-dialog.tsx @@ -122,8 +122,9 @@ function ResponsiveDialog({ > {/* Drag handle for mobile */}
+ {/* The rounded corners clip the sticky header and footer, which cover the full width. */}
@@ -148,7 +149,10 @@ function ResponsiveDialog({ ) } -/** Sticks to the top of the content that scrolls. Put it first in the dialog. */ +/** + * Sticks to the top of the content that scrolls. Put it first in the dialog. + * Its background covers the first 0.5rem below it. Leave a gap of at least 0.75rem, so that a focus ring there shows. + */ function ResponsiveDialogHeader({ className, ...props @@ -158,8 +162,10 @@ function ResponsiveDialogHeader({ ref={useStickyHeight("top")} data-slot="responsive-dialog-header" className={cn( - // The negative top and margin cover the p-4 of the content that scrolls. The min height covers the close button. - "sticky -top-4 z-10 -mt-4 flex flex-col gap-2 bg-background pt-4 text-center md:text-left in-data-close-button:md:min-h-9", + // The negative margins cover the p-4 of the content that scrolls, so a focus ring in the side gutter does not show + // next to the header. pb-2 covers the close button and the content just below the title. -mb-2 keeps pb-2 out of + // the layout, so the dialog is not taller. + "sticky -top-4 z-10 -mx-4 -mt-4 -mb-2 flex flex-col gap-2 bg-background px-4 pt-4 pb-2 text-center md:text-left", className )} {...props} @@ -177,8 +183,9 @@ function ResponsiveDialogFooter({ ref={useStickyHeight("bottom")} data-slot="responsive-dialog-footer" className={cn( - // The negative bottom and margin cover the p-4 of the content that scrolls. - "sticky -bottom-4 z-10 -mb-4 flex flex-col-reverse gap-2 bg-background pt-4 pb-4 sm:flex-row sm:justify-end", + // The negative margins cover the p-4 of the content that scrolls, so a focus ring in the side gutter does not show + // next to the footer. + "sticky -bottom-4 z-10 -mx-4 -mb-4 flex flex-col-reverse gap-2 bg-background px-4 pt-4 pb-4 sm:flex-row sm:justify-end", className )} {...props} diff --git a/frontend/tests/features/filter-demo/filter-demo.tasks.ts b/frontend/tests/features/filter-demo/filter-demo.tasks.ts index 24923abe..b14f0fee 100644 --- a/frontend/tests/features/filter-demo/filter-demo.tasks.ts +++ b/frontend/tests/features/filter-demo/filter-demo.tasks.ts @@ -190,13 +190,13 @@ export function filterDemoPage(page: Page) { const { width, height } = element.getBoundingClientRect() return width > 1 && height > 1 } - const headerBottom = title.parentElement!.getBoundingClientRect().bottom + const titleBottom = title.getBoundingClientRect().bottom let top = Infinity let bottom = -Infinity for (const element of sheet!.querySelectorAll('*')) { const box = element.getBoundingClientRect() const isLeaf = ['BUTTON', 'INPUT'].includes(element.tagName) || ![...element.children].some(shows) - if (!isLeaf || !shows(element) || box.top < headerBottom) continue + if (!isLeaf || !shows(element) || box.top < titleBottom) continue top = Math.min(top, box.top) bottom = Math.max(bottom, box.bottom) } @@ -205,7 +205,7 @@ export function filterDemoPage(page: Page) { scroller = scroller.parentElement } const contentEnd = scroller.getBoundingClientRect().top + scroller.clientTop - scroller.scrollTop + scroller.scrollHeight - return { top: Math.round(top - headerBottom), bottom: Math.round(contentEnd - bottom) } + return { top: Math.round(top - titleBottom), bottom: Math.round(contentEnd - bottom) } }, sheet) }, From d53a15012d8e5eea70d7fb43b26326891059cd1d Mon Sep 17 00:00:00 2001 From: Paul DeVito Date: Wed, 28 Oct 2026 02:24:16 +0200 Subject: [PATCH 9/9] refactor(frontend): let ResponsiveDialog own its bottom sheet cap ResponsiveDialog capped the sheet with a fixed 4.5rem for one toast, and index.css overrode that cap with --toast-reserve. The component now uses var(--toast-reserve, 4.5rem) and the max-height transition, and the override in index.css is gone. The drawer already works this way. --- frontend/src/components/ui/responsive-dialog.tsx | 10 ++++++---- frontend/src/index.css | 12 ------------ 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/frontend/src/components/ui/responsive-dialog.tsx b/frontend/src/components/ui/responsive-dialog.tsx index 24d4c606..dd286c22 100644 --- a/frontend/src/components/ui/responsive-dialog.tsx +++ b/frontend/src/components/ui/responsive-dialog.tsx @@ -106,10 +106,12 @@ function ResponsiveDialog({ data-side={isMobile ? "bottom" : undefined} className={cn( "fixed z-50 w-full bg-background text-sm ring-1 ring-foreground/10 outline-none", - // Mobile: bottom sheet. While it is open, index.css shows the toasts at the top: 1rem from the edge, and 3rem - // for one toast. The sheet stops 0.5rem below that, so the toast does not cover its drag handle. dvh is the visible - // height of a phone with its toolbar shown. vh is the height without the toolbar. - "right-0 bottom-0 left-0 max-h-[85vh] supports-[height:100dvh]:max-h-[min(85vh,calc(100dvh-4.5rem))] rounded-t-xl", + // Mobile: bottom sheet. While it is open, index.css shows the toasts at the top and sets --toast-reserve, the + // room of the toasts, so the sheet stops below them. The fallback is the room of one toast of one line. dvh is + // the visible height of a phone with its toolbar shown. vh is the height without the toolbar. + "right-0 bottom-0 left-0 max-h-[85vh] supports-[height:100dvh]:max-h-[min(85vh,calc(100dvh-var(--toast-reserve,4.5rem)))] rounded-t-xl", + // Move with the toast fade, not in one frame. + "max-md:motion-safe:transition-[max-height] max-md:motion-safe:duration-200 max-md:motion-safe:ease-out", "md:top-1/2 md:bottom-auto md:left-1/2 md:max-h-[calc(100vh-4rem)] md:max-w-md md:-translate-x-1/2 md:-translate-y-1/2 md:rounded-xl", className )} diff --git a/frontend/src/index.css b/frontend/src/index.css index 2769e44b..7427487d 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -243,18 +243,6 @@ /* Before any toast shows, keep room for one line, so that a short toast does not move the sheet. */ --toast-reserve: calc(1rem + max(3rem, var(--toast-region-height, 0px)) + 0.5rem); - /* ResponsiveDialog caps itself for one line of toast. This cap follows the toasts that show. */ - @supports (height: 100dvh) { - [data-slot="responsive-dialog-content"][data-side="bottom"] { - max-height: min(85vh, calc(100dvh - var(--toast-reserve))); - - /* Move with the toast fade, not in one frame. */ - @media (prefers-reduced-motion: no-preference) { - transition: max-height 200ms ease-out; - } - } - } - [data-slot="toast-region"] { top: 1rem; bottom: auto;