From 6d7c2761f775269a1827bdac0a2388e8482fd3d1 Mon Sep 17 00:00:00 2001 From: TurtleWolfe Date: Fri, 21 Aug 2026 02:47:48 +0000 Subject: [PATCH] fix(#867): the last two auth forms now say which field they are about MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ForgotPasswordForm and ResetPasswordForm had the identical shape #866 fixed in SignInForm and SignUpForm: field-specific validation funnelled into a single form-level alert with no `id`, and no input carrying `aria-invalid` or `aria-describedby`. Leaving two of four forms behind was worse than doing all four or none. ResetPasswordForm is the sharper case — TWO conditions shared one string, a password that fails validatePassword and a confirmation that does not match, so "Passwords do not match" was announced with nothing saying which of the two boxes to fix. It is announced against the CONFIRMATION now: that is the one the user is being asked to change. Deliberately still form-level: the rate limit, the captcha challenge, the Supabase update failure, and the password-reset response — the last of those is generic on purpose so it cannot be used to confirm whether an address is registered. BOTH ACCESSIBILITY TEST FILES WERE GENERATOR STUBS THAT ASSERTED NOTHING `should have proper ARIA attributes` in each contained only example comments — rendering the form and asserting nothing, in the file whose job is accessibility. Two more instances of #396 on top of the two found in #866, which makes four of four auth forms shipped that way. The ResetPasswordForm test is table-driven over both conditions and checks the half that makes it worth having: the OTHER field must stay clean. An implementation that marked every input invalid would satisfy "the error is associated" and still tell the user nothing. MUTATION-VERIFIED, both directions: - routing the mismatch to `password` instead of `confirmPassword` fails exactly the mismatch case - removing `aria-describedby` while still rendering the message fails with "the email input must point at its error: expected null to be truthy" THREE THINGS THAT COST TIME, ALL THE SAME SHAPE 1. My first test password for the mismatch case was `LongEnough123`, which has no special character — so validatePassword rejected it FIRST, the mismatch branch never ran, and the test asserted against a state it never reached. Both values must pass validation for the mismatch to be reachable at all. 2. `passwordValidation.error` is `string | null`. The old `setError` accepted null; a typed field message does not. It falls back to a real sentence rather than a cast, so an invalid password without a message still says something. 3. My first patch attempt matched three of four anchors silently and wrote the file anyway. Each edit now asserts its own anchor AND that the anchor is unique — a patch that half-applies is the same defect class as a guard that half-matches. lint clean, 4734 vitest tests, tsc --noEmit clean. Closes #867 Co-Authored-By: Claude Opus 5 (1M context) --- .../ForgotPasswordForm.accessibility.test.tsx | 37 +++++++++-- .../ForgotPasswordForm/ForgotPasswordForm.tsx | 37 ++++++++++- .../ResetPasswordForm.accessibility.test.tsx | 61 ++++++++++++++++-- .../ResetPasswordForm/ResetPasswordForm.tsx | 62 +++++++++++++++++-- 4 files changed, 179 insertions(+), 18 deletions(-) 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 && (