Harden frontend accessibility outside Settings#329
Conversation
Add accessible dialog, composite widget, live-region, remote UI, and semantic control contracts outside Settings. Enforce selected jsx-a11y rules and add maintained Playwright/axe coverage.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Automated manual QA: passed after fixesStatus: Ready for review. Every finding from the first QA pass is fixed and covered by maintained regression assertions.
Resolved findings
Verification
Fixed-state evidence
Remaining human checkThe connected Remote journey was exercised against a strict route mock rather than a live Pane host. A live-host smoke is still useful but is not blocking this accessibility QA result. Screenshot evidence is temporary x0.at content with size-based 3-100 day retention. |
Keep Create Pane dialogs open when branch popups consume Escape, name advanced switches, and correct affected contrast tokens. Surface malformed Remote connection codes and cover the repaired desktop and Remote states in maintained axe tests.
parsakhaz
left a comment
parsakhaz
left a comment
There was a problem hiding this comment.
PR Review
Issue context: #323 — harden frontend accessibility beyond dropdown navigation: replace non-semantic clickables, put AboutDialog on the shared accessible dialog contract, add deliberate live-region announcements, complete custom ARIA widget keyboard behavior, and add lint + axe guardrails.
Quality Gates
- Typecheck: PASS
- Lint: PASS (0 errors; jsx-a11y subset enforced as errors)
Must-Fix (1)
frontend/src/components/ProjectSessionList.tsx:750— sidebar Archive/Pin buttons are unreachable by mouse. The SessionRow content wrapper isrelative z-10 pointer-events-none contents.display: contentsremoves the element's box, soposition/z-indexnever apply — the children are painted as plain in-flow content. Meanwhile the row-select overlay<button className="absolute inset-0 z-0">is positioned, and positioned elements withz-index: 0paint above in-flow siblings. Result: pointer clicks on Archive/Pin land on the overlay and select/open the pane instead. Verified in Chromium with a reduced repro:document.elementFromPointover the Archive button returns the overlay, and a realmouse.clickfires the overlay handler. (Keyboard still works because Tab reaches the real buttons — which is why the axe/keyboard journeys didn't catch it; the accessibility spec only asserts the buttonstoBeAttached.) Fix: give the actions container its own stacking, e.g.relative z-10 pointer-events-autoon the actions div (the pattern ExecutionList and DiffViewer already use), or dropcontentsand restore a real positioned wrapper. Consider adding a Playwright assertion that clicking Archive fires the archive action, not row selection.
Should-Fix (3)
frontend/src/components/panels/PanelTabStrip.tsx:333—aria-ownsreferences a nonexistent id during rename. The tablist'saria-ownsalways lists every panel's tab id, but while a tab is being renamed (isEditing) its tab button isn't rendered, so the reference dangles. That's anaria-valid-attr-value(WCAG A) violation — the very axe gate this PR adds would flag it if a scan ran mid-rename. Exclude the editing panel from thearia-ownslist.frontend/src/components/panels/PanelTabStrip.tsx:210— close-tab focus fallback can jump strips.document.querySelector('[role="tab"][tabindex="0"]')is a global query; with split panel groups there are multiple tablists, and closing the last tab in one group can move focus to a tab in a different strip (or the top bar). Scope the fallback to the current strip, e.g. query ids prefixed withpanel-tab-${safeDomId(idNamespace)}-.frontend/src/components/PaneChatView.tsx:141— focus is dropped mid-interaction while switching agents. Arrowing/clicking a radio setsswitchingAgent, which immediatelydisableds both radios including the focused one — keyboard focus falls to<body>and is not restored when the switch completes. Prefer keeping the inputs enabled and guarding inonChange(the existingswitchingAgentcheck already does this), or refocus the active radio after the switch resolves.
Suggestions (3)
frontend/src/components/panels/PanelTabStrip.tsx:330— the emptyrole="tablist"div withdisplay: contents+aria-ownsis a clever way to keep drag/drop DOM structure, but it's a fragile pattern (older non-Chromium engines droppeddisplay: contentselements from the accessibility tree; fine in Electron's Chromium). Worth a short comment explaining why the tablist is detached from its tabs so a future refactor doesn't "simplify" it into a broken state.frontend/src/components/DetailPanel.tsx:575—aria-label="Commit history"on a plaindiv(no role) is not reliably exposed; since this is a focusable scroll region, addrole="region"so the label lands.frontend/src/remote/components/RemoteConnectionScreen.tsx—showConnectionTroubleshootingdecides whether to show the Tailscale hint by regex-matching error copy (/^(Connection code|Invalid remote Pane connection code)/). This couples UI logic to message strings that live elsewhere; a structured error kind (or error code) would be sturdier.
Completeness
Against #323's acceptance criteria:
- ✅ Non-semantic clickables replaced with native buttons across Sidebar, ExecutionList, DiffViewer headers, ProjectSessionList, ProjectDashboard rows, FileEditor search results, HomePage, GitStatusIndicator, GitHistoryGraph, SessionView pills.
- ✅ AboutDialog consumes the shared Modal (now Radix Dialog) with title, focus containment, and opener restore — including the update-handoff case that intentionally skips restore.
- ✅ Live regions: shared
LiveRegioncomponent, deliberate announcements for archive progress, dashboard loads, updates, copy actions, session status, remote connection state; terminal output correctly not announced. Nice touch keeping regions mounted in null branches (ArchiveProgress) so announcements actually fire. - ✅ Composite widgets: roving-tabindex tabs (desktop + remote), comboboxes with
aria-activedescendant(CommandPalette, branch pickers, FilePathAutocomplete), menu keyboard nav, native radio group for Pane Chat, and the joystick as a realinput[type=range]witharia-valuetext. - ✅ Lint guardrails: jsx-a11y rules as errors, CI-enforced, no ignore-wall.
⚠️ Axe journeys cover Home, session creation, active-session workflow, Pane Chat, themes, and both Remote paths — but not Settings, which #323's criteria list. The PR explicitly defers Settings implementation work (#328-adjacent scope note), which is reasonable, but the Settings axe journey should land with that follow-up so the criterion isn't silently dropped.- ✅ Manual pass documented (12 themes, 200% reflow, mobile viewport);
--owl-300and interactive-primary contrast fixes came out of it.
Summary
This is a thorough, well-architected accessibility pass — the Radix Dialog migration with the portal-container context (so nested tooltips/selects/dropdowns stay inside the dialog's focus boundary) is the standout piece, and the test infrastructure (axe helper, seedable Electron API mock) is a durable investment. One real regression must be fixed before merge: the sidebar Archive/Pin buttons lose mouse clicks to the row overlay (finding 1). The remaining items are small. After fixing #1 (and ideally #2–4), this is ready to merge.
| className="absolute inset-0 z-0 focus:outline-none focus:ring-2 focus:ring-inset focus:ring-interactive" | ||
| /> | ||
| </Tooltip> | ||
| <div className="relative z-10 pointer-events-none contents"> |
There was a problem hiding this comment.
Must-Fix: display: contents removes this element's box, so the relative z-10 here never applies — children become plain in-flow content. The sibling overlay button (absolute inset-0 z-0) is positioned, and positioned elements with z-index: 0 paint above in-flow siblings, so pointer clicks on the Archive/Pin buttons hit the overlay and select the pane instead of archiving/pinning. Verified in Chromium: document.elementFromPoint over the Archive button returns the overlay, and a real click fires the overlay's handler.
Fix: move the stacking onto the actions container itself, e.g. change the actions div to relative z-10 pointer-events-auto ... (the pattern ExecutionList/DiffViewer use), or drop contents in favor of a real positioned wrapper.
| <div | ||
| role="tablist" | ||
| aria-label="Panel tabs" | ||
| aria-owns={panels.map((panel) => getPanelTabId(idNamespace, panel.id)).join(' ')} |
There was a problem hiding this comment.
Should-Fix: while a tab is being renamed (isEditing), its tabButton isn't rendered, so this aria-owns references a nonexistent id — an aria-valid-attr-value (WCAG A) violation that the axe gate added in this PR would flag if scanned mid-rename. Exclude the editing panel:
aria-owns={panels
.filter((panel) => panel.id !== editingPanelId)
.map((panel) => getPanelTabId(idNamespace, panel.id))
.join(' ')}| const preferredTarget = focusTarget | ||
| ? document.getElementById(getPanelTabId(idNamespace, focusTarget.id)) | ||
| : null; | ||
| const fallbackTarget = document.querySelector<HTMLElement>('[role="tab"][tabindex="0"]'); |
There was a problem hiding this comment.
Should-Fix: this fallback is a global query. With split panel groups there are multiple strips, so closing the last tab of one group can move focus to a tab in a different strip. Scope it to this strip, e.g. document.querySelector(\[id^="panel-tab-${safeDomId(idNamespace)}-"][tabindex="0"]`)`.
| name="pane-chat-agent" | ||
| value={option.id} | ||
| checked={selected} | ||
| disabled={switchingAgent !== null} |
There was a problem hiding this comment.
Should-Fix: disabling the focused radio the moment the user activates it drops keyboard focus to <body>, and it isn't restored when the switch completes. Since handleAgentChange already early-returns while switchingAgent !== null, consider leaving the inputs enabled (or using aria-disabled), or refocusing the selected radio in the finally block.
| <div className="flex-1 min-h-0 overflow-y-auto px-2 pb-2"> | ||
| <div | ||
| tabIndex={0} | ||
| aria-label="Commit history" |
There was a problem hiding this comment.
Suggestion: aria-label on a role-less div isn't reliably exposed to assistive tech. Since this is a focusable scroll region, add role="region" so the label takes effect.
Restore pointer access to sidebar actions, keep tab and radio focus semantics valid, and use structured Remote connection errors. Add Playwright coverage for the reviewed interaction states.
|
Addressed the linked review in
Validation: typecheck; lint (0 errors); frontend unit tests (83/83); frontend production build; minimal CI Playwright suite (26/26). Settings implementation remains untouched as requested. |
1 participant
Summary
jsx-a11yrules and add maintained Playwright/axe journeys to minimal CIScope
Direct Settings dialog implementation work is intentionally excluded because it is being handled separately. This PR does not modify
frontend/src/components/Settings.tsx; it only preserves Settings compatibility through shared Modal and Select contracts.Refs #323
Testing
pnpm typecheckpnpm lint(0 errors; 167 existing warnings)pnpm --filter frontend test(83 passed)pnpm run build:frontendpnpm test:ci:minimal(26 passed, including 7 axe journeys)pnpm test -- tests/settings.spec.ts(15 passed)pnpm test -- tests/dropdown-keyboard-nav.spec.ts(3 passed)The connected Remote Pane path was not exercised against a live host; automated QA covers it through a route-mocked host and covers the disconnected journey through the Electron API mock. No visual redesign is included.
Automated manual QA
Status: Passed and ready for review.
2efe26af36b35a175c7eef0fabf14ca556e6f74awith markeragent-e2e-pr-329-20260713-fixed.