fix: six measured interaction and a11y defects (dialog Tab, switch names, tooltip, copy failure, checkbox, approval card) - #268
Open
Broccolito wants to merge 8 commits into
Open
Conversation
Measured in Chromium on **New schedule**: twelve Tab presses cycled Name → Browse → "Repeat every" → the dialog container. Cancel, "Create schedule", the × and the three time selects were never focused, and the dialog carried no `aria-modal`. The trap was not broken. `react-select` unmounts its four `aria-live` spans synchronously while blurring, and Radix's `FocusScope` watches the dialog subtree for removals so it can answer "the focused node was removed and the browser dropped focus on <body>" by focusing the container. Between a `focusout` and its `focusin` that is exactly what the DOM looks like, so the observer's microtask parks focus on the dialog and the in-flight Tab is lost — once per mutation record, four times over. Every control after the first `Select` was unreachable, in every dialog that holds one. `dialogTabRepair` remembers where the browser said the Tab was going and hands focus back if the container takes it instead, deferring the restore past the park loop so the last word is the user's. A blur with no `relatedTarget` — a genuine removal — is left to Radix, which is what its park is for. Evidence: the new spec fails 3/5 on the unfixed primitive. In Playwright against the real dialog the cycle goes from four stops to all ten, forwards and with Shift+Tab.
Measured with the accessibility tree: Menu bar icon, Dock icon, Prevent sleep
and Cost tracking were `role=switch` with `aria-label` null, `aria-labelledby`
null and no text content. A screen reader announced "switch, on" — four times
in one panel, each about something different.
The subject was already on screen, in the `<p>` beside each switch; it was
simply never connected to the control. The Privacy tiers switch two panels
over does connect it, and this copies that: `aria-label` quoting the visible
label verbatim, so someone driving the app by voice can say what they read.
Two labels move to sentence case in the same breath ("Prevent Sleep", "Cost
Tracking"), which is the app's copy rule and what keeps name and label the
same string. Nothing else in the repo referenced either.
Evidence: the new spec fails all 5 before (no switch can be found by name).
Hover a row control, then change route without moving the pointer: the target
unmounts, so no `pointerout` is ever delivered and the tooltip is stranded on
screen describing an element that no longer exists.
The guard for this was `useEffect(() => { if (tooltip &&
!tooltip.target.isConnected) setTooltip(null) }, [tooltip])` — a check that
runs when the tooltip STATE changes, which is never what happens here. Adding
`tooltip.target` to the deps would not help either: a node reference does not
change when the node is removed. A removal is a DOM event, so it takes a DOM
observer, and this watches for one while — and only while — a tooltip is open.
Evidence: the new case fails before (the tooltip is still in the document
after the target unmounts), passes after; the five existing cases are
unchanged.
Both clipboard writes failing ended at two `console.error`s and nothing else: `markCopied()` was never reached, so the button went on saying "Copy" and the only way to learn that nothing had been copied was to paste somewhere and find out. The button now reports the outcome it actually had — "Copy failed", in the warning ink with a warning glyph — and a single error toast says what to do instead, because the pointer is rarely where the eye is. Both signals are the same 2s transient the success path uses, so the control settles back to "Copy" on its own. One transient value with two states rather than two booleans, so the button can never claim both. The success and rich-copy-then-fallback paths are untouched, and both are now pinned. Evidence: the refusal case fails before (the button keeps saying "Copy", no toast); the two success cases pass before and after.
The primitive hid its input with `peer sr-only` — a 1px clipped box in the corner — so the 22px square everyone can see was a picture. It toggled only when a call site remembered to wrap it in a `<label>`, and two shipped screens did not: `ResetPanel`'s per-category boxes toggled on nothing at all, and `ExportAppDialog`'s toggled only on their text, because its label sits beside the box rather than around it. Fixed in the PRIMITIVE, not at the two call sites, because "correct only if every caller remembers a wrapper" is the actual defect — a third call site would have inherited it. The input is now an `appearance-none opacity-0` layer over the whole 24px target, on top of paint that is all `pointer-events-none`, so a click anywhere on the square lands on the real input. `appearance-none` + `opacity-0` keeps the OS from drawing its own un-themeable box, which is what `sr-only` was there for. A wrapping `<label>` still toggles exactly once: the browser does not forward a label activation whose target is already the labelled control. The focus ring moves to the square as well. `main.css` answers `input[type='checkbox']:focus-visible` with `outline: none` plus a background colour, which on a transparent input is no indication at all — so keyboard focus on a checkbox was invisible either way. Evidence, in Chromium: clicking the centre of the bare (ResetPanel-shaped) square gave `checked=false` before and `checked=true` after, with the label-wrapped case toggling once in both. In jsdom, 3 of the 5 primitive specs fail before. Two existing specs asserted the old `sr-only` spelling and now assert the new one.
⚠ Permission surface. The Rust half could not be compile-checked here (load average ~386 with cold Rust builds in flight) — please run `cargo check -p biorouter` and read this one closely. The card reads `prompt` as a SECURITY FINDING: warning banner, and "Always Allow" withheld, because a permanent grant is not something to decide from a card that exists only because an inspector objected. That is sound while `prompt` carries nothing but `approval_prompt_for_request`. The coding-agent bridge filled it with its own framing — "<child> asked to run this through Biorouter" — on *every* bridged call, so every one of them arrived under a security banner for a verdict no inspector had reached, and the user could never grant a lasting permission for a tool their coding agent uses constantly. The renderer was reading its field correctly; the producer was misusing it. `bridge.rs` no longer writes that field, and the attribution is logged instead of dressed up as a finding — showing it on the card properly needs a `requestedBy` field of its own through `ActionRequiredData` and the OpenAPI client, which is a schema change and a separate piece of work. Two smaller things fall out of it: * The renderer now derives ONE `securityFinding` boolean and both halves read it, so the banner and the withheld button cannot disagree — and a whitespace-only prompt, which used to paint an empty warning band *and* take the user's "Always Allow" away, is no finding at all. * An "Always Allow" answered on a bridged card now lands in the permission store. `handle_approved_and_denied_tools` does this on the agent's own path; the bridge only logged the permission, so a permanent grant behaved as allow-once and the next identical call asked again. Offering the button without this would have made the card lie in a new way. An *existing* entry was always honoured — the permission inspector reads the store before a card is ever raised — so what was missing was only the write. `AlwaysDeny` is recorded symmetrically; a one-off is not recorded at all. Evidence: 4 tests fail before — the three in `bridgeApprovalPrompt.test.ts` (a TS guard reading `bridge.rs`, because this rule spans two languages and was checked on neither side of the boundary) and "treats a blank prompt as no finding at all". The genuine-finding case (banner, no Always Allow) passes before and after, which is the point: it must not regress.
`format:check` is not wired into any gate, so it gets run by hand; this is what it asked for. No behaviour change.
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.
Six measured interaction/a11y defects, one commit each so any can be reverted alone. Every fix carries fail-before evidence; where jsdom cannot see the defect I measured it in real Chromium with Playwright and say so.
1.
4b0b39f6— "New schedule" dialog's primary action was keyboard-unreachableMeasured (Playwright, the real component): twelve Tab presses cycled
Name → Workflow file → Browse → Repeat every → [dialog container], forever. Cancel, Create schedule, the × and the three time selects were never focused.aria-modalwasnull.Root cause — an interaction between two libraries, neither wrong alone:
react-selectrenders its fouraria-livespans only while focused, so blur unmounts them synchronously inside thefocusoutdispatch (React flushes discrete events unbatched).FocusScopewatches the dialog subtree with aMutationObserverfor "the focused node was removed, so focus fell to<body>", and answers it by focusing the container.Between
focusoutandfocusin,document.activeElementis<body>and the JS stack empties — so the observer's microtask runs inside that window and parks focus on the dialog, cancelling the Tab in flight, once per mutation record (four). Traced by patchingHTMLElement.prototype.focus: the stealing call isfocus()fromMutationObserver.handleMutationsin@radix-ui/react-focus-scope. StubbingMutationObserverrestored the full tab order, which is what identified it.Not schedule-specific — every dialog holding a
Select(model picker, provider modals, lead/worker settings) had it, so the fix is onDialogContent: newdialogTabRepair.tsremembersfocusout.relatedTargetand hands focus back if the container takes it instead, deferred to a microtask so it lands after the whole park loop. A blur with norelatedTarget— a genuine removal — is left to Radix, which is what its park is for. Intent expires at the end of the task; restores are capped.aria-modal="true"added.Fail-before:
dialogTabRepair.test.tsxfails 3/5 on the unfixed primitive, 5/5 after. Real browser: 4 stops → all 10, forwards and with Shift+Tab.⚠ jsdom cannot reproduce this:
userEvent.tab()computes the next focusable and focuses it directly, so the<body>window never opens and the unfixed code passes. The spec drives the measured event pattern by hand instead.2.
e544ccd0— four Appearance switches had no accessible nameMenu bar icon, Dock icon, Prevent sleep, Cost tracking were
role=switchwitharia-labelnull,aria-labelledbynull, no text content → "switch, on", four times in one panel.aria-labeladded quoting the visible label verbatim (the Privacy tiers switch's pattern). Two labels moved to sentence case so label and name are one string; nothing else in the repo referenced either.Fail-before: new
AppSettingsSection.test.tsx— 5/5 fail ("Unable to find role=switch and name …"). It also asserts no switch in the panel is left unnamed.3.
a0f46a98— a tooltip stranded by a route changeuseEffect(… , [tooltip])ran only when the tooltip state changed — never for the case it was written for. Addingtooltip.targetto the deps does not help: a node reference does not change when the node is removed. Now aMutationObserver, installed only while a tooltip is open, clears it when the target stops beingisConnected.Fail-before: new case in
AppTooltipLayer.test.tsxfails (tooltip still in the document after the target unmounts); 6/6 after.4.
da27d72e— a failed message Copy told the user nothingBoth writes failing ended at two
console.errors;markCopied()was never reached, so the button kept saying "Copy". Now one transient value with two states: the button says Copy failed in warning ink with a warning glyph, and one error toast says what to do instead. Success and rich-copy→fallback paths untouched and now pinned.Fail-before: the refusal case fails (button keeps saying "Copy", no toast); the two success cases pass before and after.
5.
c05d30bf— theCheckboxsquare was not clickable → fixed in the primitiveDecision: the primitive, not the two call sites. "Correct only if every caller remembers a
<label>wrapper" is the defect — a third call site inherits it, and #247 already had to fix one instance that way. Swept all three users:settings/app/ResetPanel.tsxaria-label, no label element → the square toggled nothing (keyboard only)applications/ExportAppDialog.tsx<label htmlFor>→ the text toggled, the square did notsessions/SessionListView.tsx<label>→ worked (the #247 shape)The input is now
absolute inset-0 h-full w-full appearance-none opacity-0(waspeer sr-only) — an invisible layer over the whole 24px target, above paint that is allpointer-events-none.appearance-none+opacity-0is what keeps the OS from drawing its own box, which is whatsr-onlywas for. A wrapping<label>still toggles exactly once: the browser does not forward a label activation whose target is already the labelled control. The focus ring moves onto the visible square —main.cssanswersinput[type=checkbox]:focus-visiblewithoutline:none+ a background colour, which on a transparent input was no indication at all.Fail-before: in Chromium, clicking the centre of the bare square gave
checked=falsebefore andchecked=trueafter, with the label-wrapped case toggling once in both (no double toggle). jsdom: 3/5 primitive specs fail before. Two existing specs asserted the oldsr-onlyspelling and now assert the new one.6.
7dcff3b7— the bridge's approval card hid "Always Allow" ⚠ permission surface, wants a human lookThe exact condition:
{prompt && <banner>}and{!prompt && <Always Allow>}— two independent truthiness reads of one field.The bridge was setting the wrong field.
bridge.rs::await_approvalsetprompt: Some("<child> asked to run this through Biorouter")on every parked card.ToolApprovalRequest::promptis documented as "why approval is being asked for, when the inspector said", and the agent's own path fills it fromapproval_prompt_for_request. So every bridged call arrived under a security banner for a verdict no inspector had reached, and a tool the user's coding agent uses constantly could never be granted a lasting permission. The renderer was reading its field correctly. Fixed:prompt: None, attribution logged instead.A second Rust defect that had to be fixed with it.
await_approvalonly loggedApproved { permission }; the agent's path writesPermissionLevel::AlwaysAllowto the store (tool_execution.rs:481). So "Always Allow" on a bridged card behaved as allow-once and the next identical call asked again — un-hiding the button without this would make the card lie in a new way. Newrecord_lasting_decision(AlwaysAllow → AlwaysAllow, AlwaysDeny → NeverAllow, one-offs not recorded). Note: an existing entry was always honoured (the permission inspector reads the store before a card is raised); what was missing was the write.Renderer: one derived
securityFinding(non-blank prompt) read by both halves, so they cannot disagree — and a whitespace-only prompt, which used to paint an empty warning band and withhold Always Allow, is no finding.Fail-before: 4 tests fail — the three in the new
bridgeApprovalPrompt.test.ts(a TS guard that readsbridge.rs; the rule spans two languages and was pinned on neither side of the boundary —autovis_cdn_desktop_contract.rsis the precedent in the other direction) plus "treats a blank prompt as no finding at all". The genuine-finding case (banner, no Always Allow) passes before and after: that is the point.The card no longer shows who asked. Restoring that properly needs a
requestedByfield throughActionRequiredData+just generate-openapi+npm run generate-api— a schema change, deliberately not bundled here.Verification
npm run lint:checknpm run test:runsrc/utils/artifactCdnAssets.browser.test.tsafterAll→browser.close()30 s hook timeout, which fails on pristinemaintoo.npm run format:checkdialogTabRepair,dialog,AppSettingsSection,AppTooltipLayer,MessageCopyLink,Checkbox,SessionListView,ResetPanel,applications,ToolCallConfirmation,bridgeApprovalPrompt— all greenNo
npm install/npm ciwas run.Only settleable in the running app
peer-focus-visible:ring-*utilities actually generate (jsdom runs no Tailwind, and this repo has measured cases of a new utility silently failing to generate).🤖 Generated with Claude Code