Inline Errors not shown for container elements

I find it weird that container is acting like a form element (legacy stuff), in that it gets generated an ID. But given that it does, I see the point that it probably should also be able to output the errors as well.

If you set errors on it, how about adding this to the template?
{% if errors %}

{% endif %}

If so let's consider adding that to core?

@alexpott, @Cottser, @joelpittet, @lauriii, and I discussed this issue at DrupalCon New Orleans and agreed that it is just a normal bug with the form system because details elements are already fixed and the usecase is more limited. However, it is also a major issue for the Inline Form Errors experimental module (among many others), so adding it to that meta.

I'm not sure why anyone would set an error against the 'container' element. Of course, the nature of this element is to contain actual form items -- for which would set a validation error of off. it seems like this is an odd request for a container todo.

this should be a feature request.

@SKAUGHT, I appreciate your enthusiasm for the IFE module and related issues, but this is still a bug and falsely downgrading or miscategorizing issues won't make that module more stable.

my apologies for overstepping.

This patch does yet not include any specific test yet.

as a manual test base, i have a small fork of dmsmidt/errorstyle on github: https://github.com/skaught-/errorstyle the container is the last in the form.

I've rework the test from the #2512306: Inline errors not shown for details elements and touched up it's naming a little to model this test from.

this should handle the RenderElementTypesTest test fail. the twig change adds in \n so it just needs to be added to the test.

the View StyleMappingTest test -- i don't at all understand this fail at all.
the Migrate tests -- i've seen this fail often on other tickets... usually passes after 'adding test' so i'll let this run then see.

i noticed that patch was too big..accidentally included a interdiff. removed.

i'm having this StyleMappingTest error occur while running the test BEFORE applying this patch. I'm marking this for review. i can not backtrace this test well enough to recreate it in my local dev to see what it's actually suppose to be doing.

i've run it with php 5.5, 5.6 & 7 (in mamp3.5 containing phpunit 4.7.6) under both 8.1.x and 8.2.x branches

Should the patch work in 8.1.x or 8.2.x because it's failing in the latter. I'm not sure where we should be targeting this.

i am hoping to get back to see if its just a CI glitch. i had just retested under 8.2 a few days ago when this fail occurred

cherry-picked and rebuilt patch.

running tests

I'm tagging this for accessibility as it's required to get inline form errors as a fully fledged part of Core.

patch does contain its own test. Such can be seen in #10 (first fail of 'testContainer'). initial test was corrected.

Secondarily, it's a Views test failing. I have no insight for that fail.

RenderElementTypesTest.php moved and #22 doesn't apply any more.

I added an extra assertIdentical() line to mappedOutputHelper() to see why the two values are not identical.

Here's the output of #32:

"<em class="placeholder">title_field:John</em>" was expected, "<em class="placeholder">
title_field:John
</em>" was the actual value.

"<em class="placeholder">title_field:John</em>" was expected, "<em class="placeholder">
title_field:John
</em>" was the actual value.

So it seems like the problem is that extra whitespace is being added around the string?

@DamienMcKenna you can remove the whitespace with whitespace modifiers, spaceless, or adding the whitespace to the test if you want it to persist.

http://twig.sensiolabs.org/doc/templates.html#whitespace-control

testing

#39 removes all line returns from the style mapping test strings. As the test should only be checking for the string values i think this is the best general work around.

adding diff. sorry bad form.
as 29 and 32 where just test stuff I had skipped them.. nothing lost.

#39 still applies. I'm not sure what is the quickest way to verify that this patch works though. Thanks @SKAUGHT for applying the patch & interdiff. We just need to do more testing to see we can resolve it as RTBC.

If we can more on this issue, we might be able to keep this module in Core:
https://www.drupal.org/node/2504847#comment-11750733

