Fall back to first-error focus when the error summary isn't rendered - #3490
Conversation
js_validate's client-side validateFormSubmit() -> addAjaxFormErrors() never renders [data-frm-error-summary] - it only calls addFieldError() per field. checkForErrorsAndMaybeSetFocus() resolved focusErrorSummary true (the summary is "active" per filter, independent of js_validate) but focusFirstError false, so a failed submit on this path focused nothing at all - worse than the pre-#3408 behavior. Fall back to first-error-field focus when the summary was expected but isn't actually in the DOM, scoped to the submitting form's own container. Hand-patched js/formidable.min.js's matching function since SCRIPT_DEBUG is off in production/CI (same pattern PR#3408 used for this same file). Fixes Strategy11/formidable-pro#6763
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vivi-the-going-merry
Bot
added
run analysis
run e2e tests
Resolves conflict in the hand-patched js/formidable.min.js: keeps master's querySelectorAll expansion (.frm_error, [data-frm-error]) and previousSibling null guard alongside this branch's focusErrorSummary fallback condition.
|
CI: all green except two pre-existing/unrelated failures, both confirmed rather than assumed:
Self-review (code-review/security-review/simplify) run against the diff. One code-review finding investigated and ruled out: whether the new Ready for review. |
![franky-the-going-merry[bot]](https://avatars.githubusercontent.com/in/4233782?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approving. The fallback is correct and minimal: when focusErrorSummary is true but no [data-frm-error-summary] exists in the form (js_validate's client-side path never renders one), focus now falls through to the first errored field. Traced the config combinations: summary rendered still wins; both filters off still returns early; summary active with frm_focus_error_summary filtered off still resolves focusFirstError true server-side, unchanged. formidable.min.js matches the source change (node --check passes, same condition text). The changed Cypress spec (fieldsInFormBuilder-validation.cy.js) ran and passed in shard 2 on this head; the only red Cypress check is shard 1's FormTemplates.cy.js (not touched here), and Stylelint's failure is on a file this diff doesn't touch.
Non-blocking, three inline notes: one a11y gap with the same root cause that this fix doesn't cover, and two comment-length/style nits.
Not verified: the fix was not re-run in a browser by me (no local Cypress/wp-env run); evidence is the passing shard-2 CI run plus source tracing. The branch is 32 commits behind master.
| // Summary was resolved active server-side but isn't in the DOM on this path (e.g. | ||
| // js_validate's client-only validation never renders it) - fall back to the first | ||
| // error field instead of leaving focus nowhere. |
There was a problem hiding this comment.
Nit (non-blocking): the comment is 3 lines and sits dangling after the inner if closes. dev-rules allows 2 lines unless it's a real gotcha; this can say it in 2:
| // Summary was resolved active server-side but isn't in the DOM on this path (e.g. | |
| // js_validate's client-only validation never renders it) - fall back to the first | |
| // error field instead of leaving focus nowhere. | |
| // No summary in the DOM (js_validate's client-side path never renders one): fall | |
| // through to the first-errored-field focus below. |
| } | ||
|
|
||
| if ( ! config.focusFirstError ) { | ||
| if ( ! config.focusFirstError && ! config.focusErrorSummary ) { |
There was a problem hiding this comment.
Non-blocking, same root cause [Likely, source-traced, not run in a browser]: is_error_summary_active_for_form() is true regardless of js_validate, and should_include_alert_role_on_field_errors() defaults to false whenever the summary is active (FrmAppHelper.php ~4198). On the js_validate path addFieldError() (~1236) therefore builds <div class="frm_error" id=...> with no role="alert", and the summary that was supposed to announce the errors instead is never rendered. Net effect for a screen reader on a js_validate form: the first field gets focus and reads its aria-describedby error, but the other errors are never announced. This PR fixes the focus half of the summary/js_validate mismatch and leaves the announcement half. Worth a follow-up issue (either render the summary on the client path, or have getErrorConfigForForm()'s consumer keep role="alert" when the summary can't render); not required for this fix.
There was a problem hiding this comment.
Agreed, out of scope here. Leaving the role=alert/announcement half for a separate follow-up issue.
| // js_validate's client-side path never renders the error summary markup - focus must | ||
| // fall back to the first errored field instead of going nowhere (formidable-pro#6763). |
There was a problem hiding this comment.
Nit (non-blocking): the issue number in a code comment is changelog-style narration (it belongs in the PR description, which already has Fixes ...#6763). State the durable fact only:
| // js_validate's client-side path never renders the error summary markup - focus must | |
| // fall back to the first errored field instead of going nowhere (formidable-pro#6763). | |
| // js_validate's client-side path never renders the error summary markup, so focus | |
| // must fall back to the first errored field. |
vivi-the-going-merry
Bot
added
vivi-working
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
vivi-the-going-merry
Bot
added
franky-review
and removed
vivi-working
1 participant
What was broken
js_validate's client-side path (
validateFormSubmit()->addAjaxFormErrors(),js/formidable.js~2591) never renders the[data-frm-error-summary]markup - it only callsaddFieldError()per field.checkForErrorsAndMaybeSetFocus()still resolvedconfig.focusErrorSummarytrue (the summary is "active" per filter, independent of whetherjs_validateis enabled) whileconfig.focusFirstErrorwas forced false server-side (mutually exclusive with the summary, seeFrmAppHelper::resolve_error_focus_target()). Net effect: a failed submit on a form with "Validate Fields as User Completes Form" enabled focused nothing at all - worse than the pre-formidable-forms#3408behavior (first errored field).What changed
checkForErrorsAndMaybeSetFocus(): when the summary is expected (focusErrorSummarytrue) but isn't actually found in that error's own form container, fall through to the existing first-errored-field focus logic instead of returning early. No change when the summary is genuinely rendered (unaffected), and no change when an integration deliberately filters bothfrm_focus_first_errorandfrm_focus_error_summaryoff (still no auto-focus).js/formidable.min.js: hand-mirrored the same one-condition change into the matching minified function (js_suffix()serves this file wheneverSCRIPT_DEBUGis off) - not independently executed locally, verified by direct text diff against the source change plusnode --check.tests/cypress/e2e/Forms/fieldsInFormBuilder-validation.cy.js: added ahave.focusassertion to the existing "should validate forms with javascript setting" test, on the submit click where only the required Text field is invalid (email/phone already valid format there, avoiding native browser constraint-validation interference on the other submit click in the same test).How verified
Local: real Cypress run (
npx cypress run) against a localwp-envWordPress instance withjs_validateenabled via the form builder UI - confirmed red (assertion timeout) against unfixed code, green after the fix, and red again when the fix was temporarily reverted with the test kept. CI: see checks on this PR.Fixes Strategy11/formidable-pro#6763