Add a #disable_inline_form_errors_summary property to disable the Inline Form Errors summary

Thanks for the request and your work, there is already an issue for this however. #2856950: Add a possibility to disable inline form errors for a complete form

@nikunjkotecha - I think your use case needs more detail.

we need inline form errors but not the errors on top

Please clarify: what do you expect to see at the top of the page? A design mockup (or screenshot with notes) may help here.

At minimum, we should have a red message block which says: "there are errors in the form." Otherwise a user would need to read through the entire form again, as far as the first error, to even know there was an error present at all.

I don't think this qualifies as major, until we get a better idea of what is being proposed.

Thanks @nikunjkotecha for the explanation and your work.
We have an earlier request that asks for the same #2841040: Allow disabling the Inline Form Errors summary (yep, found another one), I'm closing that one in favor of this because there has been more progress here.

For me it is a clear enough request now, but I have some additional thoughts.
I guess we also want to be able to prevent the summary for only partial AJAX form replacements. So allow the summary for the complete form, but disable it when for example only a fieldset is reloaded/submitted/validated via AJAX.

I think the property error_no_message is not clear enough, or can be confusing. The whole property is a bit of a mess anyhow. I think it was introduced to counter the error bubbling behaviour and display of duplicate messaged for compound fields like dates and checkboxes. Currently it means something like: highlight the field/RenderElement as having an error, but don't show the inline error message and don't show a summary link to it. This meaning wouldn't apply to the usage proposed by @nikunjkotecha, since it would hide a non-inline message.

For example disable_inline_form_errors_summary, is already more fitting, and more in line with #2856950: Add a possibility to disable inline form errors for a complete form.
This could applied to the whole form or to any other child RenderElement and their children.
Let's discuss this and update the IS accordingly.

In any case this would need tests.

The last patch works as expected.
I suggest to add a settings form where can configure on which form i want the inline form feature, and where i don't want the summary.
this will be really better.
Thanks,