1. +++ b/core/modules/system/templates/container.html.twig
   @@ -27,4 +28,11 @@
   -<div{{ attributes.addClass(classes) }}>{{ children }}</div>
   +<div{{ attributes.addClass(classes) }}>
   +{% if errors %}
   +  <div>
   +    {{ errors }}
   +  </div>
   +{% endif %}
   +{{ children }}
   +</div>
   
   +++ b/core/themes/classy/templates/form/container.html.twig
   @@ -25,4 +26,11 @@
   -<div{{ attributes.addClass(classes) }}>{{ children }}</div>
   +<div{{ attributes.addClass(classes) }}>
   +{% if errors %}
   +  <div class="form-item--error-message">
   +    <strong>{{ errors }}</strong>
   +  </div>
   +{% endif %}
   +{{ children }}
   +</div>
   

   We should avoid the new new lines being added. I'm not sure how this template change would play out with stable since I think stable needs the errors too which makes this change awkward to say the least. I wonder if we should prepend the errors to the children render array and then we wouldn't have to change the template. It'd be good the get the opinion of a theme maintainer.

2. +++ b/core/modules/views/tests/src/Kernel/Plugin/StyleMappingTest.php
   @@ -64,11 +64,15 @@ protected function mappedOutputHelper($view) {
   +        /**
   +         * The expected result is the mapping name and the field value,
   +         * separated by ':'.
   +         *
   +         * #2560467: Inline Errors not shown for container elements Fix for unknown line returns being added in.
   +         */
   

   This comment style is not permitted for inline comments.

3. +++ b/core/tests/Drupal/KernelTests/Core/Render/Element/RenderElementTypesTest.php
   @@ -56,7 +56,7 @@ function testContainer() {
   -    ), "<div>foo</div>\n", "#type 'container' with no HTML attributes");
   +    ), "<div>\nfoo\n</div>\n", "#type 'container' with no HTML attributes");
   
   @@ -65,7 +65,7 @@ function testContainer() {
   -    ), '<div class="bar">foo</div>' . "\n", "#type 'container' with a class HTML attribute");
   +    ), '<div class="bar">' . "\n" . 'foo' . "\n" . '</div>' . "\n", "#type 'container' with a class HTML attribute");
   
   @@ -73,7 +73,7 @@ function testContainer() {
   -    ), "<div>foo</div>\n", "#type 'container' with child elements");
   +    ), "<div>\nfoo\n</div>\n", "#type 'container' with child elements");
   

   We should try to ensure this type of change does not occur - there are ways of making the twig template not add the new lines.

@alexpott, thank you for reviewing this and @SKAUGHT for creating the patches.

1., 2. and 3. The newlines added to RenderElementTypesTest are no concern of this issue. So I removed everything related from the patch.
Edit: this seems to be a bit rash from me, I missed the actual test problems mentioned earlier, my bad. But still we need a bit cleaner approach as per #44.

That leaves the question about the theme in 1. All changes proposed to container twig files are in line with how displaying errors are solved for details elements, so this should be alright. Since errors on container elements hardly ever happen it is also unlikely people will fall over it. I also updated the twig file for the stable theme, that was not included in the previous patch.

And did some renaming and cleaning up for to confirm with our code styling rules.

P.S. I merged the added form element into the testmodule here: https://github.com/dmsmidt/errorstyle. And added a checkbox to trigger the error.
P.S. 2, updated the issue summary to be more clear and show an after picture.

Woops, forgot some documentation.

I've searched for the usage of setErrorByName() and setError().
Nowhere is it used on $elements of type 'container'.

There are some possible cases that errors will not be printed correctly.

Via setErrorByName() (no $element type set):
File -> $parents

• FileTransferAuthorizeForm.php: 'connection_settings'
• ImageEffectFormBase.php - ScaleImageEffect.php: 'data'

Via setError():

• Drupal\Core\Entity\Entity::validateFormValues (set on entire $form).
• Drupal\views\Plugin\EntityReferenceSelection::settingsFormValidate (no element type set, no title set)

Tagging https://www.w3.org/WAI/WCAG21/Understanding/error-identification.html