git commit -m 'Issue #2493957 by droplet, mgifford, eiriksm: Add summary and errors support to native HTML5 details tag'

Problem/Motivation

Summary and errors support are no longer added to native HTML5 details tag

Proposed resolution

- Add Summary & Error support to HTML5 native Details tag.
- Trigger Details tag to OPEN state on HTML5 validation failure.

Remaining tasks

- Review patch

(Patch should be tested in all major browsers, especially Chrome and Firefox)

Steps to Reproduce

A: Add summary to native details tag

  1. Enable Book module
  2. Add Book Page
  3. Edit the new page's outline and select `- Create a new book -`
  4. Add another book page and assign it to the newly created book
  5. View the child page's book

Current behavior: no description in the details tag

Intended Behavior: selected book title in the details tag

B: Open Details tag when an error is triggered
Note, the easiest way to recreate this issue is to install the field group module.

  1. Install the Field Group module
  2. Add a required field to the Basec Page content type
  3. Go to the `manage form display` tab for the Basic Page content type
  4. Add a new group of type Details
  5. Add the required field to the Details group
  6. Add a new Basic Page and fill out all required fields except the required field contained within the details group
  7. Try to save the content

Current behavior: The form fails to submit with no notification of the failure
Intended Behavior: The form fails to submit, opens the group containing the field that failed to validate, and pops up the error notification.

Remaining Follow Up
- Rename `collapse-processed` to more meaningful, eg: `collapse-polyfill` or `details-polyfill`
- Change `$('')` to a button tag

User interface changes

-

API changes

-

Files: 
CommentFileSizeAuthor
#60 old-page-desc.png30.88 KBwolffereast
#60 duplicate-desc.png47.88 KBwolffereast
#42 after submit.png261.4 KBzuuperman
#42 before submit.png247.06 KBzuuperman
#33 core-js-details-error-2493957-33.patch3.15 KBnod_
#31 core-js-details-error-2493957-31.patch3.1 KBnod_
#31 interdiff.txt1.69 KBnod_
#27 2015-11-06_1-19-29.jpg109.42 KBdroplet
#25 2493957-25-details.patch4.04 KBdroplet
#17 2493957-17-details.patch4.53 KBdroplet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,152 pass(es). View
#17 testing.patch5.29 KBdroplet
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,046 pass(es), 5 fail(s), and 0 exception(s). View
#17 error-toggle.png17.31 KBdroplet
#17 book-outline.png20.58 KBdroplet
#9 2493957-details.patch4.79 KBdroplet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,026 pass(es). View
#8 2493957-details.patch3.46 KBdroplet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2493957-details_0.patch. Unable to apply patch. See the log in the details link for more information. View
#7 2493957-details.patch2.55 KBdroplet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2493957-details.patch. Unable to apply patch. See the log in the details link for more information. View
#5 details-2493957-5.patch2.91 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
details.patch2.61 KBdroplet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch details.patch. Unable to apply patch. See the log in the details link for more information. View
testing.patch3.33 KBdroplet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,470 pass(es), 5 fail(s), and 0 exception(s). View

Comments

The last submitted patch, testing.patch, failed testing.

mgifford queued details.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, details.patch, failed testing.

The last submitted patch, details.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Just a late night re-roll.

Status: Needs review » Needs work

The last submitted patch, 5: details-2493957-5.patch, failed testing.

droplet’s picture

Title: Add errors support to native HTML5 details tag » Add back errors & summary support to native HTML5 details tag
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2493957-details.patch. Unable to apply patch. See the log in the details link for more information. View
droplet’s picture

FileSize
3.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2493957-details_0.patch. Unable to apply patch. See the log in the details link for more information. View

Actually we have to keep it same styling.

droplet’s picture

FileSize
4.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,026 pass(es). View

Sorry. right patch here.

The last submitted patch, 7: 2493957-details.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2493957-details.patch, failed testing.

The last submitted patch, 8: 2493957-details.patch, failed testing.

Status: Needs work » Needs review

droplet queued 9: 2493957-details.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2493957-details.patch, failed testing.

Status: Needs work » Needs review

droplet queued 9: 2493957-details.patch for re-testing.

