fix(ui): give the shared components the behaviour their roles promise - #313
Open
antosubash wants to merge 1 commit into
Open
fix(ui): give the shared components the behaviour their roles promise#313antosubash wants to merge 1 commit into
antosubash wants to merge 1 commit into
Conversation
`SegmentedControl` announced `role="radiogroup"` while behaving like a row of buttons: every option was its own tab stop and the arrow keys did nothing, so a screen-reader user was told to expect one control and handed six. It now carries a roving `tabIndex` and Arrow/Home/End handling that moves focus and selects as it travels, skipping disabled options and wrapping at the ends. Six screens use it (users, settings store, tasks, flags, add-people, branding preview). `initTheme` called `media.addEventListener` unguarded. On Safari < 14 that is a TypeError thrown inside the effect that calls it, which takes the whole theme init down and leaves the app on whatever the class list happened to say. It now falls back to the deprecated `addListener`, and to a no-op unsubscribe when neither exists. The auth screens used the default 36px `Input` and 40px `size="lg"` Button against a deck that specifies ~46px fields and ~48px buttons on a 10px radius (docs/superpowers/specs/hifi-gap/auth-screens-02-07.md). `AuthCardShell` now sizes its card's controls once, so a seventh auth screen cannot be born a size behind — and the fields clear the 44px phone tap target, which they did not before. The `max-lg:min-h-11` overrides those `size="lg"` buttons carried are now redundant and gone. `split-light` put both columns on `bg-background`, leaving the card's 1px border as the only thing marking the split on register and accept-invite. The aside moves to `bg-secondary/40` — the same page surface the centred variant sits on, and still a token, so the dark theme applies. `useRelativeTime`'s `until` re-implemented the null/invalid parsing `ageOf` already encodes. It cannot simply call `ageOf`, which clamps at zero and would read every future deadline as expired; the shared `parseTimestamp` is extracted instead and `timeUntil` is its unclamped sibling. The file table's select-all box grew its phone hit area with a symmetric `before:-inset-3.5`, but sits at the top edge of a table whose own container is `overflow-x-auto` (which computes the block axis to `auto` too) inside an `overflow-hidden` card — the upward 14px was clipped, and clipped overflow is not hit-testable. A 56px header row on phones centres the box with 20px of cell either side, so the whole 44px target lands inside both clipping boxes and clear of row one's own bleed. Settings Browse sized its card with `100vh - --app-chrome-h - 15rem`, where the `15rem` was a hand-measured stand-in for the heading, tabs and search bar above it and drifted whenever any of them changed. `PageShell` grows a `fill` prop that hands the leftover viewport height to the content column, so the card asks for `lg:flex-1` and the arithmetic stays in the layout engine. Tests: `@testing-library/user-event` is added so the keyboard work is driven through real Tab/Arrow/Home/End rather than asserted from markup. The layout items are class-level assertions — jsdom has no layout engine — so the emitted utilities were checked against a real `npm run build` instead. Closes #297
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
1 participant
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.
Closes #297
Seven follow-ups from reviewing the shared components
worktree-hifi-pagesintroduced. Each claim was checked against the code before acting; two of the
suggested fixes turned out to be wrong on inspection and are done differently,
noted below.
What changed
SegmentedControlnow behaves like theradiogroupit announces. RovingtabIndex(the group is one tab stop, parked on the checked option) plusArrow/Home/End that move focus and select, skipping disabled options and
wrapping at the ends. Six screens use it.
initThemefalls back toaddListener.media.addEventListeneris aTypeError on Safari < 14, thrown inside the effect that calls
initTheme—it took the whole theme init with it. Falls back to the deprecated pair, then
to a no-op unsubscribe.
AuthCardShellsizes its card's[data-slot=input]andsize="lg"buttons once rather than each screenspelling it out, so a new auth screen can't be born a size behind. The fields
now clear the 44px phone target, which they did not at
h-9; themax-lg:min-h-11overrides on those buttons are redundant and removed.split-lightreads as two surfaces. The aside moves offbg-backgroundto
bg-secondary/40— the same page surface the centred variant sits on, andstill a token, so dark theme applies.
useRelativeTimestops re-parsing. Deviation: the issue suggestedreusing
ageOf, butageOfclamps at zero, so every future deadline wouldread as expired. The shared
parseTimestampis extracted instead, withtimeUntilas its unclamped sibling. No behaviour change.clipping box is closer than the issue says — the table's own container is
overflow-x-auto, which computes the block axis toauto, so the upward 14pxwas already lost before the card's
overflow-hidden. A directional bleedwould also have collided with row one's bleed (later in DOM, so it wins hit
testing). Fixed by making the header row 56px on phones: the 16px box sits
with 20px of cell either side, so the whole 44px target is inside both
clipping boxes and clear of row one.
PageShellgrows afillpropthat hands the leftover viewport height to the content column; the card asks
for
lg:flex-1instead of100vh - --app-chrome-h - 15rem.Testing approach
Keyboard and focus work is driven through real interaction —
@testing-library/ user-eventis added as a devDependency foruser.tab()/user.keyboard(),and every new behavioural test was confirmed to fail against the unpatched
component before being kept (4 SegmentedControl keyboard tests, 2
initThemefallback tests).
The four layout/deck-fidelity items cannot be measured in jsdom — there is no
layout engine, so those tests assert the class or attribute that is the
mechanism. To cover the gap the bundle was built and the emitted CSS checked:
[data-slot=input]{height:46px},border-radius:10px,[data-size=lg]{height:calc(var(--spacing) * 12)}(48px),.max-lg\:h-14, andcalc(100vh - var(--app-chrome-h))are all present, and the- 15remvariantno longer comes from settings Browse.
Verification
npx vitest runnpx vitest run packages/uinpx biome check .npx tsc -p packages/ui --noEmitnpx tsc -p modules/file_storage --noEmitnpx tsc -p modules/settings --noEmitnpx tsc -p modules/users --noEmitnpx tsc --noEmit -p host/client_app/tsconfig.jsonnode scripts/check_untranslated_strings.mjsuv run python scripts/check_file_size.pyuv run pytest -qnpm run buildNo new user-visible strings were added, so no catalog change and no
make gen-i18nwas needed.Deliberately out of scope
modules/feature_flags/.../FlagTable.tsxandmodules/dashboard/.../Home.tsxcarry the same
100vh - --app-chrome-h - Nremmagic numbers. The issue scopedthat bullet to settings Browse, and both pages have different content below the
card, so converting them to
fillis a separate change worth its own visualcheck. The mechanism is now there for it.
Not visually verified in a browser: the four layout items are asserted through
the emitted CSS, not a screenshot.