-
Notifications
You must be signed in to change notification settings - Fork 11
fix(web): label six forms for screen readers #6068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import React from 'react'; | ||
| import { renderToStaticMarkup } from 'react-dom/server'; | ||
| import { describe, expect, it } from '@jest/globals'; | ||
| import { PasswordProtection } from './PasswordFormFields'; | ||
|
|
||
| function render(enabled: boolean) { | ||
| return renderToStaticMarkup( | ||
| React.createElement(PasswordProtection, { | ||
| value: { password: '', confirmPassword: '', enabled }, | ||
| onChange: () => undefined, | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| function visibilityToggles(html: string): string[] { | ||
| return html.match(/<button type="button" aria-label="Show password"[^>]*>/g) ?? []; | ||
| } | ||
|
|
||
| describe('PasswordProtection accessibility', () => { | ||
| it('keeps every password visibility toggle keyboard reachable and state-labeled', () => { | ||
| const toggles = visibilityToggles(render(true)); | ||
|
|
||
| expect(toggles).toHaveLength(2); | ||
| for (const toggle of toggles) { | ||
| expect(toggle).not.toContain('tabindex="-1"'); | ||
| expect(toggle).toContain('aria-pressed="false"'); | ||
| expect(toggle).toContain('aria-controls="password confirm-password"'); | ||
| } | ||
| }); | ||
|
|
||
| it('associates the password requirements hint with the password input', () => { | ||
| const html = render(true); | ||
|
|
||
| expect(html).toContain('id="password-requirements"'); | ||
| expect(html).toContain('aria-describedby="password-requirements"'); | ||
| }); | ||
|
|
||
| it('does not render password fields while protection is disabled', () => { | ||
| const html = render(false); | ||
|
|
||
| expect(html).not.toContain('id="password"'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,6 +251,12 @@ export function InviteMemberDialog({ | |
| handleInviteMember(); | ||
| } | ||
| }} | ||
| aria-invalid={shouldShowEmailError || emailDomainMatchesDirectSSODomain} | ||
| aria-describedby={ | ||
| shouldShowEmailError || emailDomainMatchesDirectSSODomain | ||
| ? 'invite-email-error' | ||
| : undefined | ||
| } | ||
| className={ | ||
| shouldShowEmailError || emailDomainMatchesDirectSSODomain | ||
| ? 'border-red-500 focus:border-red-500' | ||
|
|
@@ -267,10 +273,12 @@ export function InviteMemberDialog({ | |
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button | ||
| id="role" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Radix's Reply with |
||
| variant="outline" | ||
| size="sm" | ||
| className="flex h-10 items-center justify-between gap-2 px-3" | ||
| disabled={inviteMemberMutation.isPending} | ||
| aria-label={`Role: ${ROLE_LABELS[role]}`} | ||
| > | ||
| {ROLE_LABELS[role]} | ||
| <ChevronDown className="h-3 w-3" /> | ||
|
|
@@ -316,7 +324,7 @@ export function InviteMemberDialog({ | |
| clipRule="evenodd" | ||
| /> | ||
| </svg> | ||
| <p className="text-sm text-red-300" role="alert"> | ||
| <p id="invite-email-error" className="text-sm text-red-300" role="alert"> | ||
| {shouldShowEmailError && 'Please enter a valid email address'} | ||
| {emailDomainMatchesDirectSSODomain && ssoErrorText} | ||
| </p> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: Keep the toggle button's accessible name constant when using
aria-pressedaria-pressedalready exposes the toggled state, and the WAI-ARIA APG recommends keeping a toggle button's accessible name constant. Changing the name to "Hide password" whilearia-pressed="true"makes screen readers announce "Hide password, pressed", which is contradictory. Keep the name as "Show password" on both visibility toggles (this line and line 141) and letaria-pressedconvey the state.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.