diff --git a/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.accessibility.test.tsx b/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.accessibility.test.tsx index e88876ae..bd41ddb2 100644 --- a/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.accessibility.test.tsx +++ b/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.accessibility.test.tsx @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; -import { render } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { axe } from 'jest-axe'; import ForgotPasswordForm from './ForgotPasswordForm'; @@ -11,12 +12,36 @@ describe('ForgotPasswordForm Accessibility', () => { expect(results).toHaveNoViolations(); }); - it('should have proper ARIA attributes', () => { - const { container } = render(); + /** + * #867 — a validation error must be tied to the field it is about. + * + * This was a generator stub containing only example comments: it rendered the form + * and asserted NOTHING, in the file whose job is accessibility. Same shape as the two + * found in #866. + */ + it('ties the email validation error to the email field', async () => { + render(); + + const email = screen.getByLabelText(/email/i); + // `a@b` is deliberate: type="email" ACCEPTS it, so the browser's own validation does + // not block submission, but `validateEmail` rejects it (a dot in the domain and a 2+ + // alpha TLD are required). A value the browser rejects never reaches React at all. + await userEvent.type(email, 'a@b'); + await userEvent.click( + screen.getByRole('button', { name: /send reset link/i }) + ); + + // NOT findByRole('alert') — this form carries an empty live region, so that query + // resolves to a container with no text and the assertion proves nothing (#850). + // Follow the association instead; it is the property under test. + await waitFor(() => expect(email).toHaveAttribute('aria-invalid', 'true')); - // Add specific ARIA attribute tests based on component type - // Example: const button = container.querySelector('button'); - // expect(button).toHaveAttribute('aria-label'); + const describedBy = email.getAttribute('aria-describedby'); + expect(describedBy, 'the email input must point at its error').toBeTruthy(); + const slot = document.getElementById(describedBy!); + expect(slot, `no element with id="${describedBy}"`).not.toBeNull(); + expect(slot!.textContent?.trim()).toBeTruthy(); + expect(slot!.querySelector('[role="alert"]')).not.toBeNull(); }); it('should be keyboard navigable', () => { diff --git a/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.tsx b/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.tsx index b70f7283..f822f306 100644 --- a/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.tsx +++ b/src/components/auth/ForgotPasswordForm/ForgotPasswordForm.tsx @@ -35,6 +35,22 @@ export default function ForgotPasswordForm({ const supabase = createClient(); const [email, setEmail] = useState(''); const [error, setError] = useState(null); + /** + * The error for ONE named field, kept apart from `error` (#867, same shape as #857). + * + * `validateEmail` produces a message about the email box specifically, and it used to + * land in the form-level alert with no `id` while the input carried neither + * `aria-invalid` nor `aria-describedby` — so a screen reader announced it with nothing + * tying it to the field. + * + * The rate limit, the captcha challenge and the reset failure stay form-level: none is + * about one field, and the reset response is deliberately generic so it cannot confirm + * whether an address is registered. + */ + const [fieldError, setFieldError] = useState<{ + field: 'email'; + message: string; + } | null>(null); const [success, setSuccess] = useState(false); const [loading, setLoading] = useState(false); // (#353) SECURITY_CAPTCHA_ENABLED is global to Supabase auth — password @@ -45,12 +61,16 @@ export default function ForgotPasswordForm({ const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setError(null); + setFieldError(null); setSuccess(false); // Enhanced email validation (REQ-SEC-004) const emailValidation = validateEmail(email); if (!emailValidation.valid) { - setError(emailValidation.errors[0] || 'Invalid email address'); + setFieldError({ + field: 'email', + message: emailValidation.errors[0] || 'Invalid email address', + }); return; } @@ -134,11 +154,24 @@ export default function ForgotPasswordForm({ type="email" value={email} onChange={(e) => setEmail(e.target.value)} - className="input min-h-11" + className={`input min-h-11 ${ + fieldError?.field === 'email' ? 'input-error' : '' + }`} placeholder="you@example.com" required disabled={loading} + aria-invalid={fieldError?.field === 'email'} + aria-describedby={ + fieldError?.field === 'email' ? 'email-error' : undefined + } /> + {fieldError?.field === 'email' && ( +
+ + {fieldError.message} + +
+ )} {error && ( diff --git a/src/components/auth/ResetPasswordForm/ResetPasswordForm.accessibility.test.tsx b/src/components/auth/ResetPasswordForm/ResetPasswordForm.accessibility.test.tsx index 2c832336..42ccb300 100644 --- a/src/components/auth/ResetPasswordForm/ResetPasswordForm.accessibility.test.tsx +++ b/src/components/auth/ResetPasswordForm/ResetPasswordForm.accessibility.test.tsx @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; -import { render } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { axe } from 'jest-axe'; import ResetPasswordForm from './ResetPasswordForm'; @@ -11,12 +12,60 @@ describe('ResetPasswordForm Accessibility', () => { expect(results).toHaveNoViolations(); }); - it('should have proper ARIA attributes', () => { - const { container } = render(); + /** + * #867 — each validation error must reach the field it is about. + * + * This was a generator stub asserting nothing. TWO conditions used to funnel into one + * form-level alert with no `id` — a password that fails validation, and a mismatched + * confirmation — so "Passwords do not match" was announced with nothing saying which + * of the two boxes to fix. + */ + it.each([ + { + case: 'weak password', + fill: { password: 'short', confirm: 'short' }, + field: 'password', + other: 'confirm-password', + }, + { + case: 'mismatched confirmation', + // BOTH must pass validatePassword (8+, upper, lower, number, SPECIAL) or the + // password branch fires first and the mismatch is never reached — the test would + // then assert against a state it never got to. + fill: { password: 'LongEnough123!', confirm: 'Different123!' }, + // Announced against the CONFIRMATION box: that is the one being asked to change. + field: 'confirm-password', + other: 'password', + }, + ])('announces the $case against its own field', async (scenario) => { + render(); + + // Selected by id, not label text: the ids are exactly what the aria-describedby + // wiring under test refers to, and /password/i matches more than one control. + const byId = (id: string) => + document.getElementById(id) as HTMLInputElement; + + await userEvent.type(byId('password'), scenario.fill.password); + await userEvent.type(byId('confirm-password'), scenario.fill.confirm); + await userEvent.click( + screen.getByRole('button', { name: /reset password/i }) + ); + + const target = byId(scenario.field); + await waitFor(() => expect(target).toHaveAttribute('aria-invalid', 'true')); + + const describedBy = target.getAttribute('aria-describedby'); + expect(describedBy, 'the field must point at its error').toBeTruthy(); + const slot = document.getElementById(describedBy!); + expect(slot, `no element with id="${describedBy}"`).not.toBeNull(); + expect(slot!.textContent?.trim()).toBeTruthy(); + expect(slot!.querySelector('[role="alert"]')).not.toBeNull(); - // Add specific ARIA attribute tests based on component type - // Example: const button = container.querySelector('button'); - // expect(button).toHaveAttribute('aria-label'); + // The half that makes this worth having: the OTHER field stays clean. Marking every + // input invalid would satisfy the assertions above and tell the user nothing. + const other = byId(scenario.other); + expect(other).not.toHaveAttribute('aria-invalid', 'true'); + expect(other).not.toHaveAttribute('aria-describedby'); }); it('should be keyboard navigable', () => { diff --git a/src/components/auth/ResetPasswordForm/ResetPasswordForm.tsx b/src/components/auth/ResetPasswordForm/ResetPasswordForm.tsx index 1d80d172..12a6e439 100644 --- a/src/components/auth/ResetPasswordForm/ResetPasswordForm.tsx +++ b/src/components/auth/ResetPasswordForm/ResetPasswordForm.tsx @@ -25,20 +25,46 @@ export default function ResetPasswordForm({ const [password, setPassword] = useState(''); const [confirmPassword, setConfirmPassword] = useState(''); const [error, setError] = useState(null); + /** + * The error for ONE named field, kept apart from `error` (#867, same shape as #857). + * + * TWO conditions here were funnelled into one form-level alert with no `id`: a password + * that fails `validatePassword`, and a confirmation that does not match. Neither input + * carried `aria-invalid` or `aria-describedby`, so "Passwords do not match" was + * announced with nothing saying which of the two boxes to fix. + * + * The Supabase update failure stays form-level — it is not about one field. + */ + const [fieldError, setFieldError] = useState<{ + field: 'password' | 'confirmPassword'; + message: string; + } | null>(null); const [loading, setLoading] = useState(false); const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setError(null); + setFieldError(null); const passwordValidation = validatePassword(password); if (!passwordValidation.valid) { - setError(passwordValidation.error); + setFieldError({ + field: 'password', + // `error` is `string | null` on the validator's result; the field message is + // not optional, so an invalid password without a message still says something. + message: + passwordValidation.error ?? 'Password does not meet the requirements', + }); return; } if (password !== confirmPassword) { - setError('Passwords do not match'); + // Announced against the CONFIRMATION box, not the password: that is the one the + // user is being asked to change. + setFieldError({ + field: 'confirmPassword', + message: 'Passwords do not match', + }); return; } @@ -71,11 +97,24 @@ export default function ResetPasswordForm({ type="password" value={password} onChange={(e) => setPassword(e.target.value)} - className="input min-h-11" + className={`input min-h-11 ${ + fieldError?.field === 'password' ? 'input-error' : '' + }`} placeholder="••••••••" required disabled={loading} + aria-invalid={fieldError?.field === 'password'} + aria-describedby={ + fieldError?.field === 'password' ? 'password-error' : undefined + } /> + {fieldError?.field === 'password' && ( +
+ + {fieldError.message} + +
+ )}
@@ -87,11 +126,26 @@ export default function ResetPasswordForm({ type="password" value={confirmPassword} onChange={(e) => setConfirmPassword(e.target.value)} - className="input min-h-11" + className={`input min-h-11 ${ + fieldError?.field === 'confirmPassword' ? 'input-error' : '' + }`} placeholder="••••••••" required disabled={loading} + aria-invalid={fieldError?.field === 'confirmPassword'} + aria-describedby={ + fieldError?.field === 'confirmPassword' + ? 'confirm-password-error' + : undefined + } /> + {fieldError?.field === 'confirmPassword' && ( +
+ + {fieldError.message} + +
+ )}
{error && (