Inline errors repeated on child elements in module uninstall form

The attached patch should solves the issue. It adds the error to a non existing element rather then to a real element (as it is done in various forms in core)

This is the result

This issue is normal priority.

Looks like we should test this to make sure we don't regress again.

@TR: fair enough! - good info on why, thanks

Here is the patch with testcase. Not sure with the location for this (with test case name ): ).

Here is the test only patch.

Also same is the case module install page. It doesn't show up any message if we just hit submit without selecting anything.

extra space Oopz :)

Setting an error on a parent element should not repeat the same error on all child elements.
I've just encountered this bug on a field widget, it will keep turning up, so we can't just add a workaround.

Retitling. The problem is coming from FormErrorHandler::setElementErrorsFromFormState() which calls $form_state->getError(), which returns errors for parent elements as well.

The #errors key was previously used to indicate "do I or one of my parents have an error", to add an error class when needed, but now it's also used to display the error message, hence the problem.

This can be reproduced on any element that has #tree => TRUE.

Okay, worked on this a bit.

Requires a UI change, to make $form['uninstall'] a real form element so that the error can be attached to it.
Likely needs a better label than "Modules that can be uninstalled".

No interdiff because it's a new approach.

All of those fails are from constraints, like \Drupal\aggregator\Plugin\Validation\Constraint\FeedUrlConstraint.

It's called from \Drupal\Core\Field\WidgetBase::flagErrors(), using \Drupal\Core\Field\WidgetBase::errorElement() to determine the element to set the error on.
It is setting it with a #parents of ['url'] when it should be ['url', 0, 'value'].
However, in these cases $delta is '' instead of 0, because $violation->getPropertyPath() is empty.

Seems we're blocked on #2027059: Improve the documentation of WidgetBase::errorElement() for mapping violation property paths to form elements :(

Straight reroll.

Coming from #2027059: Improve the documentation of WidgetBase::errorElement() for mapping violation property paths to form elements - discussed with @tim.plunkett in Barcelona.

I agree that the current behavior is a massive drag on things like checkboxes :-/

We do however have a lot of field widgets that rely on the current behavior when it comes to the task of "mapping a field constraint violation down to an actual form element" (i.e WidgetInterface::errorElement($violation)).

The default implementation in WidgetBase, which is good enough for most widgets, is "I'll flag the widget wrapper element as a whole, and the error will be displayed on every children (in most cases there's only one)". More complex widgets can, if they need, decide to do more fine-grained flagging if they have several child input elements.

