Details form element should open when there are errors on child elements

I confirm this bug, more info:

- If you delete both date and hour information and save: no problem.
- If you keep the date part but empty the second part (hours, minutes seconds) textfield and save, then you have the error message 'The ... date is invalid. Please enter a date in the format 2014-09-29 13:54:49. '
-> so on save, if second textfield is blank it should fill in hour:minutes:seconds of time of saving.

Really, I think that all fieldset or hidden component with errors inside should be shown outspreaded. I'm going to take a look, I'm not sure if I can do this task, but I think that is very nice appreciation.

Thank for the report.

I don't find the correct place for putting the "open" attribute to details tag. I suppose that you can add it in the validate function or in the detail render if some of the rendered element have error... but I tried to find it and it was imposible for me. I will follow this issue and learn if any resolve it.

Patch attached, working as described in comment #2.

I'll review this.

Did manual review and the patch works as intended. I updated the issue summary with some screenshots.

Waiting for testbot results before marking as RTBC.

Found an error in the patch - new patch. Error occured when patch is applied and user tries to edit an content type.

I tested and it's run ok. I think that can be maked as RTBC.

Updated title, to be more specific.

This issue is also a problem for #1493324: Inline form errors for accessibility and UX.

So this issue is actually much worse with #1493324: Inline form errors for accessibility and UX. As I mentioned in #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE):

...Previously, at least the validation error message was visible so the user would hopefully figure out what was going on. Now the informative validation message is actually hidden, and furthermore it now comes with a link that seems not to work at all. I can imagine this bug being extremely frustrating for users and making it seem like Drupal is just broken.

Promoting to major. I considered whether it should be critical, but it is still possible to use the form. It's just difficult.

There's another related problem here, if you set an error on an element that has children, the children will (incorrectly, #2509268: Inline errors repeated on child elements in module uninstall form) show the errors, but the parent element (for which the error was actually set) won't. If you change the #type to 'fieldset', the error starts showing up.

EDIT: The same problem persists if type is "container". Opened #2512306: Inline errors not shown for details elements.

I don't really see how this was not a problem before. The only thing lacking here is an error style on these detail state right? I am unsure why that would be hard to solve.

Looks like I opened a duplicate of this here.

avoid some errors.

I wonder how this should work for #18. You cannot show multiple vertical tabs and this may require some JS code, too.

@hass... Re #18, I don't think there is a need to show multiple v tabs, just to indicate that there is an error by making the pages v tab (in that example) red to indicate that an error can be found in that v tab

The reroll is done, so IMHO the best is removing the tag.

There may be a better way to go about this, but for now just avoid more errors.

It's cool that the patch passes tests now.

+++ b/core/lib/Drupal/Core/Render/Element/Details.php
@@ -84,6 +84,26 @@ public static function preRenderDetails($element) {
+      $parents = implode('][', $element['#parents']);

It just seems weird to do this. Isn't there some other way to access the widgets?

Testing on simplytest shows that the patch works for Seven theme. I put in a path alias without a slash and changed the author to a username that doesn’t exist in the system. Even if you then manually close those sections before hitting save they show expanded with the error messages on top:

It does not work in Bartik with the vertical tabs, there is no indication that something is not right inside a given tab.
Similarly, it doesn't work in Stark but that may be by design?

So it works with detail sections but not with vertical tabs, are vertical tabs in scope for this patch?

Well, with vertical tabs, only one can be open. Do you think you can adjust the patch so that it marks the the last tab with an error to be "selected"?
I tried the below, but it didn't work

<?php
@@ -96,6 +96,7 @@ public static function preRenderDetails($element) {
               foreach (Element::children($child['widget']) as $widget) {
                 if (isset($child['widget'][$widget]['value']) && $child['widget'][$widget]['value']['#errors'] != NULL) {
                   $element['#attributes']['open'] = 'open';
+                  $element['#default_tab'] = $parents;
                 }
               }
             }
?>

In this same file, I noticed that we need to do this:

- * @see \Drupal]Core\Render\Element\VerticalTabs
+ * @see \Drupal\Core\Render\Element\VerticalTabs

For vertical tabs I like the suggestion in #18. If we decide to tackle these in here that's fine, maybe needs an updated issue title then.

I don't code, so no updated patches from me :)

Hi,
Patch works fine for Me.

Still needs tests.

Just getting tests started. Probably is a better place to put them?

will try to look at this

