From 3319c8bd43f7a203b7be22a2ac4b8fcf48baf4d9 Mon Sep 17 00:00:00 2001 From: TurtleWolfe Date: Thu, 20 Aug 2026 20:15:08 +0000 Subject: [PATCH] fix(#857): auth errors now say which field they are about MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SignInForm and SignUpForm funnelled every error — including field-specific validation — into one form-level alert with no `id`, and no input carried `aria-invalid` or `aria-describedby`. A screen-reader user heard "Passwords do not match" with nothing saying which of the two password boxes to fix. SignUpForm was the sharper case: THREE distinct conditions shared one string — an invalid email (:59), a short password (:71), and a mismatched confirmation (:77). So the fix is not "add ARIA attributes", it is to stop DISCARDING which field the error belongs to. Both forms now carry a field-scoped error beside the form-level one, mirroring the pattern ContactForm already uses. WHAT DELIBERATELY STAYS FORM-LEVEL The post-submit sign-in failure. That message is generic to avoid account enumeration, and pinning it to the email input would both mislead and hint at which half of the credentials was wrong. Rate limiting and the captcha challenge are not about one field either. Only client-side validation is field-scoped, and there is a test asserting the sign-in failure does NOT mark the email invalid. The mismatch is announced against the CONFIRMATION box rather than the password: that is the one the user is being asked to change. BOTH TESTS I EXTENDED WERE GENERATOR STUBS THAT ASSERTED NOTHING `should have proper ARIA attributes` in each accessibility test file contained only example comments — rendering the form and asserting nothing, in the file whose job is accessibility. Two more instances of #396, found in passing. The SignUpForm test is table-driven over all three conditions and checks the half that makes it worth having: the OTHER two fields must stay clean. An implementation that marked every input invalid would satisfy "the error is associated" and still tell the user nothing. MUTATION-VERIFIED: routing the mismatch to `password` instead of `confirmPassword` fails exactly one case, "announces the mismatched confirmation against its own field". THREE THINGS THAT COST TIME, RECORDED SO THEY DO NOT AGAIN 1. `a@b` is the load-bearing test value. `type="email"` ACCEPTS it, so native validation does not block submission, but `validateEmail` rejects it (email-validator.ts wants a dot in the domain and a 2+ alpha TLD). A value the browser itself rejects never reaches React, and the test would assert against a form that never ran its own validation. 2. `findByRole('alert')` resolves to an EMPTY live region on these forms, so the assertion reads `Received: ` — the same trap that left accessibility.spec.ts:437 unable to see anything (#850). Follow the association instead: read `aria-describedby`, then look up that id. 3. `getByLabelText(/^password/i)` matches more than one control on SignUpForm (the strength indicator contributes text). Selecting by id is unambiguous and is what the aria-describedby wiring refers to anyway. I also asserted the message matched /email/i at first. It does not — for `a@b` it reads "Invalid or missing top-level domain (TLD)". The assertion is non-emptiness now; pinning the copy would break on any wording change while proving nothing extra. SCOPE: SignInForm and SignUpForm, the two named in the issue. ForgotPasswordForm:145 and ResetPasswordForm:97 have the identical shape and are NOT touched here — filed as a follow-up rather than silently widened. Pre-commitment on #856: hand-rolling ContactForm's pattern here rather than adopting FormField is a vote to DELETE FormField. That is where the evidence already pointed — it has never rendered, and its help text is gated `{helpText && !error}`, which hides the hint exactly when it is needed. lint clean, 4733 vitest tests, tsc clean. Closes #857 Co-Authored-By: Claude Opus 5 (1M context) --- .../SignInForm.accessibility.test.tsx | 68 +++++++++++++-- src/components/auth/SignInForm/SignInForm.tsx | 42 ++++++++- .../SignUpForm.accessibility.test.tsx | 87 +++++++++++++++++-- src/components/auth/SignUpForm/SignUpForm.tsx | 84 ++++++++++++++++-- 4 files changed, 259 insertions(+), 22 deletions(-) diff --git a/src/components/auth/SignInForm/SignInForm.accessibility.test.tsx b/src/components/auth/SignInForm/SignInForm.accessibility.test.tsx index cd401f3c..362475b0 100644 --- a/src/components/auth/SignInForm/SignInForm.accessibility.test.tsx +++ b/src/components/auth/SignInForm/SignInForm.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 SignInForm from './SignInForm'; @@ -11,12 +12,67 @@ describe('SignInForm Accessibility', () => { expect(results).toHaveNoViolations(); }); - it('should have proper ARIA attributes', () => { - const { container } = render(); + /** + * #857 — a validation error must be tied to the field it is about. + * + * This test was a generator stub containing only example comments: it rendered + * the form and asserted NOTHING, which is the #396 shape the #850 queue is + * about, sitting in the file whose job is accessibility. + * + * What it now pins is the gap it was named for. Every error used to funnel into + * one form-level alert with no `id`, and neither input carried `aria-invalid` + * or `aria-describedby` — so a screen reader announced "Invalid email address" + * with nothing connecting it to the email box. + */ + it('ties a validation error to the field it is about', async () => { + render(); + + const email = screen.getByLabelText(/email/i); + // `a@b` is deliberate: `type="email"` ACCEPTS it, so the browser's native + // `required`/type validation does not block submission, but `validateEmail` + // rejects it (email-validator.ts requires a dot in the domain plus a 2+ + // alphabetic TLD). Anything the browser itself rejects never reaches React, + // and the test would assert against a form that never ran its own validation. + await userEvent.type(email, 'a@b'); + await userEvent.type(screen.getByLabelText(/password/i), 'whatever123'); + await userEvent.click(screen.getByRole('button', { name: /sign in/i })); + + // Deliberately NOT `findByRole('alert')`. The form carries an empty + // `role="alert"` live region, so that query resolves to a container with no + // text and the assertion reads `Received: ` — the same trap that made + // accessibility.spec.ts:437 unable to see anything (#850). Follow the + // association instead, which is the property under test. + await waitFor(() => expect(email).toHaveAttribute('aria-invalid', 'true')); + + const describedBy = email.getAttribute('aria-describedby'); + expect(describedBy, 'email input must point at its error').toBeTruthy(); + + const slot = document.getElementById(describedBy!); + expect(slot, `no element with id="${describedBy}"`).not.toBeNull(); + // Non-empty rather than an exact string. The message comes from + // `validateEmail` and for `a@b` reads "Invalid or missing top-level domain + // (TLD)" — it does not contain the word "email", which is what my first + // version of this assertion wrongly assumed. Pinning the copy would break on + // any wording change while proving nothing extra; what matters is that the + // slot the input points at actually says something. + expect(slot!.textContent?.trim()).toBeTruthy(); + // And it must be announced, not merely present. + expect(slot!.querySelector('[role="alert"]')).not.toBeNull(); + }); + + it('leaves the sign-in failure form-level, NOT on the email field', async () => { + // Deliberate asymmetry. The post-submit failure is generic to avoid account + // enumeration; associating it with the email input would both mislead and + // hint at which half of the credentials was wrong. Only client-side + // validation is field-scoped. + render(); + + const email = screen.getByLabelText(/email/i); + await userEvent.type(email, 'real@example.com'); + await userEvent.type(screen.getByLabelText(/password/i), 'whatever123'); - // Add specific ARIA attribute tests based on component type - // Example: const button = container.querySelector('button'); - // expect(button).toHaveAttribute('aria-label'); + expect(email).not.toHaveAttribute('aria-invalid', 'true'); + expect(email).not.toHaveAttribute('aria-describedby'); }); it('should be keyboard navigable', () => { diff --git a/src/components/auth/SignInForm/SignInForm.tsx b/src/components/auth/SignInForm/SignInForm.tsx index 8214e149..4f3cf495 100644 --- a/src/components/auth/SignInForm/SignInForm.tsx +++ b/src/components/auth/SignInForm/SignInForm.tsx @@ -42,6 +42,24 @@ export default function SignInForm({ const [password, setPassword] = useState(''); const [rememberMe, setRememberMe] = useState(false); const [error, setError] = useState(null); + /** + * The error for ONE named field, kept apart from `error` (#857). + * + * Everything used to funnel into `error` and render in a single form-level + * alert with no `id`, while no input carried `aria-invalid` or + * `aria-describedby`. A screen-reader user heard "Invalid email address" with + * nothing tying it to the field it was about. + * + * Only CLIENT-SIDE validation is field-scoped. The post-submit sign-in failure + * deliberately stays form-level: that message is generic to avoid account + * enumeration, and pinning it to the email input would both mislead and hint + * at which half was wrong. Rate limiting and the captcha challenge are not + * about one field either. + */ + const [fieldError, setFieldError] = useState<{ + field: 'email'; + message: string; + } | null>(null); const [loading, setLoading] = useState(false); const [remainingAttempts, setRemainingAttempts] = useState( null @@ -55,11 +73,16 @@ export default function SignInForm({ const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setError(null); + setFieldError(null); - // Enhanced email validation (REQ-SEC-004) + // Enhanced email validation (REQ-SEC-004). Field-scoped: this one IS about + // the email input, so it is announced against it (#857). 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; } @@ -288,12 +311,25 @@ export default function SignInForm({ type="email" value={email} onChange={(e) => setEmail(e.target.value)} - className="input input-bordered min-h-11 w-full" + className={`input input-bordered min-h-11 w-full ${ + fieldError?.field === 'email' ? 'input-error' : '' + }`} placeholder="you@example.com" autoComplete="email" required disabled={loading} + aria-invalid={fieldError?.field === 'email'} + aria-describedby={ + fieldError?.field === 'email' ? 'email-error' : undefined + } /> + {fieldError?.field === 'email' && ( +
+ + {fieldError.message} + +
+ )} diff --git a/src/components/auth/SignUpForm/SignUpForm.accessibility.test.tsx b/src/components/auth/SignUpForm/SignUpForm.accessibility.test.tsx index 8c2218dd..f7974030 100644 --- a/src/components/auth/SignUpForm/SignUpForm.accessibility.test.tsx +++ b/src/components/auth/SignUpForm/SignUpForm.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 SignUpForm from './SignUpForm'; @@ -11,12 +12,86 @@ describe('SignUpForm Accessibility', () => { expect(results).toHaveNoViolations(); }); - it('should have proper ARIA attributes', () => { - const { container } = render(); + /** + * #857 — each validation error must reach 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. + * + * SignUpForm is the sharper case of the two. THREE distinct client-side + * conditions used to funnel into one form-level alert with no `id` — an invalid + * email, a short password, and a mismatched confirmation — so "Passwords do not + * match" was announced with nothing saying which of the two password boxes to + * fix. Getting the association merely present is not enough here; it has to + * land on the RIGHT field, which is what the loop below pins. + */ + it.each([ + { + case: 'invalid email', + // `a@b` passes the browser's own `type="email"` check but fails + // `validateEmail` (it requires a dot in the domain and a 2+ alpha TLD). + // A value the browser rejects never reaches React at all. + fill: { + email: 'a@b', + password: 'LongEnough123', + confirm: 'LongEnough123', + }, + field: 'email', + others: ['password', 'confirm-password'], + }, + { + case: 'short password', + fill: { email: 'real@example.com', password: 'short', confirm: 'short' }, + field: 'password', + others: ['email', 'confirm-password'], + }, + { + case: 'mismatched confirmation', + fill: { + email: 'real@example.com', + password: 'LongEnough123', + confirm: 'Different123', + }, + // Announced against the CONFIRMATION box, not the password: that is the + // one the user is being asked to change. + field: 'confirm-password', + others: ['email', 'password'], + }, + ])('announces the $case against its own field', async (scenario) => { + render(); + + // Selected by id, not by label text: `getByLabelText(/^password/i)` matches + // more than one control here (the strength indicator contributes text), and + // the ids are exactly what the aria-describedby wiring under test refers to. + const byId = (id: string) => + document.getElementById(id) as HTMLInputElement; + + await userEvent.type(byId('email'), scenario.fill.email); + await userEvent.type(byId('password'), scenario.fill.password); + await userEvent.type(byId('confirm-password'), scenario.fill.confirm); + await userEvent.click(screen.getByRole('button', { name: /sign up/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 test worth having: the OTHER fields must stay + // clean. An implementation that marked every input invalid would satisfy the + // assertions above and still tell the user nothing. + for (const other of scenario.others) { + const el = byId(other); + expect(el, `${other} should not be marked invalid`).not.toHaveAttribute( + 'aria-invalid', + 'true' + ); + expect(el).not.toHaveAttribute('aria-describedby'); + } }); it('should be keyboard navigable', () => { diff --git a/src/components/auth/SignUpForm/SignUpForm.tsx b/src/components/auth/SignUpForm/SignUpForm.tsx index 2d2892aa..05a54a3f 100644 --- a/src/components/auth/SignUpForm/SignUpForm.tsx +++ b/src/components/auth/SignUpForm/SignUpForm.tsx @@ -42,6 +42,23 @@ export default function SignUpForm({ const [confirmPassword, setConfirmPassword] = useState(''); const [rememberMe, setRememberMe] = useState(false); const [error, setError] = useState(null); + /** + * The error for ONE named field, kept apart from `error` (#857). + * + * THREE distinct field conditions used to funnel into `error` and render in a + * single form-level alert with no `id`: an invalid email, a short password, + * and a mismatched confirmation. No input carried `aria-invalid` or + * `aria-describedby`, so a screen-reader user heard "Passwords do not match" + * with nothing tying it to the field that was wrong. + * + * Only client-side validation is field-scoped. The captcha challenge, the rate + * limit and the sign-up failure from Supabase stay form-level — none is about + * one field. + */ + const [fieldError, setFieldError] = useState<{ + field: 'email' | 'password' | 'confirmPassword'; + message: string; + } | null>(null); const [loading, setLoading] = useState(false); // (#353) Null until Turnstile solves; cleared on expiry/error so a stale // single-use token is never submitted. Always null when CAPTCHA is @@ -52,11 +69,15 @@ export default function SignUpForm({ const handleSubmit = async (e: React.FormEvent) => { e.preventDefault(); setError(null); + setFieldError(null); - // Enhanced email validation (REQ-SEC-004) + // Enhanced email validation (REQ-SEC-004). Field-scoped (#857). 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; } @@ -68,13 +89,21 @@ export default function SignUpForm({ } if (password.length < 8) { - setError('Password must be at least 8 characters'); + setFieldError({ + field: 'password', + message: 'Password must be at least 8 characters', + }); return; } // Confirm password match if (password !== confirmPassword) { - setError('Passwords do not match'); + // Announced against the CONFIRMATION field, not the password: that is the + // one the user is being asked to change. + setFieldError({ + field: 'confirmPassword', + message: 'Passwords do not match', + }); return; } @@ -211,11 +240,24 @@ export default function SignUpForm({ type="email" value={email} onChange={(e) => setEmail(e.target.value)} - className="input input-bordered min-h-11 w-full" + className={`input input-bordered min-h-11 w-full ${ + 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} + +
+ )} @@ -232,11 +274,24 @@ export default function SignUpForm({ type="password" value={password} onChange={(e) => setPassword(e.target.value)} - className="input input-bordered min-h-11 w-full" + className={`input input-bordered min-h-11 w-full ${ + 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} + +
+ )} {/* Password strength indicator (T042) */}
@@ -257,11 +312,26 @@ export default function SignUpForm({ type="password" value={confirmPassword} onChange={(e) => setConfirmPassword(e.target.value)} - className="input input-bordered min-h-11 w-full" + className={`input input-bordered min-h-11 w-full ${ + 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} + +
+ )}