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

-

CommentFileSizeAuthor
#76 Screenshot from 2022-04-11 16-55-19.png49.76 KBlarowlan
#60 old-page-desc.png30.88 KBwolffereast
#60 duplicate-desc.png47.88 KBwolffereast
#42 after submit.png261.4 KBnils.destoop
#42 before submit.png247.06 KBnils.destoop
#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
#17 testing.patch5.29 KBdroplet
#17 error-toggle.png17.31 KBdroplet
#17 book-outline.png20.58 KBdroplet
#9 2493957-details.patch4.79 KBdroplet
#8 2493957-details.patch3.46 KBdroplet
#7 2493957-details.patch2.55 KBdroplet
#5 details-2493957-5.patch2.91 KBmgifford
details.patch2.61 KBdroplet
testing.patch3.33 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

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

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Status: Needs work » Needs review
FileSize
details-2493957-5.patch2.91 KB

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

droplet CreditAttribution: droplet commented
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
2493957-details.patch2.55 KB
droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
2493957-details.patch3.46 KB

Actually we have to keep it same styling.

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
2493957-details.patch4.79 KB

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

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented

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

droplet CreditAttribution: droplet commented
Issue summary: View changes
FileSize
book-outline.png20.58 KB
error-toggle.png17.31 KB
testing.patch5.29 KB
2493957-17-details.patch4.53 KB

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

UPDATED SUMMARY for REVIEWS STEPS.

droplet’s picture

droplet CreditAttribution: droplet commented
Issue summary: View changes

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

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Issue tags: +html5

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

droplet’s picture

droplet CreditAttribution: droplet commented

@mgifford,

I will submit it after this issue.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Parent issue: » #1838114: Change node form’s vertical tabs into details elements

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

droplet CreditAttribution: droplet commented

@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

droplet CreditAttribution: droplet commented
Issue tags: +Needs manual testing, +js-novice, +Novice
FileSize
2493957-25-details.patch4.04 KB

Rerolled

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented

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

droplet CreditAttribution: droplet commented
Issue summary: View changes
FileSize
2015-11-06_1-19-29.jpg109.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

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented

ops.. wrong window.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented

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

droplet CreditAttribution: droplet commented

what do you see ? any screenshot ? caching ?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Priority: Normal » Major
FileSize
interdiff.txt1.69 KB
core-js-details-error-2493957-31.patch3.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

droplet CreditAttribution: droplet commented 
+++ 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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
FileSize
core-js-details-error-2493957-33.patch3.15 KB

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

droplet’s picture

droplet CreditAttribution: droplet commented 
+++ 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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

I did more review than patching

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
+++ 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

droplet CreditAttribution: droplet commented

to standardize the color

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
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

droplet CreditAttribution: droplet commented

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

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

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

nils.destoop’s picture

nils.destoop CreditAttribution: nils.destoop as a volunteer and at Wunder commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
before submit.png247.06 KB
after submit.png261.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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
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

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
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

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

This may need JS testing instead of manual

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

This may need JS testing instead of manual

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Issue tags: +Needs JavaScript 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

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt at Triquanta commented
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

droplet CreditAttribution: droplet commented
Status: Closed (duplicate) » Needs work

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

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt at Triquanta commented

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

droplet CreditAttribution: droplet commented

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

Torenware CreditAttribution: Torenware as a volunteer commented
Issue tags: -Novice

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

wolffereast’s picture

wolffereast CreditAttribution: wolffereast commented
Issue tags: +Baltimore2017

Beginning Triage

wolffereast’s picture

wolffereast CreditAttribution: wolffereast commented
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

wolffereast CreditAttribution: wolffereast commented
FileSize
duplicate-desc.png47.88 KB
old-page-desc.png30.88 KB
wolffereast’s picture

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

cilefen CreditAttribution: cilefen commented

@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

wolffereast CreditAttribution: wolffereast commented
Related issues: +#2848507: Indicate that grouping elements have child element errors for ux and a11y

@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

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt commented

@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

droplet CreditAttribution: droplet commented
Issue tags: -html5, -Needs manual testing, -js-novice

#60 is slightly out of this issue scope.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Smile commented
Title: Add back errors & summary support to native HTML5 details tag » Add back errors support to native HTML5 details tag

let's take the summary in the other issue and focus this one on the error part.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue summary: View changes
Status: Needs work » Closed (cannot reproduce)
Issue tags: +Bug Smash Initiative
FileSize
Screenshot from 2022-04-11 16-55-19.png49.76 KB

This came up on the bug smash triage, looking at the latest patch it was 7 years ago, before we had es6.

However, after manual testing per the steps to reproduce, it no longer occurs