Set an error on a "container" element (the root of a widget, for example), and it won't be shown inline. Meanwhile, fieldsets and details work fine.

This issue was found while working on #2512306: Inline errors not shown for details elements.

Before: no error shown.

After:

Comments

bojanz created an issue. See original summary.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

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 %}

{{ errors }}

{% endif %}

If so let's consider adding that to core?

xjm’s picture

Component: render system » forms system
Priority: Major » Normal
Parent issue: » #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)

@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.

SKAUGHT’s picture

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’s picture

Category: Bug report » Feature request
tim.plunkett’s picture

Category: Feature request » Bug report

@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.

SKAUGHT’s picture

my apologies for overstepping.

SKAUGHT’s picture

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.

SKAUGHT’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 10: 2560467_10-Inline-errors-not-shown-for-container-TEST.patch, failed testing.

SKAUGHT’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 12: 2560467_12-errors-not-shown-for-container.-adjust-re.patch, failed testing.

The last submitted patch, 12: 2560467_12-errors-not-shown-for-container.-adjust-re.patch, failed testing.

SKAUGHT’s picture

Status: Needs review » Needs work

The last submitted patch, 15: 2560467-15-Inline-errors-not-shown-for-containers.patch, failed testing.

SKAUGHT’s picture

Status: Needs work » Needs review
FileSize
71.84 KB

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

dmsmidt’s picture

Issue tags: +DevDaysMilan, +ux, +user experience

Status: Needs review » Needs work

The last submitted patch, 15: 2560467-15-Inline-errors-not-shown-for-containers.patch, failed testing.

mgifford’s picture

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.

SKAUGHT’s picture

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

SKAUGHT’s picture

SKAUGHT’s picture

Status: Needs work » Needs review

running tests

Status: Needs review » Needs work

The last submitted patch, 22: 2560467-22-Inline-errors-not-shown-for-container.patch, failed testing.

mgifford’s picture

Issue tags: +accessibility

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

mgifford’s picture

Issue tags: +Needs tests
SKAUGHT’s picture

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.

The last submitted patch, 22: 2560467-22-Inline-errors-not-shown-for-container.patch, failed testing.

mgifford’s picture

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

mgifford’s picture

Issue tags: +govcon2016

Status: Needs review » Needs work

The last submitted patch, 29: 2560467-28-Inline-errors-not-shown-for-container.patch, failed testing.

DamienMcKenna’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
8.73 KB

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

Status: Needs review » Needs work

The last submitted patch, 32: drupal-n2560467-32.patch, failed testing.

DamienMcKenna’s picture

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?

joelpittet’s picture

@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

The last submitted patch, 32: drupal-n2560467-32.patch, failed testing.

SKAUGHT’s picture

Status: Needs work » Needs review
FileSize
9.15 KB

testing

Status: Needs review » Needs work

The last submitted patch, 37: 2560467-37-test1.patch, failed testing.

SKAUGHT’s picture

#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.

SKAUGHT’s picture

FileSize
1.53 KB

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mgifford’s picture

#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

alexpott’s picture

Status: Needs review » Needs work
  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.

dmsmidt’s picture

Title: Inline errors not shown for container elements » Inline Errors not shown for container elements
Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#2509268: Inline errors repeated on child elements in module uninstall form
FileSize
9.19 KB
5.79 KB
6.48 KB

@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.

dmsmidt’s picture

The last submitted patch, 45: 2560467-45-inline_errors_container.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 46: 2560467-46-inline_errors_container.patch, failed testing.

dmsmidt’s picture

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)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.