1. +++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
   @@ -88,6 +88,10 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
          if (!empty($form_element['#error_no_message'])) {
   ...
   +      elseif (!empty($form['#disable_inline_form_errors_summary'])) {
   

   We need to add an inline_form_errors.api.php where we can document how both of these work.

2. +++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
   @@ -169,4 +169,34 @@ public function testDisplayErrorMessagesNotInline() {
   +    $form_state = new FormState();
   +    $form_state->setErrorByName('test', 'invalid');
   +    $form_error_handler->handleFormErrors($form, $form_state);
   +
   +    $form_error_handler->expects($this->never())->method('drupalSetMessage');
   

   This is a negative test we should assert the case where drupalSetMessage gets called here too so that if this changes we know to change this test.

3. And we still need an issue summary update

There may be some field that does not display the inline error, only in the summary. As an example the Captcha field.
I modified the patch to not display in the summary the fields that contain errors in line, but to display the others, so that they are not hidden.

Re-roll the patch from #13 as #19 does not apply.
I also add the changes suggested by #19.

1. We still need to add an inline_form_errors.api.php where we can document how both of these work ;
2. We still need an issue summary update ;
3. I'm not sure how I apply the suggestion of #17.2 (so please review my changes, any feedback appreciate).

1. Test testDisplayErrorMessagesNoSummary() needs work. It passes even if #disable_inline_form_errors_summary is FALSE.

$form = [
  '#parents' => [],
  '#disable_inline_form_errors_summary' => FALSE,
  '#array_parents' => [],
];

Testing Drupal\Tests\inline_form_errors\Unit\FormErrorHandlerTest
....                                                                4 / 4 (100%)

Time: 3.33 seconds, Memory: 6.00MB

OK (4 tests, 6 assertions)

2. An additional condition (see note 2 below) is not necessary.

--- b/core/modules/inline_form_errors/src/FormErrorHandler.php
+++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
@@ -88,18 +88,21 @@
+      // Do not show links/summary for the whole form if disabled.
+      elseif (!empty($form['#disable_inline_form_errors_summary'])) {
+        unset($errors[$name]);
+      }
       elseif ($is_visible_element && $has_title && $has_id) { // Note 1: here always empty($form['#disable_inline_form_errors_summary'])
-        $error_links[] = Link::fromTextAndUrl($title, Url::fromRoute('<none>', [], ['fragment' => $form_element['#id'], 'external' => TRUE]))->toRenderable();
+        // Do not show links/summary for fields with inline errors if disabled.
+        if (empty($form['#disable_inline_form_errors_summary'])) { // Note 2: not necessary
+          $error_links[] = Link::fromTextAndUrl($title, Url::fromRoute('<none>', [], ['fragment' => $form_element['#id'], 'external' => TRUE]))->toRenderable();
+        }

It is similar to the following code:

elseif (!empty($form['#disable_inline_form_errors_summary'])) {
  unset($errors[$name]);
}
elseif ($is_visible_element && empty($form['#disable_inline_form_errors_summary'])) {

3. If #disable_inline_form_errors_summary is set to TRUE $errors and $error_links will be empty after loop. This means that the code behind the loop will not show messages.
In this case, I think it is better to check #disable_inline_form_errors_summary at the beginning, instead of doing it in each iteration.

protected function displayErrorMessages(array $form, FormStateInterface $form_state) {
  // Skip generating inline form errors when opted out.
  if (!empty($form['#disable_inline_form_errors'])) {
    parent::displayErrorMessages($form, $form_state);
    return;
  }

  // Do not show links/summary for the whole form if disabled.
  if (!empty($form['#disable_inline_form_errors_summary'])) {
    return;
  }

Thanks for your review !

I mad a huge mistake on #22 by not reading properly #19.
I had to change #22 for the reasons explain by #19 - I'm sorry for my mistake.

Because we had this issue, #23.2 & #23.3 was then non applicable.

Here are my changes :

- Use the patch from #19 instead of #13
- Refactor the tests of #20

1. We still need to add an inline_form_errors.api.php where we can document how both of these work ;
2. We still need an issue summary update ;
3. I'm not sure how I apply the suggestion of #17.2 (so please review my changes, any feedback appreciate).

Use Link::fromTextAndUrl instead of l function.

1. We still need to add an inline_form_errors.api.php where we can document how both of these work (any example of issue doing it well would help me or the next one to finalize this step) ;
2. We still need an issue summary update ;
3. I'm not sure how I apply the suggestion of #17.2 (so please review my changes, any feedback appreciate).

Add the API changes on issue summary.

I was reviewing and this works great! I have suggestion.

1. Now we're testing the form with only one field in the testDisplayErrorMessagesInlineNoSummary() and testDisplayErrorMessagesNotInlineWithSummary().

I think we should also test the remaining fields with errors (e.g. missing or invisible fields).

Can we define a multi-field form in the setUp() method?

protected function setUp() {
  parent::setUp();

  $form = [
    '#parents' => [],
    '#form_id' => 'test_form',
    '#array_parents' => [],
  ];
  $form['test1'] = [
    '#type' => 'textfield',
    '#title' => 'Test 1',
    '#parents' => ['test1'],
    '#array_parents' => ['test1'],
    '#id' => 'edit-test1',
  ];


  (...)


  $form['test6'] = [
    '#type' => 'value',
    '#title' => 'Test 6',
    '#parents' => ['test6'],
    '#array_parents' => ['test6'],
    '#id' => 'edit-test6',
  ];

  $this->form = $form;

Then check more test cases.

public function testDisplayErrorMessagestInlineNoSummary() {

  (...)

  $this->form['#disable_inline_form_errors_summary'] = TRUE;

  $form_state = new FormState();
  $form_state->setErrorByName('test1', 'invalid');
  $form_state->setErrorByName('test2', 'invalid');
  $form_state->setErrorByName('fieldset][test3', 'invalid');
  $form_state->setErrorByName('test4', 'no error message');
  $form_state->setErrorByName('test5', 'no title given');
  $form_state->setErrorByName('test6', 'element is invisible');
  $form_state->setErrorByName('missing_element', 'this missing element is invalid');
  $this->formErrorHandler->handleFormErrors($this->form, $form_state);

  // Assert the inline error still exists.
  $this->assertSame('invalid', $this->form['test1']['#errors']);

2. Typo in the function name.

--- a/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
+++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
@@ -210,7 +210,7 @@ public function testDisplayErrorMessagesNotInlineWithSummary() {
   /**
    * Tests that disabling Inline Form Errors summary works.
    */
-  public function testDisplayErrorMessagestInlineNoSummary() {
+  public function testDisplayErrorMessagesInlineNoSummary() {

Thanks for review:

I think we should also test the remaining fields with errors (e.g. missing or invisible fields).

Yep why not.

Can we define a multi-field form in the setUp() method?

It would be great to have only 1 form generation (the one used in the first test seems pretty good & complete). Then use it in every other tests.
But it would be an out-of-scoop change. We should create a child-issue which focuses on this change (make the tests easier to read by using only 1 Form in the Setup). Then refactor our Class & tests added here.
I don’t want to apply any change on already existing tests here (refactoring of Setup function) which is out-of-scoop of this issue.

What do you think ? Can you create this issue & start the refactoring Class (form creation in Setup). Then we will be able to backport this change here & progress on this issue ?

2. Typo in the function name.

Yep sorry ^^

But it would be an out-of-scoop change. We should create a child-issue which focuses on this change.

Good point!

#3027318: Improve test coverage for Inline Form Errors

Update completed tasks & remaining ones.

Add links to blocker issue #3027318

The blocker issue #3027318: Improve test coverage for Inline Form Errors has been fixed.

#26 needs a reroll

Rerolled #26 against 8.8, fixed typo and updated tests.

1. Addressed issue #29.

2. Still needs inline_form_errors.api.php (#17) so back to "Needs work".

I added inline_form_errors.api.php.

Worked fine for me.
RTBC

I added change record Allow disabling the Inline Form Errors summary.

Unrelated test failure. Back to RTBC.

Hello, I've just tested this myself and it works great. I already have use cases for it too. Another vote for being committed.

Hello, I am using this for my forms on 8.8.4 and it works great!

Hello, it worked for us as well on 8.7.9 thanks!

Apologies for the delayed patch review here.

1. +++ b/core/modules/inline_form_errors/inline_form_errors.api.php
   @@ -0,0 +1,50 @@
   + * The #error_no_message property, allows to disable error messages for
   + * individual items. Set the #error_no_message to TRUE for the specific element:
   ...
   + *   '#error_no_message' => TRUE,
   

   @dmsmidt said in #11 that he did not think this was a sufficiently clear machine name, but that's already present in HEAD, so out of scope to change it here.

2. +++ b/core/modules/inline_form_errors/inline_form_errors.api.php
   @@ -0,0 +1,50 @@
   + * If one or more elements with errors can't show inline-error (e.g. invisible,
   + * missing or with no title elements), the summary will remain visible with
   

   If one or more elements with errors
   can't display inline errors (for example, it is invisible, missing, or has no title elements)...

3. +++ b/core/modules/inline_form_errors/inline_form_errors.api.php
   @@ -0,0 +1,50 @@
   + * See https://www.drupal.org/docs/8/core/modules/inline-form-errors for more
   + * information about the Inline Form Errors module.
   

   The URL here should probably use @link/@endlink.

4. +++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
   @@ -95,7 +95,11 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +        // Do not show links/summary for fields with inline errors if disabled.
   +        // Still keep message for not visible field.
   

   If inline errors are disabled, do not show the links and summary. Retain the message if the field is not shown or cannot display inline errors.

5. +++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
   --- a/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
   +++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
   

   Hmm, a unit test does not seem like the right choice here. That said, since the module is already using unit tests, that might be out of scope. I would have used a browser test, or additionally provided a browser test.

6. +++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
   @@ -214,4 +214,50 @@ public function testErrorMessagesNotInline() {
   +    // Asserts messages of non-visible or missing element and with no title are
   

   Should be "missing elements" (plural).

   "and with no title" also doesn't make sense in context.

   Maybe the comment should be:

   Assert that messages are summarized for elements that have both no title and are missing or invisible.

Retitling to make it clear that this is API-only, not a UI feature. Thanks!

Working on this.

Thanks you for picking this up again @xjm, and all the work by the others!

But... from what I read, I'm not convinced these changes are clear enough.
Inline Form Errors adds multiple API only properties to allow for certain edge cases. Sadly these are needed because of how the form-api and rendering trees work. These edge cases are legit, so we should account for them.
And it needs to be totally clear what the different IFE Form API properties do and when to use what.
From the review from @xjm I take away we describe the workings of the properties not clear enough in the code and DO documentation.

1

For example #52 point 4:

+++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
@@ -95,7 +95,11 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
+        // Do not show links/summary for fields with inline errors if disabled.
+        // Still keep message for not visible field.
+        if (empty($form['#disable_inline_form_errors_summary'])) {
+          $error_links[] = Link::fromTextAndUrl($title, Url::fromRoute('<none>', [], ['fragment' => $form_element['#id'], 'external' => TRUE]))->toRenderable();
+        }

One: We are creating too much code complexity here with nested if/elses.
Two: Because of the complexity we need to add comments.
Three: The proposed rewriting of the comment to `If inline errors are disabled, do not show the links and summary. Retain the message if the field is not shown or cannot display inline errors.` misses the point. We are not disabling IFE, just trying to disable summary of inline errors.

So, let's create clearly named helper methods and/or move the logic to the point where we add the summary message.

2

Also we still need an IS and Change Record update. It looks like 'duplicate messages' are the problem. But that is IMHO not what we are trying to do. Because IFE doesn't make duplicate messages, it just also generates a summary (one message) of all inline form errors with fragment/hash/anchor links to them.

3
Further, from the IS and CR 'There may be some field that does not display the inline error, so the message is only visible in the summary.'
If a form element error is not 'inline', it is never in the summary message to begin with right? It will just have its own message outside the 'summary message' in the area we print messages. So this is a non-concern right? Let's update the IS/CR.

4
Last but not least about the current test.
I partly agree with @xjm this test would probably be easier to understand as a functional/browser test.
Unit test is faster though ;-) So as long we are confident and it is clear I'm ok with it.

(4.1) Please do show a test only failing patch so that we know the fix works without trying ourselves.

(4.2)

+++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
@@ -214,4 +214,50 @@ public function testErrorMessagesNotInline() {
+   * Tests that disabling Inline Form Errors summary works.
+   */
+  public function testDisplayErrorMessagesInlineNoSummary() {

The description is far more clear than the method name, but we can improve both.
'testDisabledInlineErrorsSummary'
`Tests that disabling the inline errors summary works`

(4.3)

+++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
@@ -214,4 +214,50 @@ public function testErrorMessagesNotInline() {
+    // Asserts messages of non-visible or missing element and with no title are
+    // summarized.

Euh? This is not what we want to test. Just test we don't have a summary at all. Other errors should have their own normal non-inline messages. Test one thing per test method and we don't need comments.

(4.4)

+++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
@@ -214,4 +214,50 @@ public function testErrorMessagesNotInline() {
+      ->method('renderPlain');

So this checks there is no summary rendered? Not really obvious what is testing that there is no summary. Could use a comment.

(4.5)

+++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
@@ -214,4 +214,50 @@ public function testErrorMessagesNotInline() {
+    $form_state->setErrorByName('test4', 'no error message');

Funky message: this is no message. Just use a clear 'fake' message.

(4.5)

+++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
@@ -214,4 +214,50 @@ public function testErrorMessagesNotInline() {
+    // Assert the inline error still exists.

Testing that 'all the rest' still works after disabling the summary seems legit, but now we are testing multiple things in one test method. We should split this, and can't we just set the disabled property and call a different test method that does this already (or is that evil)?

My apologisch for the long wait, keep up the good work.

(duplicate)

I am not very experienced with PHPUnit so I have left that alone for now. Here is what I accomplished with this patch :

#52 : 2, 3, 4, 6
I have changed the wording of these comments based on suggestions.

#54 : 1
I have inverted one of the if statements to allow for use to just continue early, this reduced the nesting used. I'd just like to add, complexity does not mean you need comments, it means you need less complexity. A helper function would not be suitable here. The continue statements clearly show a path with minimal branching.

#54 : 4.2, 4.3
I've renamed the test and cleaned up the comments.

Perhaps later I will see if I can do something with changing the actual tests. For now, I invite anyone to pick up that bit as they are not my strong suit.
Though I have not addressed the test concerns, I am changing this to needs review as there have been changes.

I think the API documentation (and change record) should clearly mention accessibility and usability implications. Some cautionary advice, similar to the "accessibility considerations" section now included in many MDN articles.

I'm not against this API feature, but it isn't something that should be used frequently or casually. I'll have a think about how to phrase this.

The main risk I want to highlight when disabling the summary message, is that the first mention of an error can occur outside the browser viewport just after page load (a.k.a. "below the fold"). In this case, users may be unaware that there are any errors at all, and mistakenly think their form submission was successful.

Any user could run into this situation; it isn't just about users with disabilities. However some users and situations can be more seriously impacted:

1. On longer forms (more than 3 elements, say).
2. On small screens (phones).
3. When using browser zoom to increase text size.
4. When using screen magnifiers and/or screen readers, it's preferable to mention errors near the start of the page/form. Don't make me inspect the whole form again to confirm there were no errors.

EDIT (14th June 2020): This issue is still lacks good use cases, IMO.

I'm not convinced that "the form uses AJAX" amounts to a use case here, because it doesn't say anything about the context or purpose of the form. The form submission mechanism doesn't have anything to do with the need for error reporting.

So far we just have @nikunjkotecha's "Newsletter signup block in the footer" - but what's the significant feature there? Is it the small number of fields, the location, or something else?

Almost all forms with few (or a single) elements could benefit and are thus use cases. Having a login-form with multiple red alerts isn't exactly visually pleasing.

I would generally argue, that good use cases can't be provided beforehand in all cases and that more options tend to be the better way for an open-minded framework (like drupal).

+++ b/core/modules/inline_form_errors/src/FormErrorHandler.php
@@ -93,11 +93,23 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
+        continue;
       }
-      elseif ($is_visible_element && $has_title && $has_id) {
+
+      // If an element does not have a title, ID, or is invisible use the
+      // default error display.
+      if (!$is_visible_element || !$has_title || !$has_id) {
+        continue;
+      }

I know this is supposed to be about reducing complexity, but really? Replacing one line with 4 new code lines (and 2 comment lines), it is improving the code? Also, please don't use continue if you can avoid it. It's easy to not use it. Just do the new code inside the existing elseif block.

The patch #57 was no longer applied so added a re-rolled patch with above mentioned changes by #60.

Added another patch hope it works.

+1 to adding this. I am already using a version of this patch on a site; my use case is that the UX/design includes inline form errors across several forms but no summary, so to satisfy the design requirements I had to remove the summary. Doing this without this patch is non trivial as IFE already alters the form messages to move the errors inline and add the summary; subsequently removing the summary again is even more complicated.

However, is it worth negating the option here? We don't have many "disable" options in Drupal; instead should we name the flag "#inline_form_errors_summary" and default it to TRUE, and then allow FALSE to disable it?

Changing this to be a default true for enabling the summary is good to me. I was going to mention the current property name was too long. I don't know if #inline_form_errors_summary is the right name yet. It feels too verbose without actually telling us much.

I know this is within a module so it should have that namespace. Ideally I see this as provide_error_summary.

As an alternative to providing this option which would also satisfy my use case: how about making the summary a Twig template?

You can either just blank out the template entirely if you don't need summaries, or provide a shorter custom message at the top of the form such as "This form has errors, please correct them below."

We could also provide template suggestions based on form ID if that level of flexibility is needed?

I suppose this suggestion relates to #2980380: Inline error messages are not using proper theming mechanisms as well.

We discussed this issue at the #3168518: Drupal Usability Meeting 2020-09-08 and a theme hook seems like a great suggestion as it would provide much better control over the summary contents.

@longwave I won't be updating the IS just yet, since others have put in a lot of work in the current patch, but I hope to see more support for improved inline_form_errors theming!

The previous patch switched $is_visible_element && $has_title && $has_id to $is_visible_element || $has_title || $has_id which caused issues with elements that only meet one of these requirements. It also did not respect #error_no_message.

New patch should sort this and still checks for #disable_inline_form_errors_summary :)

Tests for recent patch worked so marking as need review.

New patch which will hide error messages in the summary for fields even if they don't have a title or ID.

Meant to include ID actually.

#57 works for me on 8.9.13. Thanks so much!

This may the solution for failed patch #75 , please review.

Patch #81 worked for me on 9.2.3. Thanks!

Patch #81 is working with Drupal 9.2.4

Fixed the fail tests for 9.3.x, Please have a look.

Confirmed #85 works for me in 9.2.9.

Hello,
Thank for this solution.
The patch #85 works for me in 9.3.9 and 9.4.0-dev

Just a reroll for 9.5

I think that can tie to 1.4.10.

During the A11y office hours, we added an AI generated summary to the description. This still needs to be validated by a human, but we are hoping this might help us make it easier to address issues.

Patch 92 worked in both 9.5 and 10.1.
I'm moving it to RTBC.

About AI generated summary, I reviewed it and looks pretty similar to initial summary.
It looks fine for me.

As far as the API goes here I would like to repeat my comment from #67

We don't have many "disable" options in Drupal; instead should we name the flag #inline_form_errors_summary and default it to TRUE, and then allow FALSE to disable it?

Having said that we also have #error_no_message three lines above, so should we be consistent with that somehow?

I agree with @longwave. We discussed how that would be implemented. I think we need to implement hook_element_info_alter and add #inline_form_errors_summary there with a default of TRUE. I think the implementation would be:

function inline_form_errors_element_info_alter(&$info) {
  $info['form']['#inline_form_errors_summary'] = TRUE;
}

Also I'm really confused about why and why you'd use the setting here #disable_inline_form_errors_summary vs #disable_inline_form_errors vs #error_no_message. There's way too many double negatives here and also with the current implementation #disable_inline_form_errors_summary has to be set on the form level - but the comments and issue summary here indicate that there are still situations when it will be shown even if this is set...

We need to add docs of these three variables and where and how they can be used. And perhaps when we write better docs a good name for the new thing will be more obvious.

Patch #100 goes with followed suggestion: use key #inline_form_errors_summary

Note: we also have #disable_inline_form_errors, so consistency about naming here is really broken.
My honest suggestion is that we have this issue solved since this feature is really overtime and open a task to discuss only consistency of naming for those keys.

+++ b/core/modules/inline_form_errors/tests/src/Unit/FormErrorHandlerTest.php
@@ -177,7 +177,7 @@ public function testErrorMessagesNotInline() {
-    $this->testForm['#disable_inline_form_errors'] = TRUE;
+    $this->testForm['#inline_form_errors_summary'] = FALSE;

Not sure why this is changing - looks like we'd be losing coverage.

Good cache alexpott. This was mistakenly changed.

Adding patch 103 without this change.

Path #103 disables summary errors on all forms.
&& $form['#inline_form_errors_summary']
should be
&& !$form['#inline_form_errors_summary']

Tried to address the pointer in #105.
Please review.

Fixed the fail tests for 10.1.x, Please have a look.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.