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} + +
+ )}