[A11Y][D8] Added form API properties to support custom validation and error messages for required fields.

[klonos]

This is the respective issue for https://www.drupal.org/project/drupal/issues/742344, a blocker for the #5346 backport, and related to #1040.

Problem/Motivation

Accessibility guidelines recommend specific, meaningful error messages when required form elements are empty. However, FAPI currently does not support custom error messages for required elements.

Proposed resolution

Support a #required_error property for form elements. If set, this message will be used when a required form element is empty, instead of the default "Field x is required."
Additionally set a #required_but_empty flag automatically during validation when the required field is empty. This is an internal property and is not set manually. It allows #element_validate handlers to set a custom required error message, but without having to re-implement the complex logic to figure out whether the field value is empty.

Remaining tasks

Commit feature and document these properties. The feature is a small API improvement that is applicable to both D8 and D7.

User interface changes

None.

API changes

Minor, backwards-compatible addition only. No existing code will be affected. Addition of two FAPI element properties:

• #required_error
• #required_but_empty

Draft of feature description for Press Release (1 paragraph at most)

Backdrop now includes two new form API properties to support specific, meaningful error messages when required form elements are empty.

---

Advocate for this issue: @klonos

[klonos]

Minor, backwards-compatible addition only. No existing code will be affected. Addition of two FAPI element properties...

Can this be regarded as a task instead of a feature then? I'd love to have this (and also #5346) in place before starting work on #1040 for the 1.21.0 release.

[klonos]

To get this moving, in order to progress #1040, I've set the milestone label for the next minor release. Can I please get someone to add the actual milestone? Thanks.

PR ready for review/testing: backdrop/backdrop#3844
Draft/unpublished change record here: https://docs.backdropcms.org/change-records/new-form-api-properties-required_error-and-required_but_empty

[bugfolder]

Tested the PR with this form:

/**
 * Return a test form for custom errors.
 */
function backdrop_test_custom_error_form($form, &$form_state) {
  $form['backdrop_test_custom_error'] = array(
    '#type' => 'textfield',
    '#title' => t('Custom error test'),
    '#required' => TRUE,
    '#required_error' => t('You must fill in a value for the field.'),
//    '#element_validate' => array('backdrop_test_custom_error_form_validate'),
  );

  $form['submit'] = array(
    '#type' => 'submit',
    '#value' => t('Submit'),
    '#suffix' => l(t('Cancel'), 'backdrop-test-custom-error'),
  );
  return $form;
}

/**
 * Validation for custom error form testing.
 */
function backdrop_test_custom_error_form_validate($element, &$form_state) {
  if (isset($element['#required_but_empty'])) {
    form_error($element, t('Someone must fill in a value for the field.'));
  }
}

Worked as expected, both as above and with #element_validate uncommented.

Reviewed the code: LGTM.

[klonos]

Worked as expected...

Thanks @bugfolder 🙏🏼 ...based on your comment above, I've also se the "WFM" label.

[argiepiano]

Hey @klonos . I haven't had a chance to test this, but at quick glance, this PR may break a test that I added in PR backdrop/backdrop#3837 that fixed the issue with the field label not being displayed in the validation error for multi-value required fields. That test is in field.test in method testFieldFormUnlimitedRequired().

I'm not sure about this, but there may be other tests in that file that check for validation error specific wording that may also be affected. Sorry, I don't have time until tonight to check, but you may have caught those other ones already?

(Although on second thought this may not be an issue as those tests do not mess with #required_error and therefore the default validation error will be shown)

[klonos]

...this PR may break a test that I added in PR backdrop/backdrop#3837

That PR was merged a couple of hours ago, so let me rebase and test this again...

[klonos]

(Although on second thought this may not be an issue as those tests do not mess with #required_error and therefore the default validation error will be shown)

Well, most likely yes, but lets make sure. ...I've rebased and pushed. Tests are currently running, so let's see.

[klonos]

...OK, this was a clean rebase (no conflicts to resolve - no changes pushed), and all tests are still green. So we're good 👍🏼

[klonos]

Huh! ...interesting find while working on #5381:

search_box_form_submit():

...

  // Check to see if the form was submitted empty.
  // If it is empty, display an error message.
  // (This method is used instead of setting #required to TRUE for this field
  // because that results in a confusing error message. It would say a plain
  // "field is required" because the search keywords field has no title.
  // The error message would also complain about a missing #title field.)
  if ($form_state['values']['search_block_form'] == '') {
    form_set_error('keys', t('Please enter some keywords.'));
  }

...

Checking to see if that should be a follow-up issue to this one here, or if we can solve it with the change we're adding here...

[quicksketch]

Thanks @docwilmot and @indigoxela! Your testing and feedback is much appreciated.

@docwilmot I went ahead and changed that property name as you suggested and added code comments.

Further, I reverted more of the original changes to Search module because they caused a regression in intentional behavior. If the user clicks "Search" but does not enter any terms, the intentional behavior seems to have been to take the user to the /search page before showing the error message. This makes sense, and so I left the behavior in place but updated the code comments to explain why #required is not used.

[docwilmot]

Only one small language nitpick.

[quicksketch]

We didn't get this finalized in time for 1.22.0, so I'm bumping this to the next release.

[bugfolder]

Code reviewed. Aside from a few tiny wording/case suggestions in comments, all looks good.

[klonos]

So I'd be in favor of the following property names:

• #required_message (instead of the current #required_error)
• #required_value_missing (instead of the current #required_but_empty)

Sorry for not chiming in for so long here ...I chose to go with #required_error and #required_but_empty in order to keep some parity with the equivalent changes that are happening in Drupal-land. My thinking was that if these are somehow used in contrib and/or any documentation/tutorials or code snippets, then perhaps having the same naming conventions as Drupal would make it that less work is required for cross-/back-ports. Happy for us to go either way, and the proposed new names based on what these things are called in the JavaScript Constraint Validation API do sound better IMO 👍🏼

I actually found something related to the search form that might be broken. Details coming soon...

Any update on this?

...

...the search field is not actually required because it gets populated by other fields. It's totally confusing and not the way I think we'd handle this if we reworked this form...

If memory serves well (been a few months since), that's the search-related issue I mentioned I came across back in December last year. Totally confusing as you said @quicksketch, and at the time I didn't know how to proceed with it.

The search block can use the new #required_message property but the main search form cannot, because the main terms field is optional and may be populated by the advanced search fields instead. I added a code comment explaining why the field is not marked required.

Yes 🤔 ...I think that we should file a follow up issue to eventually fix things properly with the search form, but I suspect that we'd need to fix things like #2732 and perhaps other issues related to how required fields are (not) handled by #states.

I'll need to have a better look at the code changes, but please do not hold this on account of me.

[indigoxela]

@bugfolder you removed the "needs code review" label, but didn't add "rtbc" - why that?

[bugfolder]

Overlooked it.

[quicksketch]

Merged backdrop/backdrop#4054 into 1.x for 1.23.0! Thanks folks!

backdrop/backdrop@7e760e9 by @klonos, @bugfolder, @quicksketch, @docwilmot, and @indigoxela.