That "catch all" default behavior :
- is the only default implementation that makes sense generically,
- is also what you want for violations that really are on the field itself (like a NotNull constraint for required fields, or the FeedUrl constraint for the feed 'url' field that seems to be causing the fails mentioned in #19) rather than on some specific property within the field.

So I'm a bit worried at how much we'd break by changing the "error displaying" behavior ?

No longer major as the code was moved out of the forms system and into the experimental module.

please refer to: https://www.drupal.org/node/2729287#comment-11209457

THESE ARE NOT TRUE CHILD ELEMENTS, they are 'checkbox' not 'checkboxes'

this is a clean patch aside from others.

this removes the initial validate check completely then lets the confirm form set the user message and redirected back to fresh form.

to note:
this visual error sequence is due to #tree('d) form elements, if the tree (was say) be an textfield, an email field, a select. you probably still wouldn't check against 'all items' in the tree but each uniquely. i think it's as much the case that these type of edge-case validation needs would be better handled by the developer of That From, not the From System trying to automatically handle everything in the tree.

some kind of test-bot oddness. changing status.

i have just noticed [#21]

if this method is considered acceptable, i will happily look at testing module to address that form situation.
@tim.plunkett

@dmsmidt
please review comment [#31]. I do believe that this issue title/description to be miss leading in that it is expectable that if you #tree a bunch of FAPI items then call the error to that group, it should trip any appropriate errors for those children.

IE: if this was a Field Collection (acting like a #tree) and you called an error to the group and you have an image field, a select.. a bool checkbox. they would (and should) all have individual errors that would be rendered.

comment: [#32]. this patch lets the confirm_form complete and deliver a single error as would be expected at this is a confirm_form that got a validation tacked on that basically isn't needed

I can't repeat the problem described in the summary at /admin/modules/uninstall

With or without the patch it doesn't give me that error.

Is there a better example where checkboxes produce errors as child elements? I understand that the one in the summary is a series of type="checkbox"

My bad.. Sorry @TR. Without the patch it still looks like it does in the issue summary. With the patch it now looks much better and looks like:

where checkboxes produce errors as child elements?

This is screenshot only validates child checkbox elements:
<input data-drupal-selector="edit-uninstall-automated-cron" id="edit-uninstall-automated-cron" name="uninstall[automated_cron]" value="1" class="form-checkbox error" aria-invalid="true" type="checkbox">

This issue seems to swing between finding a structural solution and only fixing the particular instance of the problem in the uninstall form. I think the current scope of the issue needs to be clarified.

Here is a run down of the different points that were raised in the comments:

1. There are reports that this problem occurs on more than just the uninstall form. In fact @bojanz mentions in #2509268-16: Inline errors repeated on child elements in module uninstall form that this happens on every form that sets '#tree' => TRUE. @TR gives another real life example in core in #2509268-21: Inline errors repeated on child elements in module uninstall form: it happens when submitting an empty form at /admin/config/development/testing.
2. A structural solution was investigated by @tim.plunkett in #2509268-17: Inline errors repeated on child elements in module uninstall form.
3. This raised concerns and it was discussed at Drupalcon Barcelona and in issue #2729287: [policy, no patch] Decide if and how to remove experimental modules from core. From what I can gather it was decided that a structural solution was either unneeded or impossible:
   • @yched in #2509268-25: Inline errors repeated on child elements in module uninstall form argues that the default behaviour "makes sense" and is worried about "how much we'd break by changing the "error displaying" behavior". There are ways for individual form elements to work around this by deciding "to do more fine-grained flagging if they have several child input elements". This seems to indicate that @yched prefers to solve this on a case-by-case basis. Based on this comment I also think that the '#tree' => TRUE case is probably solvable by flagging the correct form element / field widget.
   • @SKAUGHT argues in #2729287-4: [policy, no patch] Decide if and how to remove experimental modules from core that the problem is due to "a poor validation check on mutliple 'checkbox' elements" where as "if they were checkboxes it would result in a clean single error". This seems to indicate that he prefers a case-by-case solution.

So it seems that a case-by-case fix is preferred. We need to update the issue title and summary.

The patch from #32 doesn't seem like the right solution. It works around the problem by removing the validate handler. Then the confirmation form will handle it, set the error, and redirect back to the uninstall form which will display it. I would prefer keeping the validate handler.

I think the patch from #1 is on the right path. It relies on a setting an empty name which is not a documented feature but according to @TR in #2509268-6: Inline errors repeated on child elements in module uninstall form it is used at least in 13 other places in core already.

I would propose to continue on that path since the solution is simple and feels right: we do not want to flag a specific element because this error does not apply to any element in the form.

We should do something about this undocumented feature though, either document it, or provide an official way to set a generic error on the form without flagging a specific element. I made a followup for this: #2780209: Core relies on undocumented feature of FormStateInterface::setErrorByName().

I just saw that the case of the duplicate inline errors in the Simpletest form is due to the generic Table form element flagging the root table element when no items are selected in Drupal\Core\Render\Element::validateTable().

There can be multiple tables in a single form, so how are we going to deal with this?

How about allowing to flag elements that should not display their errors inline? This fixes both the uninstall form and the generic Table element which is used by the simpletest form.

@pfrenssen thank you for making such a nice summary in #42.

Concerning the proposed approach in #46, two thoughts:

1. It looks like a very minor change, but in my experience there are already so many options in the FAPI. Shouldn't we try to avoid this a bit?
   Maybe we can just mark the checkboxes as erroneous without a message? And then use the new method proposed in #2780209: Core relies on undocumented feature of FormStateInterface::setErrorByName() to show one message for the complete form?
2. However, if we would add the 'error_no_inline_message' property we certainly have a lot of controle for more advanced use cases, certainly in combination with what I proposed here: #2754977: Enhance formErrorHandler to include children errors on RenderElements.

The option to mark the entire form works fine for the uninstall form which is a simple form that is fully under our control. Unfortunately it wouldn't work for the Table element. That may be used in any kind of form in contrib or custom code, and it might be critical that the exact element has been marked with an error.

I think with this #error_no_inline_message controlling which errors show up inline becomes really easy.

What I don't like about it is that both the parent container and the child elements need to get this flag. But I think it is maybe unavoidable.

Wanting to highlight @xjm's point about timelines for this:
https://www.drupal.org/node/2504847#comment-11750733

@pfrenssen & @dmsmidt how do we move this forward?

Works.
I have tested the above and can confirm from my own findings that this patch works.
Thanks for the patch!

@sean conner, thanks for testing.

@prenssen, ok let's go for it, in complex forms it can be a good tool to have.

We should create a test for that new functionality and see if we can work around the problem of the need of adding #error_no_inline_message twice.
Maybe we should create some extra logic on form elements to prevent showing child errors, if all children with errors have #error_no_inline_message. Thinking of a combination with #2754977: Enhance formErrorHandler to include children errors on RenderElements.

1. I'm not 100% convinced about the approach but I'm not sure about a better one... maybe a form maintainer has a better idea?
2. If we do go for this approach we certainly need more automated testing of the changes made in the patch
3. We need to document the new # key #error_no_inline_message somewhere and how to use it.

4. +++ b/core/modules/system/src/Form/ModulesUninstallForm.php
   @@ -131,6 +136,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
            '#type' => 'checkbox',
   ...
   +        '#error_no_inline_message' => TRUE,
   

   Isn't it quite likely that most times a checkbox type is used you're going to want to use #error_no_inline_message.

First of all, the patch in #46 disables inline errors for all Table elements. Potentially we can have multiple tables and the table is not always at the top of the page. So Tables should support Inline Form Errors, otherwise accessibility is as bad as it was without IFE enabled.

Also, since not everybody is convinced, I tried to rethink the problem at hand and find a minimum impact solution.
To prevent errors from showing up, we already have the property '#error_no_message'. This already gives us the possibility to hide messages on $elements.

These two things combined allows setting errors on a complete table, allows setting errors on particular fields in a table (row). And also prevents adding the proposed '#error_no_inline_message' property multiple times.

I'm working on a patch to see if this idea works.

Note: I also had the idea to add a property, or an argument to the setError() and setErrorByName() methods, to prevent bubbling up of error messages.

Alright this is what you get with the approach mentioned in my previous comment for the uninstall page.
Now there are real inline form errors. Patch attached (no interdiff, new approach).

I think this can work in general for tables.
But, since there is only one element that can have an error in the uninstall page case, I think in this specific case IFE is overkill.

So we can get back to the patch of #14. This is a simple change, and doesn't need API changes.
In a follow up, we can work on IFE for tables with the approach shown above.

Re-roll of #14.

@left and I tested this one. As per patch #56
Behaviour of patch visual test - No modules selected heading as per patch intent

Alert using NDVA "no modules selected" - as per intent. Error message and link from link from screen shot above does not display as per comment.

I confirm that I can replicate the issue when Inline Errors module is enabled and I click the 'uninstall' button without any modules selected. And that patch #56 works as intended. Tested with ChromeVox and the vocal error message seems correct.

Before

After

@left gets bonus credit for figuring out how to embed screenshots in a comment... :D

@dmsmidt I think you have addressed all of @alexpott concerns in #53. If so we should mark this RTBC.

Thanks @left & @ok_lyndsey!

Manually tested during sprint weekend. Looking good. A11y maintainer says looks good. Let's do it! RTBC!

So reading the entire issue the question that occurs is how do we stop similar things from occurring in contrib. The current approach fixes the problem in core and I've looked for other places where this might occur but I can't spot any. I think a followup is in order that should explore a generic fix so that sites that use inline_form_errors don't hit this when developing custom modules or using contrib. The followup should exist before commit.

@alexpott

Actually we identified a lot in this issue, and there are multiple follow ups.
I created two new ones. Ready for commit!

Followups:
#2848319: Find a way to make it easier to hide Inline Form Error messages on child elements
#2848307: Inline errors not working on form table elements
#2780209: Core relies on undocumented feature of FormStateInterface::setErrorByName()

Committed and pushed 43f6069 to 8.4.x and 4a430c8 to 8.3.x and 7e5875c to 8.2.x. Thanks!

Merged all the way back to 8.2.x since the fix is BC compatible and there is no point the uninstall form looking awful when IFE is installed on 8.2.x.