mgifford’s picture

What is the best way to test this?

I think I saw an extra (and empty) span here when looking at the summary/detail element /admin/appearance

droplet’s picture

Issue summary: View changes
FileSize
20.58 KB
17.31 KB
5.29 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,046 pass(es), 5 fail(s), and 0 exception(s). View
4.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,152 pass(es). View

I remove a change to make it move forward and easier for review.

UPDATED SUMMARY for REVIEWS STEPS.

droplet’s picture

Issue summary: View changes

The last submitted patch, 17: testing.patch, failed testing.

mgifford’s picture

Issue tags: +html5

Are you going to submit the "follow up issue to change `$('')` to button tag"?

droplet’s picture

@mgifford,

I will submit it after this issue.

YesCT’s picture

will this address what was noticed in #2473947-14: Prefix form-item-* classes with js- ? And add those back in?

"I think the javascript for the advanced settings details were these

which I git bisected to find were removed in #1838114: Change node form’s vertical tabs into details elements"

droplet’s picture

@YesCT,

This patch will add Summary back to HTML Details tag. But in your screenshot, that's verticalTabs, it's always working.

droplet queued 17: 2493957-17-details.patch for re-testing.

droplet’s picture

mgifford’s picture

I still couldn't replicate this following your instructions. Where is this missing?

Should I see it if I diff a book page somewhere like /node/3/edit with/without the patch?

Also, have these two follow up issues been created:
- Follow up issue to rename `collapse-processed` to more meaningful, eg: `collapse-polyfill` or `details-polyfill`
- Follow up issue to change `$('')` to button tag

droplet’s picture

Issue summary: View changes
FileSize
109.42 KB

1. Add a book page (should be BOOK)
2. go to Outline eg. /node/1/outline ( change 1 to Book page node ID)
3. You should see `BOOK OUTLINE (XXXXX)` after patched.

mgifford’s picture

ops.. wrong window.

mgifford’s picture

I opened up 2 windows with different versions in Simplytest.me:

Without
https://df9f8.ply.st/node/2/outline

With
https://df9fs.ply.st/node/2/outline

Can't see the difference.

droplet’s picture

what do you see ? any screenshot ? caching ?

nod_’s picture

Priority: Normal » Major
FileSize
1.69 KB
3.1 KB

Made a little change so that the toggle function is only used when the polyfill is involved, never called for native details. The previous patch did not update the aria state of the details element when using native details. Simplified it by clicking on the summary to open the details element, that way aria gets updated properly in both cases.

With this patch drupal summaries are back for native details. This should be major at least.

droplet’s picture

+++ b/core/misc/collapse.js
@@ -106,38 +106,29 @@
-      this.toggle(true);
+      this.$node.find('> summary').trigger('click');

the change will make OPENED state to CLOSED. It's why I made it always TRUE.

nod_’s picture

woops that's right. Added a condition to check for open state before triggering the click.

droplet’s picture

