Skip to content

Address remaining review items on PR #3489 (subfield error handling) #3516

Description

@robin-the-going-merry

Requested by: @Crabcyborg

Follow-up on the open review items Franky reported in #3489 ("Improve subfield error handling"). Vivi already fixed the two blocking JS bugs and regenerated js/formidable.min.js; Franky approved round 2. This issue covers what is still open. Push to the existing PR branch improve_subfield_error_handling (same PR, no new PR). The PR currently shows merge conflicts with its base, so resolve those first.

Read Franky and CodeRabbit inline threads on the PR for full detail. Summary:

Check first (not fully confirmed)

  • Repeater + AJAX submit: addAjaxFormErrors() (js/formidable.js ~line 114) looks up #frm_field_${key}_container and deletes the error when nothing matches. For a Name/Address field inside a Repeater, the new suffixed key may not match a container, so the error is dropped and submit can go through with no error shown. Reproduce with a Repeater-wrapped Name/Address field, AJAX submit, partial fill. If confirmed, resolve the container from the sub-input, like validateComboField() does.

Fix

  • aria-describedby mismatch with custom data-error-html (js/formidable.js ~line 1423): getComboFieldRequiredMessage() and the id computed there derive the error key separately, so the id points at frm_error_field_<key>_first while the rendered HTML has frm_error_field_<key>. Derive both from one source.
  • Live validation gaps (js/formidable.js ~line 475): the first blur/change on a required combo field shows no error until a submit has happened once. Also, when js_validate is off, the ! addErrors && hasErrors early return clears only the changed sub-field, leaving the outer wrapper combined error stuck.
  • AJAX [if error] wrapper: non-numeric keys like field12-first skip FrmEntriesAJAXSubmitController::maybe_modify_ajax_error() (classes/controllers/FrmEntriesAJAXSubmitController.php ~line 174), so per-sub-field errors miss the custom wrapper.
  • CSS (css/_single_theme.css.php ~line 396): the rule that keeps normal styling on non-error sub-fields only covers input/select. Also cover textarea, .mce-edit-area iframe, recaptcha, the Stripe card element, and the chosen.js containers.
  • FrmFieldCombo::get_sub_field_label() (classes/models/fields/FrmFieldCombo.php ~line 492) uses the {name}_desc text as the subject of "cannot be blank." A full-sentence description gives a broken message. Fall back to the plain sub-field label when it is not a short phrase.

Cleanup

  • removeComboFieldErrors() (~line 556): call removeFieldError( fieldContainer ) instead of hand-rolling the same cleanup.
  • Reword the stale comment near ~line 430 ("sub fields share the field key, so this is replaced below"); the getFieldId() fix already removed that collision.
  • Reuse: getComboFieldRequiredMessage re-derives a key that getFieldContainerErrorKey already computes; maybeCombineComboFieldErrors repeats checkRequiredField and the sub-input DOM query; the "clone field, override name, call get_error_msg" pattern is duplicated between FrmFieldCombo::get_sub_field_error_msg() and FrmFieldsController::add_validation_messages().

Regenerate js/formidable.min.js (npm run minimize) in the same commit as any js/formidable.js change.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions