Skip to content

fix(Form): check every name in isFieldInvalid; guard and focus scrollToFirstError - #61

Merged
hugomallet merged 2 commits into
masterfrom
fix/form-invalid-scroll
Aug 6, 2026
Merged

hugomallet merged 2 commits into
masterfrom
fix/form-invalid-scroll

Conversation

@hugomallet

Copy link
Copy Markdown
Collaborator

Fixes

  • isFieldInvalid: isFieldInvalid(['a', 'b']) only checked the first name (getFieldErrors(...tuple) dropped the rest). Now passes the array directly — getFieldErrors already accepts one.
  • scrollToFirstError guard: when no DOM element carries the first error's name (custom field, required error on a nested object), undefined was forwarded to scrollToElement. Now returns early.
  • Keyboard a11y (Audit 2026-07 : améliorations (accessibilité, bugs helpers, dépendances, outillage) #57): after scrolling, focus moves to the first invalid field via element.focus({ preventScroll: true }).

Tests

3 new tests, each verified to fail against the un-fixed code:

  • multi-name isFieldInvalid with the error on the second name only
  • invalid submit with no matching DOM element does not throw
  • after an invalid submit, document.activeElement is the invalid field

CI=true yarn test:runtime: 6 suites, 66 tests (63 on master), 5 snapshots — all green.

@hugomallet hugomallet left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review (tooled self-review)

Disclosure: PR authored under my own account, so GitHub won't accept an APPROVE — this is a tooled self-review posted as a comment. Review ran on a clean worktree: ESLint + tsc --noEmit clean, full suite green (6 suites, 66 tests, 5 snapshots).

Verdict: mergeable — minor comments only, none blocking.

Verified

  • isFieldInvalid multi-name: passing names straight to getFieldErrors yields the union of filterByFieldNameWithWildcard per name — correct OR semantics, wildcards (user.*) preserved.
  • Guard: on the submit path errors[0] is always defined (handleSubmitError only runs when valid is false; AJV guarantees non-empty errors then, and formatErrors maps 1:1 without filtering).
  • Focus: a non-focusable element carrying name (e.g. a div) makes focus() a harmless no-op; preventScroll correctly avoids fighting the 900ms animated scroll.
  • Scope: strictly Form.js + Form.test.js, no snapshot change needed — confirmed by the full run.

Minor (grouped, take or leave)

  1. scrollToFirstError direct call (Form.js:343): a consumer calling it via instance ref on a valid form hits firstError.field → TypeError. One-line hardening: if (!firstError) return; above the DOM lookup.
  2. preventScroll on old Safari (< 15, still within >0.2% browserslist): the option is ignored, so focus triggers an instant scroll competing with the animation. Graceful degradation — fine as is; focusing after the scroll duration would fix it if it ever matters.
  3. Focus test cleanup (Form.test.js): wrapper.detach() / removeChild run after the assertion, so a failing expect leaks the attached container into later tests. try/finally (or afterEach) would make it robust.
  4. Micro: isFieldInvalid normalizes fieldNames into names, then getFieldErrors re-normalizes — the local array could be dropped.

Aside (pre-existing, out of scope)

The should match snapshot test fails when the suite runs alone (--testPathPattern Form/Form.test.js): the snapshot serializes the AJV instance, which embeds jest's console (BufferedConsole vs CustomConsole depending on worker mode). Not caused by this PR — worth a follow-up to stop snapshotting the AJV instance.

hugomallet added a commit that referenced this pull request Aug 6, 2026
- scrollToFirstError: return early when the form has no errors, so a
  direct call through a ref on a valid form no longer throws.
- isFieldInvalid: drop the redundant array normalization, already done
  by getFieldErrors.
- focus test: cleanup (detach + remove container) moved to a finally
  block so a failing assertion no longer leaks the attached container.
…ToFirstError

- isFieldInvalid([a, b]) now checks every name instead of only the
  first: pass the array straight to getFieldErrors, which already
  accepts one.
- scrollToFirstError no longer forwards undefined to scrollToElement
  when no DOM element carries the first error's name (custom field,
  required error on a nested object).
- After scrolling, focus moves to the first invalid field
  (element.focus({ preventScroll: true })) for keyboard users (#57).
- scrollToFirstError: return early when the form has no errors, so a
  direct call through a ref on a valid form no longer throws.
- isFieldInvalid: drop the redundant array normalization, already done
  by getFieldErrors.
- focus test: cleanup (detach + remove container) moved to a finally
  block so a failing assertion no longer leaks the attached container.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant