fix(a11y): repo dialog and SSO form WCAG 2.2 AA gaps#459
Conversation
Address four accessibility gaps in the repository create/edit dialogs and SSO settings forms: - #410: the repo edit dialog now mirrors the upstream-auth save outcome into an in-dialog aria-live region (role="status" on success, role="alert" with aria-live="assertive" on error) so screen-reader users hear the result. The parent mutation previously surfaced only a visual toast. - #411: the create-dialog duplicate-key error now has role="alert", and the key input sets aria-invalid and aria-describedby pointing at the message. - #412: toggling the upstream-auth view to edit (and back) now moves focus to the first control of the newly-revealed view instead of dropping focus on the body. - #413: required inputs across the repo dialogs and SSO OIDC/LDAP/SAML forms expose aria-required; the edit-dialog auth-type select gains a programmatic label; the repo row actions menu trigger gains an accessible name; the edit-key warning is associated via aria-describedby. Adds a Playwright spec under e2e/suites/interactions/repositories asserting the error/alert association, focus movement on toggle, and live-region save announcement.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Accessibility review (WCAG 2.2 AA). This is the dedicated a11y PR; I verified it against the four linked issues. It is solid work and the targeted fixes are correct. A few gaps remain.
Verified closing #410/#411/#412/#413
- #411 (create dialog duplicate-key error): correct.
#create-keysetsaria-invalidandaria-describedby="create-key-error", and the message carriesrole="alert"with the matching id. SR announces it and ties it to the field (1.3.1, 3.3.1, 4.1.3). Good. - #412 (focus on auth view<->edit toggle): the rAF +
getElementById(...).focus()effect moves focus to the revealed control and theeditAuthModeInitializedguard avoids stealing the dialog's initial focus. Reasonable given SelectTrigger does not forward a ref (2.4.3). - #410 (live region for upstream-auth save): correct.
role=status/aria-live=politeon success,role=alert/aria-live=assertiveon error, andonMutateresets to idle so a repeat success re-announces (4.1.3). Good. - #413 (aria-required, auth-type label, actions-menu name): all present. Repo row trigger now has
aria-label, the auth-type select has a real<label htmlFor>(4.1.2). Good.
Gaps to consider (non-blocking)
-
aria-requiredwas added butaria-invalid/error-association was NOT extended to the SSO OIDC/LDAP/SAML forms. Those forms still surface validation only via toast (transient, not programmatically tied to the field). #413 scoped this as a "broader gap" pass, so flagging that required-field validation errors in the SSO forms remain un-associated (3.3.1). Worth a follow-up issue. -
#412: focus is moved on toggle, but when the auth-type Select is closed via Escape or the view collapses back to "view" mode, confirm focus lands on
#edit-upstream-auth-toggle(the effect coverseditAuthMode === "view"so this looks handled) and is not dropped ifeditRepo.upstream_auth_configuredflips the rendered branch. Manual SR pass recommended. -
The success live region uses
aria-live=politebut the node is always present and only its text changes, which is the correct pattern. However the empty string is rendered as""inside the div on idle; that is fine. No change needed, just confirming it is not an empty-but-busy region.
No blockers. The three core fixes are correct and verifiable. Not approving (review-only).
brandonrc
left a comment
brandonrc
left a comment
There was a problem hiding this comment.
Test/E2E quality review (v1.2.0 batch). Verdict: genuine a11y coverage, no theater. This is the model the other admin-gated specs should follow.
#411 duplicate-key alert. Strong. It doesn't just check the error text exists; it asserts the WCAG wiring that was the actual bug: role="alert", aria-invalid="true", and crucially that aria-describedby on the input equals the alert's id (expect(describedBy).toBe(errorId)). A regression that rendered the error text without wiring aria-describedby (the pre-fix state) would fail this. No skip path.
#412 focus management. This is the assertion most specs in this batch lack: await expect(authTypeSelect).toBeFocused() after the toggle. The bug was focus dumped on <body>; this directly distinguishes fixed-vs-broken. The requestAnimationFrame-based focus move in the component is matched by Playwright's auto-retrying toBeFocused, so no arbitrary wait needed. Good.
#410 live region. Honest. It asserts the region has aria-live polite/assertive, then drives the real save and asserts not.toBeEmpty() AND toHaveText(/updated|fail|error/i). The text-regex accepts either success or error, which is the right tolerance for a backend that might reject the dummy token, while still proving the live region announces something (the bug was silence). It does not vacuously pass on an empty region.
No test.skip escape hatches anywhere in this file, no brittle ordering. The component diff backs every selector (#create-key-error, id="edit-upstream-auth-type", label[for=...], data-testid="upstream-auth-status"). Not requesting changes.
Addresses review feedback on artifact-keeper#448: - Programmatically associate the TTL validation error with the input: the error <p> now has a stable id ("settings-cache-ttl-error") and role="alert", and the input sets aria-describedby to it while invalid. Screen-reader users now hear the explanation instead of just "invalid", and the message is announced when it appears. Mirrors the reference pattern in artifact-keeper#459. - Disable the Discard button while a save / cache-TTL mutation is in flight, matching the Save button's pending guard, so Discard can't race an in-flight save. - Tests: assert the aria-describedby/role=alert association (and its removal when valid again) and the Discard pending guard. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Shipped in v1.2.0. The changes from this PR were folded into the release integration branch #466, which was squash-merged to main and tagged as v1.2.0. The code is already on main, so closing this as superseded by #466 rather than merging it (re-applying would just duplicate what already shipped). No work lost. See #466 for the consolidated record. |
1 participant
Summary
Fixes four WCAG 2.2 AA accessibility gaps in the repository create/edit dialogs and the SSO settings forms (v1.2.0).
role="status"(aria-live="polite"); failure rendersrole="alert"(aria-live="assertive"). Previously the only feedback was a visual toast, which assistive tech outside the dialog did not reliably announce while focus stayed inside the open dialog. Wired from the parent mutation inrepositories-content.tsx.role="alert", and the key input setsaria-invalidandaria-describedbypointing at the message id, so screen readers announce the error and tie it to the field.<body>. Implemented with an effect that targets the control by id and defers a frame so the new control exists in the DOM.aria-required; the edit-dialog auth-type select gains a programmatic<label>; the repository row actions menu trigger gains an accessible name; the edit-key warning is associated viaaria-describedby.Closes #410
Closes #411
Closes #412
Closes #413
Test Checklist
New spec
e2e/suites/interactions/repositories/repo-dialog-a11y.spec.tsasserts the error/alert +aria-describedbyassociation (#411), focus movement on the auth toggle and the auth-type select label (#412/#413), and the live-region save announcement (#410). Existing unit suites (repo-dialogs.test.tsx77 tests, repositoriespage.test.tsx, SSOpage.test.tsx) pass unchanged.eslintclean on changed files (one pre-existing unrelated warning);tsc --noEmitreports only pre-existing errors in untouched test files.UI Changes
No visual layout changes; additions are ARIA attributes, a focus-management effect, and a status text line in the upstream-auth section of the edit dialog.