How to reproduce:

  1. Open any page with "details" HTML element on it (using Firefox 47.0), for example Performance page (/admin/config/development/performance)
  2. Click by headings: "Bandwidth optimization" or "Caching" or "Clear cache"
    • Expected result: Area below clicked heading should be collapsed
    • Current result: Nothing happend

Alternative way how to reproduce:

  1. Install Field Group module (https://www.drupal.org/project/field_group)
  2. Add new field group type "Details" to some node display (on Manage display page)
  3. Place to field group "Details" some fields
  4. Go to node display page using Firefox 47.0
  5. Click by added field group type "Details" title
    • Expected result: Area below clicked title should be collapsed
    • Current result: Nothing happend

Everything works fine if use Chrome browser.
This issue appears because:

  • Firefox not fully supported "details" HTML element: https://bugzilla.mozilla.org/show_bug.cgi?id=591737#c265
  • JS file /core/misc/collapse.js not correct handle this situation:
    • It not setup initial state (expanded/collapsed) for section inside details element
    • It try .toggle() CollapsibleDetails JS object instead of section inside details element

Firefox 49 released with support of https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details

CommentFileSizeAuthor
#18 details-wrapper.patch1.09 KBdroplet
#17 2792593-details_collapse_in_firefox_2.patch565 bytesAntonnavi
#15 2792593-details_collapse_in_firefox_1.patch539 bytesAntonnavi
#3 2792593-details_collapse_in_firefox.patch931 bytesAntonnavi
#2 2792593-details_collapse_in_firefox.patch765 bytesAntonnavi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Antonnavi created an issue. See original summary.

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
FileSize
2792593-details_collapse_in_firefox.patch765 bytes
Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
FileSize
2792593-details_collapse_in_firefox.patch931 bytes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
Issue tags: +JavaScript
droplet’s picture

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

We have patch for a while:
#2493957: Add back errors support to native HTML5 details tag

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
Status: Closed (duplicate) » Needs review

Thank You for review andypost and droplet!

Re-open this issue because patch listed in:
#2493957: Add back errors & summary support to native HTML5 details tag
Not solve my issue. After apply it have the same issue with no reaction (expand/collapse) by click "details" HTML elements in Firefox 47.0 (Ubuntu).

droplet’s picture

droplet CreditAttribution: droplet commented
Issue tags: +Needs manual testing, +linux, +ubuntu

tagging for Ubuntu testers. I can't reproduce it with FF48 in Windows & Mac.

It try .toggle() CollapsibleDetails JS object instead of section inside details element

By the way, I'm interesting on this point. Any side effects? Can you explain a bit more.

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.

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented

Thank You for review, droplet!

It try .toggle() CollapsibleDetails JS object instead of section inside details element

Here I mean code:

    onLegendClick: function (e) {
      this.toggle();
      e.preventDefault();
    },

If add console.log(this); inside onLegendClick function You will see:

CollapsibleDetails { $node={...}, $summary={...}, setupSummary=function(), more...}

Means: .toggle() applied to CollapsibleDetails object and call:

    toggle: function () {
      var isOpen = !!this.$node.attr('open');
      var $summaryPrefix = this.$node.find('> summary span.details-summary-prefix');
      if (isOpen) {
...

But inside toggle() function not available code that will handle show/hide <section>...</section> area for browsers that not fully support <details></details> tag.

Full support of <details> and <summary> elements will be added in Firefox 49 https://developer.mozilla.org/en-US/Firefox/Releases/49#HTML

Currently in Firefox 47.0 (Ubuntu 15.10) <details> and <summary> elements not works correct.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue tags: +Needs tests

We can add JS tests for this now.

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented

jibran, can you plz provide some HowTo/article about how to create/write JS tests.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: -Needs tests

@Anton there's https://www.drupal.org/node/2716803 and https://www.chapterthree.com/blog/javascript-testing-comes-to-drupal-8

But I'm sure there's no way to test this issue with phantom cos it based on Chromium but the bug in FF47(48)
So only manual testing possible

PS: slimerjs is a FF based tests but have no integration with Drupal

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Postponed (maintainer needs more info)

If #9 really a bug in Firefox (Ubuntu), pretty much things will be broken (not just Drupal) and I believe there's will be a 47.0.1 or so to fix it immediately.

I loaded up my Ubuntu box and do not find the problem. Please update the Issue Summary to reflect the exact problem (Core Only). Thanks!

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

Strange thing, after update to latest version 8.1.9 details element works correct on Firefox 47.
Will research a bit more what was an issue reason.

droplet, thank You for review and help!

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
Status: Closed (works as designed) » Needs review
FileSize
2792593-details_collapse_in_firefox_1.patch539 bytes

This issue appears because:

  1. Handling expand/collapse details area (for browsers that not fully supported details element) handled with JS + CSS
  2. JS part is:
    • core/misc/collapse.js
    • core/misc/details-aria.js
  3. CSS part is:
    • core/themes/stable/css/system/components/details.module.css
  4. Working logic:
    • collapse.js set attribute "open" = true/false for expand/collapse details area
    • details.module.css add styles that collapse details area if it is not open, CSS code is:
      .js details:not([open]) .details-wrapper {
  display: none;
}

As we can see CSS expects some HTML element wrapper (around details area content) with "details-wrapper" CSS class. If we check core/themes/stable/templates/form/details.html.twig template file code
(this template used for output details element if used theme not define own details.html.twig template file):

<details{{ attributes }}>
  {%- if title -%}
    <summary{{ summary_attributes }}>{{ title }}</summary>
  {%- endif -%}

  {% if errors %}
    <div>
      {{ errors }}
    </div>
  {% endif %}

  {{ description }}
  {{ children }}
  {{ value }}
</details>

As we can see in this file not available some wrapper (with "details-wrapper" CSS class) around "errors", "description", "children", "value" items. From this reason (if used this template file) details element will not works correctly in browsers that not fully support details element.

In admin themes this issue not appears because Bartik and Seven themes use as base theme Classy theme that define own details.html.twig template file with code:

<details{{ attributes }}>
  {%- if title -%}
    <summary{{ summary_attributes }}>{{ title }}</summary>
  {%- endif -%}
  <div class="details-wrapper">
    {% if errors %}
      <div class="form-item--error-message">
        <strong>{{ errors }}</strong>
      </div>
    {% endif %}
    {%- if description -%}
      <div class="details-description">{{ description }}</div>
    {%- endif -%}
    {%- if children -%}
      {{ children }}
    {%- endif -%}
    {%- if value -%}
      {{ value }}
    {%- endif -%}
  </div>
</details>

As we can see here available <div class="details-wrapper">...</div> element and details element works correct in any browser.

For fix this issue we should add <div class="details-wrapper">...</div> wrapper around "errors", "description", "children", "value" items in core/themes/stable/templates/form/details.html.twig template file. Attached patch add this wrapper.

Status: Needs review » Needs work

The last submitted patch, 15: 2792593-details_collapse_in_firefox_1.patch, failed testing.

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
Status: Needs work » Needs review
FileSize
2792593-details_collapse_in_firefox_2.patch565 bytes
droplet’s picture

droplet CreditAttribution: droplet commented
Title: Details HTML element not works in Firefox 47.0 (Ubuntu) » Details HTML element not works without .details-wrapper
Component: javascript » markup
Issue tags: -Needs manual testing, -linux, -ubuntu
FileSize
details-wrapper.patch1.09 KB

Good catch!!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Assigned: Unassigned » star-szr

+1 to fix both themes, but how that fits in our BC policy
Suppose Cottser should decide

PS: FF 49 released

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Proper status

andypost’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: details-wrapper.patch, failed testing.

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
Status: Needs work » Needs review

Force tests re-run

Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented
Status: Needs review » Reviewed & tested by the community

Tests pass-ed: Return RTBC status

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: Details HTML element not works without .details-wrapper » Details HTML element does not work without .details-wrapper
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Needs review

I think this would generally be okay to fix in core/stable although there is some risk there so it probably couldn't go into 8.2.x. My concern is how to avoid breaking it again. I think this div was removed because it looked like it wasn't needed. #2407725: Remove classes from system templates d*.html.twig.

And to be clear, this is only in (now slightly older) versions of Firefox, correct? Maybe the issue should be re-titled if so.

I'm wondering if a different approach would be possible using <details> feature detection via Modernizr - we already have <details> detection in our Modernizr build. Any page with a <details> element would have a details or no-details class on the html element. I don't fully understand the issue though, so this may not be possible.

droplet’s picture

droplet CreditAttribution: droplet commented

Before my patch #18, I thought using JS to guarantee that with wrappers. However, it's Performance issue for render & I thought all these HTML code are the "Details Pattern"

droplet’s picture

droplet CreditAttribution: droplet commented

Themers should be acknowledged that a polyfill layer and theme it correctly. Don't repeat #2407725: Remove classes from system templates d*.html.twig mistakes in contributed themes.

with or without Native Details. It's a pattern I meant. We theming with it (considered as a container DIV), eg: in Seven theme, we do it:

details > .details-wrapper {
    padding: 0.5em 1.5em;
}
Antonnavi’s picture

Antonnavi
Primary language Ukrainian
Location Berlin
CreditAttribution: Antonnavi at Skilld commented

Thank You for review Cottser and droplet!

And to be clear, this is only in (now slightly older) versions of Firefox, correct? Maybe the issue should be re-titled if so.

It is not only about Firefox 47.0, it's about any browser that not fully support <details> HTML element. As noted in comment #15 if user use browser that not fully support <details> HTML element used:

  • /core/misc/collapse.js
  • /core/misc/details-aria.js
  • core/themes/stable/css/system/components/details.module.css

files for handle this situation and make <details> works.
More detailed about logic in comment #15.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

I spoke to @lauriii about this at BADCamp. I was reaching for something like a js- prefixed class but forgot that it's essentially just a hack for data attributes.

So to make this more explicit and less likely to break in the future, how about we…

  1. Make this a data attribute everywhere (core, stable, classy, seven, bartik) like <div data-details-content> with a Twig comment above explaining that the wrapper is for browsers that don't natively support <details>. Everything but core and stable will keep the existing details-wrapper class.
  2. Copy details.module.css from Stable to Classy.
  3. Change details.module.css in core and Stable to reference the data attribute as well as the details-wrapper class so that anyone building off of Stable and using a modified details template still gets the original CSS.
droplet’s picture

droplet CreditAttribution: droplet commented

Change to different styling attributes seems wrong direction. Created another layer for anyone. (It will cause a lot of changes in JS also.)

Theoretically, .details-wrapper isn't totally part of JS script (collpase.js). Turned JS off, we still maintain the same style.

We should look into all usages in CORE, not this single component with bug.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

Thanks for the feedback. The point of using a data attribute here is that we basically have a policy that frontend devs massaging markup know not to remove data attributes. A class without a js- prefix is usually fair game to be removed.

I tested and doing that requires no JS changes, only minor CSS changes. I can put up a patch if you like.

We should look into all usages in CORE, not this single component with bug.

I don't quite follow, can you expand on that thought?

droplet’s picture

droplet CreditAttribution: droplet commented

Thanks

The point of using a data attribute here is that we basically have a policy that frontend devs massaging markup know not to remove data attributes.

It's interesting new stuff to me. However, it won't help someone like me never know it before. Nevermind, it seems not in this scope.

I thought a Twig comment is enough. Must I miss the point? (If you could make a patch for #30, that may help to understand)

I don't quite follow, can you expand on that thought?

Searching the CORE files, there're 45 matches of .details-wrapper. It's not good to rename or adding another new identity to deal with it.

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.

droplet’s picture

droplet CreditAttribution: droplet commented

It may blocking #2880703: Status report page without JavaScript is messed up

Furthermore,

this is a bug report. #30.1 and #30.3 is a new feature request to enhance the component I thought. We should add a follow-up for that.

`data-details-content` affected the way to use JS API (Or CSS coding)

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

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

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.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.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Postponed (maintainer needs more info)
Issue tags: -JavaScript +JavaScript, +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Following the steps to replicate this from the IS and I was not able to get the issue. Using the latest version of firefox and chrome.

Are there additional steps or is this no longer an issue?

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.