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

Comments

Antonnavi created an issue. See original summary.

Antonnavi’s picture

Antonnavi’s picture

FileSize
931 bytes
andypost’s picture

Status: Active » Needs review
Issue tags: +JavaScript
droplet’s picture

Status: Needs review » Closed (duplicate)
Antonnavi’s picture

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

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

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

Issue tags: +Needs tests

We can add JS tests for this now.

Antonnavi’s picture

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

andypost’s picture

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

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

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

Status: Closed (works as designed) » Needs review
FileSize
539 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

Status: Needs work » Needs review
FileSize
565 bytes
droplet’s picture

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
1.09 KB

Good catch!!

andypost’s picture

Assigned: Unassigned » Cottser

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

PS: FF 49 released

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Proper status

andypost’s picture

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

Status: Needs work » Needs review

Force tests re-run

Antonnavi’s picture

Status: Needs review » Reviewed & tested by the community

Tests pass-ed: Return RTBC status

alexpott’s picture

Title: Details HTML element not works without .details-wrapper » Details HTML element does not work without .details-wrapper
Cottser’s picture

Assigned: Cottser » 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

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

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

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.

Cottser’s picture

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

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.

Cottser’s picture

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

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

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)