+++ b/core/misc/collapse.js
@@ -113,6 +122,15 @@
+    /**
+     * Handle invalid event
+     */
+    invalidHandler: function () {
+      if (!this.$node.attr('open')) {
+        this.$node.find('> summary').trigger('click');
+      }

OK. Mainly nod_ restore my changes in `toggle()` and trigger OPENED state in `invalidHandler()` directly. All looks good to me. Since most changes made by me. I will leave another one for final review.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

I did more review than patching

alexpott’s picture

+++ b/core/themes/classy/css/components/collapse-processed.css
@@ -30,3 +30,6 @@
+.collapse-processed .details-title {
+  color: #333;
+}

Why is this necessary?

droplet’s picture

to standardize the color

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the manual testing steps and screenshots! Here is how I tested this patch.

  1. Install 8.0.x and enable the Book module.
  2. On /node/add/book, create a "Parent" book node. Under "Book outline", set the book to "Create new book". Save the node.
  3. Click the "Outline" tab. In HEAD, the book name is not displayed. In the patch, the book name is displayed.
  4. On /node/add/page, create a node that has a URL alias that does not start with a leading slash and try to save. In HEAD, it is not clear where the error is. With the patch, the URL alias element is expanded to show the error.

However, regarding #36 and #37, I also could not find where the CSS was being applied. Can we remove that from the patch and possibly put it in a separate issue with steps to reproduce the color inconsistency?

droplet’s picture

Have you test in firefox ? (the browsers doesn't support native details)

nod_’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Reviewed & tested by the community

No indication of FF testing in #38. CSS needed for consistency, back to RTBC.

zuuperman’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
247.06 KB
261.4 KB

I tested this on firefox and chrome via fieldgroups.
In chrome everything is ok after patch: the group opens and the error is shown on the textfield.
In firefox, the group opens, but the error is shown on top of the screen.

before submit:

after trying to submit:

nod_’s picture

Status: Needs work » Reviewed & tested by the community

Let's open a different bug for that one, not everyone uses fieldgroup module.

nod_’s picture

Checked it out with fieldgroup but there is nothing we can do about it. Byproduct of our polyfill for the details element. I don't see how another polyfill would solve that either. It's the danger of having hidden required fields.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This patch seems to be breaking something... I've tested this doing

On /node/add/page, create a node that has a URL alias that does not start with a leading slash and try to save. In HEAD, it is not clear where the error is. With the patch, the URL alias element is expanded to show the error.

in Firefox 43.0 (OS X) and the automatic opening on the details element when there is an error inside does not work. It works great on chrome 47.0.2526.106. Without this patch the situation is reversed??? Very weird.

andypost’s picture

This may need JS testing instead of manual

andypost’s picture

This may need JS testing instead of manual

andypost’s picture

This may need JS testing instead of manual

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

Issue tags: +Needs JS testing

Good call @andypost, we have a framework in core now so let's give it a try. Adding tag for JS testing.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

dmsmidt’s picture

Status: Needs work » Closed (duplicate)

It seems this is already fixed in core here #2346773: Details form element should open when there are errors on child elements.
Please re-open if I'm wrong.

droplet’s picture

Status: Closed (duplicate) » Needs work

This is diff things and more. We do it in frontend, not backend.

dmsmidt’s picture

That it are two different approaches I agree on.
But at least it is partly fixed right?
"If there are errors on an element in a details element, that details element should be opened". That is fixed by the issue I mention, and is what is described in #38.

So the part about the summary only needs to be fixed? Is this still actual?

droplet’s picture

Nope.

#2346773: Details form element should open when there are errors on child elements is changed the HTML output. (All done in backend)
This issue is working on frontend when the frontend form validation is failed. (All done in frontend)

Both patches trying to do the same thing but on diff stages.

Torenware’s picture

Issue tags: -Novice

Dropping back the Novice status, since it looks like some coding will be needed here.

wolffereast’s picture

Issue tags: +Baltimore2017

Beginning Triage

wolffereast’s picture

Issue summary: View changes

Still applies in 8.4.x.

I am seeing an oddity when the description is added to the book description. It is adding both the Book and the Parent Item to the description. Is this intended?

wolffereast’s picture

FileSize
47.88 KB
30.88 KB
wolffereast’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue summary: View changes
Issue tags: +Triaged for D8 major current state
cilefen’s picture

@wolffereast I am going over the issues analyzed at the Baltimore triage in order to assign credit. I would like to give you credit for the work done, but I can't tell whether or not all steps of the triage were completed by reading the comments. We like to see documented steps (even if brief). Here are some made-up examples of documented triage steps:

  • I tested the steps to reproduce and they did (or did not) work (so I am tagging it "Needs issue summary update").
  • I searched for duplicate issues but could not find any.
  • I checked the issue summary and it is accurate and up-to-date.
  • Etc...

Thank you!

wolffereast’s picture

@cilafen Sorry about that!
The issue still exists.
Updated the issue summary with additional steps to reproduce.
Found a related issue in the queue, though it is not an exact duplicate so I'm not comfortable marking either as closed.

dmsmidt’s picture

@wolffereast, #2848507: Indicate that grouping elements have child element errors for ux and a11y is no duplicate. However when that gets in, it would be nice that the client side HTML5 validation logic could also take care of the parent grouping element (details).

droplet’s picture

#60 is slightly out of this issue scope.