feat(#378): one Display popover, reachable at every width, with a gate that can see inside it - #465
Merged
Merged
Conversation
…d raise nine 32px targets Groundwork for `Display ▾`. That menu has to host the text-settings and colour-vision controls beside the theme switcher, and nesting either component's DaisyUI dropdown inside it would put two popovers and two competing focus traps in one control. So each panel becomes its own component and the existing nav control is that panel plus a trigger: TextSettingsPanel <- FontSizeControl renders it behind the same trigger ColorVisionPanel <- ColorblindToggle renders it behind the same trigger Nothing about the current nav changes. Both triggers, their accessible names and their popover geometry are untouched. NINE TARGETS WERE 32px, AND NO GATE COULD HAVE SEEN THEM. The S/M/L/XL size row, the 1.2/1.5/1.8 spacing row, Reset and "View all accessibility options" were all `btn-sm` — 32px against the 44px floor this repo gates on. `mobile-touch-targets.spec.ts` measures `nav button, nav label, a.btn, a.sh-btn, button.sh-btn`, and anything inside a CLOSED popover is not measured at all, so these sat under the floor indefinitely. Measured with the panel open, before and after: before after 32px S M L XL 44x44 44x46 44x44 44x52 32px 1.2 1.5 1.8 44x54 44x54 44x55 32px Reset 44x124 32px View all a11y options 44x124 `min-w-11` as well as `min-h-11`, because the floor is 44x44 and these are `flex-1`: with height alone, "L" measured 44x42 while "XL" measured 44x52 — the width follows the label text. The colour-vision `select` was 40px (DaisyUI's `btn-md`/`select` default) and is now 44px. One element still measures under: the "Assistance Mode" `<label>` at 18x256. That is a non-interactive label, not a target, and my probe flagged it only because the selector includes `label` — DaisyUI wraps the patterns checkbox in one. Left alone deliberately. Also hoisted `colorblindOptions` to a module-level `COLORBLIND_OPTIONS`. It is static data that was being rebuilt on every render, and both the panel and the trigger need it. The panel's `<select>` id comes from `useId` rather than a literal, so two mounted panels cannot collide. 23 unit tests pass across both components, type-check and eslint clean. Refs #378.
…e that can see inside it Three appearance dropdowns become one `Display ▾`: text settings, colour vision and the 34-theme list. Each of the three was `hidden lg:block`, and MEASURED, the mobile hamburger holds 13 destinations and ZERO appearance controls — so below 1024px there was no way to change the theme or the font size from the nav at all. The comment on the old font-size block claimed it was "accessible via hamburger". It was not. The new trigger is present and visible at 320 / 390 / 428 / 768 / 1024 / 1280 / 1440px — 44x45 at the narrow end, 44x103 where the label appears — and the panel opens at 288px with 16/16 gutters at 320px, no horizontal overflow anywhere. NOT A `role="menu"`. `NavGroupMenu` is right for a list of links, but a menu's children are `menuitem`s, and a `<select>`, a toggle and two button groups are not. `NavPopover` is a disclosure: `aria-expanded` on the trigger, `aria-haspopup="true"` rather than `"menu"`, and a plain `role="group"` panel. The dismiss contract — Escape closing and RESTORING FOCUS, outside-press, tabbing out — moves into a shared `useDismissable` hook so both primitives implement it once. Verified: Escape closes and returns focus to the trigger at all seven widths. Named `Display`, not "Display menu". `mobile-navigation.spec.ts` locates `[aria-label*="menu" i]`, `.first()` follows document order and nav precedes page content, so a name containing "menu" shadows the mobile hamburger — #378's own earlier regression. `aria-haspopup` already makes a screen reader announce "menu button". 34 THEME BUTTONS WERE 32px. They were `btn-sm` inside a closed popover, which is why nothing measured them; they are now 44x256. Together with the nine raised in the previous commit, that is 43 targets that were under the floor with no gate able to see them. A DUPLICATE ACCESSIBLE NAME, CAUGHT BY THE NEW TEST. `NavPopover` first put `aria-label={label}` on both the trigger and the panel, putting two nodes in the accessibility tree with one name. Playwright reported it as a strict-mode violation — `[aria-label="Display"]` resolved to 2 elements. The panel is now named by its trigger via `aria-labelledby`, which is the correct pattern, and carries a `data-testid` so tests do not have to guess. THE GATE NOW OPENS THE POPOVER, which #378 required in this PR rather than later. `the Display popover is reachable at every width and its controls meet 44px` sweeps six widths, asserts the trigger is VISIBLE at each (a breakpoint-hidden appearance control being the defect), opens it, and measures every control on both axes — 44x44, not 44 tall, because these are `flex-1` and width follows the label. It carries a per-width coverage floor of 40 controls, since a panel that rendered empty would otherwise pass silently. Mutation-checked: reverting the theme buttons to `btn-sm` makes it report "34 Display controls under 44px at 320px" and fail. ALSO FIXED A FLAKY ASSERTION IN THAT SPEC, which my change exposed rather than caused. `the Demos menu is keyboard operable` clicked the trigger and immediately asserted `aria-expanded === 'true'`. A click before React attaches its handler is silently swallowed, and the server already renders `aria-expanded="false"`, so the preceding assertion passes instantly against an inert button — the test was timing-dependent on hydration. Measured: identical code passed one run and failed the next. It now retries the click until it takes, and only while the menu is closed so retrying cannot toggle it shut. `body[data-theme]` is not a usable hydration marker, for the record: ThemeScript sets it pre-paint at 251ms, long before any handler exists. One measurement I nearly reported as a 10x hydration regression was the dev server's compile time landing in `domcontentloaded` — 5788ms cold against main's 563ms warm. With a warm-up load the numbers are 1373ms and 1443ms, i.e. no regression. Comparing a freshly-edited bundle against a cached one measures the compiler. 23 unit tests pass, type-check and eslint clean. Refs #378.
…x specs drove a trigger that no longer exists Two regressions from folding the appearance controls into `Display ▾`, both found by CI rather than by me. THE SAVED COLOUR-VISION CORRECTION STOPPED BEING APPLIED AT ALL. `useColorblindMode`'s mount effect reads localStorage and writes `document.documentElement.style.filter`. It only ever ran because the nav ALWAYS mounted `ColorblindToggle` — `hidden lg:block` is CSS, so the component was mounted at every width even when invisible. `NavPopover` renders its children only while it is open, so the effect stopped running on page load: a user who had saved a protanopia correction got no correction until they opened a menu. That is worse than the defect this ticket set out to fix. Applying a saved accessibility preference cannot depend on a menu being open. The effect now lives in `ColorblindFilters`, which layout.tsx mounts on every page and which already IS the representative of this feature — it renders the SVG filter defs the mode refers to. Two mounted instances are safe: the hook derives everything from localStorage and writes the same value idempotently. Caught by tests/e2e/colorblind-fixed.spec.ts, whose docstring explains why it cannot be a unit test — the original #305 bug was a containing-block change, and jsdom has no layout, so eight unit tests asserting the property was SET passed for the entire life of that bug. Mutation-checked here: removing the effect makes that spec fail again. SIX SPECS DROVE THE REMOVED TRIGGER. tests/e2e/accessibility/colorblind-toggle.spec.ts located `getByRole('button', {name: /color vision/i})` six times. Retargeted at `Display ▾` via one `openDisplay` helper; every assertion below the trigger is unchanged, because the controls keep their accessible names — the heading "Color Vision Assistance" and the mode combobox are both still there. This is coverage moved, not coverage deleted. Two improvements while there. The Escape test now also asserts focus returns to the trigger, which is the entire reason `NavPopover` exists rather than a DaisyUI `:focus-within` dropdown. And the outside-click test presses a fixed point in the left margin instead of `page.click('main')`: Playwright scrolls an element into view before clicking, `main` is taller than the viewport, and scrolling to its centre can bring the panel — `absolute end-0` in a `sticky` header — over the click point, which times out waiting for it to be actionable. WHAT I SHOULD HAVE DONE. Before removing a nav control I grepped tests/e2e for CLASS assertions and for accessible-name collisions. I did not grep for specs that DRIVE the control I was deleting, and I did not ask what else its mere presence was doing. A component mounted for its side effect looks like decoration right up until you remove it. Locally: 8/8 on the two colourblind specs. The wider a11y set is 14 failed / 23 passed on this branch against 15 failed / 22 passed on main under the same local harness — those are the known bare-`/`-serves-the-404-page artifacts, and this change turns one of main's failures into a pass. Refs #378.
TortoiseWolfe
force-pushed
the
feat/378-display-panels
branch
from
July 30, 2026 07:44
03cbf78 to
0b6941d
Compare
This was referenced Jul 30, 2026
TortoiseWolfe
added a commit
that referenced
this pull request
Jul 30, 2026
…mobile nav's 26px items, and the Display popover's proportions (#472) * fix(#373): remove the frame's phantom 56px, its forced scrollbars, and its scroll-container capture Three sections of #373's content-frame group. §A1 was already fixed (see #463); this is §A2, §A3 and §A4. §A2 — `pb-14` on the frame reserved 56px for chrome that CookieConsent already spaces for itself with its own `h-14`. Both came from the same commit, so it was duplicate compensation for one bug. It was wrong in both states: 112px reserved for a 68px banner while it showed, and 56px of empty `bg-base-200` left above the footer forever after dismissal, because the banner's own spacer unmounts with the component and this one did not. measured, per page, below the footer with banner 56px (CookieConsent's own spacer, correct) dismissed 0px (was 56px) §A3 — `min-h-screen` on ten elements inside the frame. The frame is `flex-1` inside `body.min-h-screen.flex.flex-col`, so 100vh on a descendant double-counts the nav and footer. All ten become `min-h-full`, which is what /docs and /status already did correctly. `body` keeps its own `min-h-screen`. 404 at 1440x900 scrollHeight 1141 -> 900 against a 900 clientHeight 241px of forced scroll -> none §A4 — `overflow-hidden` -> `overflow-x-clip`. An `overflow: hidden` box is a scroll container and becomes the nearest scrollport for every `position: sticky` descendant; because the frame is sized to its content it never scrolls, so sticky had nothing to stick to. `overflow-x: clip` does not establish a scroll container. Not `overflow-y-hidden`: per CSS Overflow 3, `overflow-y: hidden` with `overflow-x: visible` computes `overflow-x` to `auto`, which would keep the box a scroll container in both axes and add a horizontal scrollbar. Verified the capture is gone — walking the ancestor chain of the sticky element on /schedule and /game now returns ZERO overflow ancestors, where before it returned the frame. Also re-checked /messages, which is what `overflow-hidden` was originally added for: `documentScrolls=false`, zero inner scrollers, so no double scrollbar returns. Its own container moved to `position: fixed` since, which is why the clip is redundant there now. BUT §A4 DOES NOT MAKE THOSE TWO PAGES' STICKY WORK, AND I AM NOT CLAIMING IT DOES. Sticky also needs travel inside its containing block, and all three call sites have none: /schedule @1440 sticky 1505px in a 1505px <aside> travel 0px /game @1440 sticky 440px in a 450px parent travel 10px /game @ 390 sticky 398px in a 398px parent travel 0px On /schedule both grid columns stretch to a 1505px row and the sidebar's own content sums to ~1449px, so the sidebar is as tall as the row it sits in — there is nothing for it to slide against. Sampled while scrolling, its viewport-top still goes 113 -> -87 -> -287 -> -687px, exactly the symptom §A4 describes. So the overflow fix is necessary and app-wide — it unblocks every sticky descendant, present and future — and it is not sufficient for these two. The second cause is recorded in #373's body with the numbers. §A5 is deliberately not in this commit: it edits GlobalNav.tsx, which two open PRs already touch. Type-check clean. Refs #373. * fix(#373 §B2): one breakpoint number, and a validator that actually runs `pnpm validate:breakpoints` existed, already failed, and was wired into no workflow and no hook — so nothing ever ran it. It reported one real drift: globals.css:10 --breakpoint-sm: 26.875rem 430px src/config/breakpoints.ts sm minWidth 428px globals.css is the source of truth and its comment says so deliberately: the iPhone 14 Pro Max is exactly 428 CSS px and it is a phone, so `sm:` utilities must NOT apply there. `breakpoints.ts`'s own docstring calls itself a mirror of the Tailwind config, so its 428 was the drift. It is now 430, and `xs` ends at 429 rather than 427 because 428 and 429 belong to the phone bucket. Five hand-written `@media (min-width: 26.75rem)` blocks in globals.css — 428px — now read `@media (width >= theme(--breakpoint-sm))`, so they cannot drift from the token again. Verified in the COMPILED stylesheet, not just the source: it contains 26.875rem, contains no 26.75rem, and no unresolved `theme(` survives, so the token form really does compile here. Wired into both places that would have caught it: a step in the CI lint job and a check in scripts/validate-ci.sh, which is what .husky/pre-push runs. Landing them on the same commit as the fix, so they land green rather than blocking the next person. Mutation-checked: putting 428 back makes `validate:breakpoints` exit 1; restored, it exits 0. HONEST SCOPE — THE DRIFT'S BLAST RADIUS IS CURRENTLY ZERO. §B2 says that at 428-429px the hand-written queries fire while no Tailwind `sm:` utility does. That is true, and it affects only dead code: the five rules style `.btn-mobile-compact`, `.page-title`, `.section-title`, `.subsection-title` and `.minor-heading`, and all five have ZERO references anywhere outside globals.css. `BREAKPOINTS` and `MEDIA_QUERIES` in breakpoints.ts likewise have no consumers outside their own file — only the validator reads them. So nothing a user can see changes here. The value is that the number is now single and the check now runs; §C4 is where those unused classes should actually be dealt with. Refs #373. * fix(#378): the mobile navigation's own items were 26px, and the gate exempted them by name Below lg, the hamburger IS the navigation. Measured at 390px with it open: 13 destinations, every one 26x144 against the 44px floor this repo gates on. DaisyUI renders `menu li > a` at 26px, and only some items in this file had ever been given a floor — the desktop Demos submenu, the desktop account dropdown and Sign Out, each with a comment noting the 26px default. The mobile route list had none. Every item now goes through one named `MENU_ITEM` class rather than repeating the floor, so a new entry cannot arrive at 26px by omission. before 13 items, all 26x144 (signed out); 15 signed in after 0 under 44px at 320 / 390 / 428 / 768px THE GATE EXEMPTED THESE DELIBERATELY, AND THE REASON DOES NOT SURVIVE READING. `mobile-touch-targets.spec.ts`'s primary selector is `nav button, nav label, a.btn, a.sh-btn, button.sh-btn`, and its coverage note says bare `<a>` items are out of scope as "inline text links". That is true of a link inside a paragraph. It is not true of the entire mobile navigation. Same shape as #457, where every touch-target test dismisses the cookie banner before measuring, so the banner's own 32px buttons are unmeasurable. New test `the mobile menu's own items meet 44px (#378)` opens the hamburger at four widths and measures its items on both axes, with a per-width coverage floor of 13 — a menu that failed to open would otherwise pass silently, the #411/#454 shape. Mutation-checked: dropping `min-h-11` from `MENU_ITEM` makes it report "15 mobile menu items under 44px at 320px" and fail. TWO MISTAKES WORTH RECORDING, BOTH CAUGHT BY THE NEW GATE RATHER THAN BY ME. `Profile` and `Connections` each appear TWICE in this file — once in the desktop account dropdown, once in the hamburger — and my first pass used a replace-once, so it fixed the DESKTOP copies, which already had a floor, and left the mobile ones at 26px. Fixed by pattern over every unclassed menu link instead of by ordinal. My standalone probe reported 0 under-floor items and was wrong: it ran signed OUT, so it never rendered Profile, Settings, Messages, Connections or Admin Dashboard at all. The E2E project uses an authenticated fixture and saw them immediately. A probe that measures a subset of the states the component has is measuring the convenient condition. Full spec: 8 passed. The two failures — `All interactive elements` and `Links in content` — fail identically on main under this local harness, which serves bare `/` from a dev server with a basePath, i.e. the 404 page. That page renders the nav from the layout, so nav geometry is measurable there; anything needing real page content is not. CI serves the static build at root. Refs #378. * fix(#469): the Display popover measured correctly and read badly Every gate on #465 passed — 44 controls, none under 44px, Escape closes and restores focus, contrast clean — and on a 390px phone the panel was a 720px slab with the theme list buried behind two scrollbars. Measured on live production with the panel open, then fixed: before after nested scroll regions 1 (inside a 0 scrolling panel) heading font sizes 18.5 / 25.7 / 18.5 / 18.5 / 18.5 18.5 px alert blocks 1, 105px tall 0 controls / under 44px 44 / 0 44 / 0 (unchanged) Three separate causes, all mine, all shipped an hour earlier. `ColorVisionPanel` was never restyled for its new context. It kept the `h3 text-lg font-bold` it had as a standalone popover, which put it a whole tier above the "TEXT SETTINGS" and "THEME" labels beside it and wrapped it onto two lines at 390px. It is now a section label like its siblings. The colour-vision hint was an `alert alert-info` — a tinted full-width block 105px tall for one sentence, inside a menu. It is a hint line now. `role="status"` and `aria-live` stay: the announcement when the mode changes is the whole point of that text, and only its presentation was wrong. The theme list carried `max-h-64 overflow-y-auto` INSIDE a panel that is already `max-h-[80vh] overflow-y-auto` — two scrollbars in one popover, the inner one holding 1496px of content in a 256px window. The panel is now the only scroll region. WHY NO GATE CAUGHT THIS, WHICH IS THE PART WORTH KEEPING. They measure what they were told to: control count, hit-target size, contrast ratio, focus behaviour. None of them has an opinion about proportion, heading hierarchy, or how many nested scrollbars a menu has. This is not a gap another assertion closes. It is what opening the thing at phone width and looking at it is for, and I had not done that — I had measured it and moved on. Verified after: three headings at one tier, hint as a line, theme list flowing into the single scroll region with the active theme marked. Both #378 gate tests still pass, 23 ColorblindToggle unit tests pass, type-check clean. Closes #469. --------- Co-authored-by: TurtleWolfe <TurtleWolfe@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits.
Demos ▾was already shipped; this is the other half of #378 —Display ▾— plus the gate work the ticket required in the same PR.The defect
Three appearance dropdowns — text settings, colour vision, and the 34-theme list — were each
hidden lg:block. Measured: the mobile hamburger holds 13 destinations and zero appearance controls. So below 1024px there was no way to change the theme or the font size from the nav at all. The comment on the old font-size block claimed it was "accessible via hamburger". It was not.They are now one
Display ▾, present and visible at every width:Escape closes it and returns focus to the trigger at all seven widths. No horizontal overflow anywhere.
Deliberately not a
role="menu"NavGroupMenuis right for a list of links, but a menu's children aremenuitems — a<select>, a toggle and two button groups are not.NavPopoveris a disclosure:aria-expanded,aria-haspopup="true"rather than"menu", and a plainrole="group"panel. The dismiss contract (Escape + focus restore, outside-press, tab-out) moves into a shareduseDismissablehook so both primitives implement it once.Named
Display, not "Display menu" —mobile-navigation.spec.tslocates[aria-label*="menu" i],.first()follows document order and nav precedes page content, so a name containing "menu" shadows the mobile hamburger. That was #378's own earlier regression.43 touch targets were under the floor, and no gate could see any of them
selectAll of them sit inside a closed popover, and
mobile-touch-targets.spec.tsmeasuresnav button, nav label, a.btn, …— so they were never measured.min-w-11as well asmin-h-11, because the floor is 44×44 and these areflex-1: with height alone, "L" measured 44×42 while "XL" measured 44×52.The gate now opens the popover
the Display popover is reachable at every width and its controls meet 44pxsweeps six widths, asserts the trigger is visible at each (a breakpoint-hidden appearance control being the defect), opens it, and measures every control on both axes. It carries a per-width coverage floor of 40 controls, because a panel that rendered empty would otherwise pass silently — the #411/#454 shape.Mutation-checked: reverting the theme buttons to
btn-smmakes it report34 Display controls under 44px at 320pxand fail.Three things I got wrong on the way
A duplicate accessible name — caught by the new test.
NavPopoverfirst putaria-label={label}on both the trigger and the panel, putting two nodes in the accessibility tree with one name. Playwright reported[aria-label="Display"]resolving to 2 elements. The panel is now named by its trigger viaaria-labelledby, and carries adata-testidso tests don't guess.A flaky assertion I exposed rather than caused.
the Demos menu is keyboard operableclicked the trigger then immediately assertedaria-expanded === 'true'. A click before React attaches its handler is silently swallowed, and the server already rendersaria-expanded="false"— so the preceding assertion passes instantly against an inert button. Identical code passed one run and failed the next. It now retries the click until it takes, only while the menu is closed so retrying can't toggle it shut. (body[data-theme]is not a usable hydration marker —ThemeScriptsets it pre-paint at 251ms.)A 10x hydration regression that didn't exist. I measured
domcontentloadedat 5788ms against main's 563ms and nearly reported it. That was the dev server compiling a freshly-edited bundle while main's was cached. With a warm-up load: 1373ms vs 1443ms — no regression.Verification note
Locally these specs run against the dev server, where
basePathmeans bare/serves the 404 page — which still renders the nav from the layout. Nav geometry claims hold there and the mutation check works, but the two pre-existing failures in this spec (All interactive elements,Links in content) need real page content and fail identically onmainunder the same harness. CI serves the static build at root, so those get the real pages.Refs #378 —
Demos ▾was already shipped; the two defects recorded in the ticket body (26px hamburger items, no appearance control below 1024px) are addressed for the second, and the first is a separate change to the hamburger's own link geometry.