Trying a simple reroll to get things going again.

So after some testing, it doesn't seem to be working.

code never reaches

     $element['#attributes']['open'] = 'open';

in

foreach (Element::children($child['widget']) as $widget) {
  if (isset($child['widget'][$widget]['value']) && $child['widget'][$widget]['value']['#errors'] != NULL) {
    $element['#attributes']['open'] = 'open';
  }
}

For some widgets, at least for path alias, the
If statement array structure needs to be:

<?php
$child['widget'][$widget]['#errors']
?>

not

<?php
$child['widget'][$widget]['value']
?>

So I've spent a few hours debugging this patch the other day. I don't know how to fix it just yet but maybe that can give someone a clue.

Here is what I found:

1. I was testing with entering a node author that does not exist
2. The detail would not open
3. It seems that even though the author has an error, the ['#errors'] would always be NULL
4. Tracing it back it seems that the element with the error is uid][0][target_id but it has no children and/or an easy way to target it in this loop?

@ptisimard, this might work. Although, there might be a better way for us to know which array structure to check for errors that I am not aware of.

+++ b/core/lib/Drupal/Core/Render/Element/Details.php
@@ -89,7 +89,9 @@ public static function preRenderDetails($element) {
+                if ((isset($child['widget'][$widget]['value']) && $child['widget'][$widget]['value']['#errors'] != NULL) ||
+                  (isset($child['widget'][$widget]) && $child['widget'][$widget]['#errors'] != NULL) ||
+                  (isset($child['widget'][$widget]['target_id']) && $child['widget'][$widget]['target_id']['#errors'] != NULL)) {

See how this if statement is checking 3 different parts of the array structure for errors. Why aren't the errors in a consistent part of the array. Furthermore, is there a way to ask the widget where the errors are stored or is this an ok way of testing for errors?

Also, maybe the isset() != can be simplified to empty()

+++ b/core/modules/node/src/Tests/NodeEditFormTest.php
@@ -113,6 +113,13 @@ public function testNodeEdit() {
+    $edit['created[0][value][time]'] = NULL;

The current failure: Need to first login as admin user.

Thank you @lokapujya, your work has greatly improved my insight in the underlying problem.

See how this if statement is checking 3 different parts of the array structure for errors. Why aren't the errors in a consistent part of the array. Furthermore, is there a way to ask the widget where the errors are stored or is this an ok way of testing for errors?

No there is currently no standardized way of getting to know if children have errors. So to be able to solve this issue cleanly I opened another issue, that fixes this: #2754977: Enhance formErrorHandler to include children errors on RenderElements.

If the above gets in we can easily fix this issue with the following patch. This is a new approach, so I didn't make an interdiff.
I'll look at a test later.

Setting this back to 8.2.x because this is blocked by a data model change.

Please test existing tests CI.

Test combined with new version of #2754977: Enhance formErrorHandler to include children errors on RenderElements.

*Crazy* me, I combined the old version, hopefully this one is oke.

Found out (together with @Getekid) that Firefox has some own magic to open the details if it found errors (even without the patch).
Chrome however for example isn't that smart.

Is it worth opening an issue in Chrome to highlight the problem? We should test this in IE & Safari.

More detailed research for: which browser opens the details element automatically and which do not when there is an error on a child field. (note: without patch).

Does:

• IE 11
• Edge 13
• Firefox 48

Does not:

• Opera 38
• Safari 9.1
• Chrome 51
• Yandex 14.12

So this patch is still relevant!

So do you need help to fix the failing test? Thanks again for all your work on this @dmsmidt

@mgifford, thanks for the offer, but @Getekid was writing and fixing tests for this issue (I was mentoring him about this at the drupaldevdays in Milan). I've asked him to upload it. You could however help by giving a reply / review and writing a test for #2754977: Enhance formErrorHandler to include children errors on RenderElements, that blocks this one.

All,

when an element within a detail element is a required field, but no value is supplied and the detail element is not open, i see that the application doesnt respond. Can any of you confirm if this issue addresses that problem?

Thanks
Sukanya

Hello,

Here is the test I wrote during DevDays with the help of @dmsmidt (first contribution/test)!!

In order to call the assertRaw method I had to put the whole "details" tag which might not be the best choice as another module might change it, but this way you can make sure the 'open="open"' is applied to the right point.

Hope it helps

@sukanya.ramakrishnan

yes, a 'required' field alone does not open the details element.

tested using errorstyle.module (github) and commenting out the setErrorByName for 'test_child_custom_error_2'

Hi SKAUGHT,

Thanks for the reply. Wondering if this patch will fix that issue. basically the validation is happening currently if the details element is open and nothing happens if the element is closed. But this patch is for opening the element when the validation fails.

Should i open another issue in the queue?

Thanks
Sukanya

The general process is to leave a comment and change the ticket status to 'needs work'. Listing on-going bugs is expected this way. (:

This seems to still be a problem, ran into this on the path alias field.

Trigger testbot for testonly patch in #66.

Nice patch failed as expected.

Working on combined patch.

Combined new test with fix.

Removing needs test tag.

I did a manual test of #75 above combined with #32 of #2754977: Enhance formErrorHandler to include children errors on RenderElements. It showed that the container containing an error does indeed open as expected. However, other containers with no errors inside open as well. See these screenshots for some examples.

Error in the URL alias. Note that Authoring Information and Promotion Options have opened too.

Error in the authored date. Note that URL Path Settings and Promotion Options have opened too.

Missing title. Note that Authoring Information has opened too.

(needs work, see previous comment)

Found the problem mentioned in #77. The problem turned out to be in #2754977: Enhance formErrorHandler to include children errors on RenderElements (new patch coming up), not here. This one looks RTBC to me.

1. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -43,21 +44,64 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +      $elements =& $form;
   ...
   +        $child =& $elements[$key];
   

   $elements = &$form; please, otherwise it looks too much like &=

2. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -43,21 +44,64 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +        $elements['#children_errors'] = array();
   ...
   +          $elements['#children_errors'] = $child['#children_errors'];
   

   [] not array()

   So this is an internal property that shouldn't be defined manually? Or should this merge and not set to [] and overwrite

3. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -43,21 +44,64 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +        // it's parent structure.
   

   its

4. +++ b/core/lib/Drupal/Core/Render/Element/Details.php
   @@ -75,7 +75,9 @@ public static function preRenderDetails($element) {
   +    // Open the detail if specified or if a child has an error.
   +    if (!empty($element['#open']) || !empty($element['#children_errors'])) {
   

   Aha, so this is the actual bugfix.

5. +++ b/core/modules/node/src/Tests/NodeEditFormTest.php
   @@ -115,6 +115,14 @@ public function testNodeEdit() {
   +    $this->drupalGet("node/" . $node->id() . "/edit");
   

   Nit, but use single quotes when not interpolating.

6. +++ b/core/modules/node/src/Tests/NodeEditFormTest.php
   @@ -115,6 +115,14 @@ public function testNodeEdit() {
   +    $edit = array();
   

   []

#2 is the main question, the rest are nits

@tim, thanks for the review. 2: Yes that is an internal property not to be set manually. See the blocking issue.

New patch, adding the test that exactly 1 details-element should be open, no more. See also #2754977-37: Enhance formErrorHandler to include children errors on RenderElements.

If this is blocked on the other issue, it should be postponed.

Issue is blocked, see #2754977: Enhance formErrorHandler to include children errors on RenderElements.

I understand it's still postponed, but I'll change to RTBC so there is an update since I did test it. Here are my steps:

I applied patch https://www.drupal.org/node/2754977#comment-11868046 tested and no changes after running that patch alone because it was for data model only.
Then applied https://www.drupal.org/node/2346773#comment-11785294 from this page and issue has been fixed. Link is linking to the vertical tab and also it's expanding it.

Tested on FF 51.0.1 and Chrome 55.0

Perhaps per #89 it can go straight to RTBC but it should still be postponed until it is no longer blocked.

Blocker is in now.

Queued latest patch for retest and requested extra test against 8.4 to be sure.

So I guess the test failure is legitimate. There seems to be another open details element. Maybe the revision information one, because revisions are now defaulted to TRUE?

Working on it.

Working on this too. It's the last "Feb 1st must have" for Inline Form Errors, so I've the afternoon off work for it :-)
I'm in #drupal-contribute and the Drupal Slack.

Manual test was still oke. It was indeed the revision details box giving problems.
New test checks that after a form submit only one extra details element is open.

Ah bummer, some typ0s, working too quickly ;-)

Back to RTBC.

I also tested and can confirm it's good to go!

I'm doing manual testing of patch #97 with as many browsers and screen readers as I have available. So far things look good, but I'll post my results later today. Should take me an hour or so.

Tagging needs accessibility review while I do this. I'll remove it later and put RTBC back if no problems.

Manual testing after patch #97. I'm using the errorstyle testing module. The "Details closed" element is near the bottom of the the long `/error-style/form` page.

After a standard install, and enabling inline_form_errors and errorstyle modules, you can visit /error-style/form as an anonymous/authenitcated user and get the Bartik/Seven theme respectively. I tested both.

This is the list of browsers and screen readers I tested with. FWIW my Linux is Kubuntu 14.10.

linux/Firefox 50 (not expected to show the bug, but I tested with it anyway)
linux/Chromium 55
linux/Chrome 55
linux/Chrome 55 + ChromeVox
linux/Opera 42
linux/Vivaldi 1.6

It's fine in all of these scenarios. Now I'm going to test with VoiceOver on iOS and macOS.

Tested patch #97 with these too. All OK.

• iOS 10.2 + Safari
• iOS 10.2 + Safari + VoiceOver
• macOS Sierra + Safari
• macOS Sierra + Safari + VoiceOver
• Android 6 + Chrome
• Android 6 + Chrome + TalkBack
• Windows 7 + Firefox
• Windows 7 + Firefox + NVDA v2016.4 screen reader
• Windows 7 + IE 9
• Windows 7 + IE 9 + NVDA screen reader

There was no difference whatsoever with/without a screen reader in any combination I tested. Makes sense, this is just about opening the details elements when they contain an error; the internal form content of the details elements is not being addressed here.

I'm done with manual testing, all OK by me.

Code review: I looked back to the previous RTBC patch in #79, and worked though the interdiffs since then, up to the patch in #97.

Points 5 + 6 raised by @tim.plunkett in #80 had not been addressed. They were minor coding standards fixes, and I've updated the patch here.

Points 1-4 in #80 are no longer relevant. They were about things in core/lib/Drupal/Core/Form/FormErrorHandler.php, which isn't changed by the recent patches. I think Tim reviewed 2346773-75-open_details_on_error_COMBINED.patch which included the changes from #2754977: Enhance formErrorHandler to include children errors on RenderElements.

Much earlier in this issue, we didn't get an answer to whether the related vertical tabs issue is in scope for this issue or not. See comments #18 - #32 for details. (#2531700: Fragment links to children elements in closed grouping elements don't work) was marked as a duplicate of this one, but none of the patches here address it. The vertical tabs issue isn't mentioned in the issue title/summary, and it hasn't been mentioned here for over a year. I don't think any of the comments here mention that the other issue was closed as a duplicate of this one, or that it was in scope for this issue.

Given that:

• the latest patches here fix the behaviour of the details elements not opening (the issue summary), and
• we've met the Feb 1st must-have deadline from #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)

Can we re-open #2531700: Fragment links to children elements in closed grouping elements don't work as a follow-up to this one? It hasn't been decided whether this is a must-have or should-have for keeping Inline Form Errors in core. There is still a March 1st must-have deadline for #2828092: Inline Form Errors not compatible with Quick Edit in any case.

Tagging for product managers.

As per #104, the wrongly closed issue is reopened. I removed the tag for product manager here and added it again to the reopened issue. Also see our meta issue for details.

Patch in #103 is green again. I also manually tested it again, and it still works. So back to RTBC!

^ That was me saving issue credit. :)

Patch is straight-forward and has tests to demonstrate it does what it says it should do. Looks like the kerfuffle up there with product manager review was meant for another issue.

Committed and pushed to 8.4.x! Tentatively marking RTBC for 8.3.x backport as well, after code freeze.

@webchick: You say "Tentatively marking RTBC for 8.3.x backport", is there anyone that needs to be convinced it should be committed, or is it simply an hours-in-the-day problem?

Ok, cherry-picked to 8.3.x as well.

Yeah, mostly hours in a day problem. I had time on that day, but that day we were in 8.3.x code freeze. :)

Thanks @webchick! <3

Correcting issue credit.

@webchick, @xjm: I am sincerely sorry for my impatience in #111 :-(

Just to report on an attempt at an 8.2.7 backport and the work in progress, I applied this patch and tried to include necessary code dependencies to make it work. I wasn't successful, but here's the non-working in progress 8.2.7 patch, in case someone wants to pick it up. We ended up using the in-progress contrib solution here https://www.drupal.org/node/2787179#comment-11877050 which is working.