Problem

Goal

  • Replace collapse.js with a proper polyfill for details/summary.

Details (irony?)

Comments

sun’s picture

Title: Replace collapse.js with a proper polyfill » Replace collapse.js with a proper polyfill for details
mgifford’s picture

I'm looking for input from @mbrett5062 on this. Really great the things that a modern browser and HTML5 is taking care of.

mbrett5062’s picture

Just some quick thoughts/ideas.

I see that Mathias Bynens feature detection that is used by @sun is now in 'modernizer', and also that they are getting close to releasing version 3 which breaks it all up into a pluggable system.

Meanwhile, I have looked over his latest version, and it is much improved, taking the whole thing (detection and polyfill) will be a benefit to us. I commented over on #1168246: Freedom For Fieldsets! Long Live The DETAILS. (Inappropriately).

The new script works better across browsers, and I notice that he no longer tries to add a link because Chrome now supports this element. Instead his script now makes all browsers look and work the same. Then just style in CSS as you wish.
I.E. change cursor to pointer and text color to blue on 'summary' element to emulate a link and give user feedback.

He also includes new aria roles for accessibility.

Here is the link to his code. jquery-details

Gaelan’s picture

Title: Replace collapse.js with a proper polyfill for details » Replace collapse.js with a proper polyfill for <details>
sun’s picture

Status: Postponed » Active
sun’s picture

The change notice for Modernizr mentions a feature detection for details: http://drupal.org/node/1852968

Since the one I copied into collapse.js seems to be the wide-spread-adopted one, we might actually have duplicate JS in core now ;)

mbrett5062’s picture

@sun the one you copied into collapse.js, was that not from Mathias Bynens? If so that is the one included in modernizer, so your could be removed. I really don't care were it lives, but 1 only is a good idea I think.

Also as I mentioned before, currently in Google Chrome the summary element looks horrible. I know it is browser default styling, but no visual clue that this is clickable is a UX regression for our users, IMHO. If as I stated (and Mathius has done) we do not add the summary as a link, instead just as plane text, that is clickable. Then style all with the same text color as a link and change the cursor to a pointer, this will make all browsers, regardless of support for details, look and feel the same.

ry5n’s picture

Just like to weigh in that I support styling the details summary to improve affordances. I see no problem modifying the browser default appearance; we do this all the time with CSS on links, buttons, inputs and more, all to improve the experience for users, and <details> is no different. It makes sense to me to add the cursor: pointer and make native details consistent across all browsers.

jwilson3’s picture

The default <details> on browsers that support it is collapsed; when it is expanded it becomes <details open>. The only potential problem I see here (from an extremely quick review of a jsbin test) is that with javascript turned off on a browser that doesnt support the details tag, it is just rendered as a paragraph or div or something, so no collapsing. This could present issues for people that need to style things based on whether something is collapsed or expanded. Any CSS styles that target the default details { /*presume collapsed */ } will be wrongly styled, and the details[open] { /*style expanded */ } would never be triggered.

The issue boils down to whether Drupal 8 still claim to "work" with javascript disabled?

ry5n’s picture

@jwilson We can build the CSS up from the expanded state, like so:

details {
  /* expanded styles */
}

/* Assume the polyfill adds the 'collapsed' class */
details:not([open]),
details.collapsed {
  /* collapsed styles */
}

In browsers that don't understand <details>, with JS off, this should still work. I’m pretty sure this is how it’s done in core ATM.

ry5n’s picture

Issue summary: View changes

Updated issue summary.

Jelle_S’s picture

I'm not sure if this can be solved with the polyfill that will be created, but states.js also has a regression for browsers that support the details element.

I found this when I was updating the testswarm module:

$form['details2'] = array(
    '#type' => 'details',
    '#title' => t('Details2'),
    '#collapsible' => TRUE,
    '#collapsed' => TRUE,
    '#states' => array(
      'expanded' => array(
        ':input[name="expanddetails2"]' => array('value' => 'expand'),
      ),
    ),
  );
  $form['details2']['item'] = array(
    '#type' => 'item',
    '#title' => t('Do you seee me?'),
    '#markup' => t('If you can see me the details is not collapsed!'),
  );
  $form['expanddetails2'] = array(
    '#type' => 'textfield',
    '#title' => t('Change details state'),
    '#description' => t("Type 'expand' to expand Details2."),
  );

In browsers that do not support the details element, typing 'expand' in to the textfield, will expand the "details" element, in browsers that do support the details element, nothing will happen.

EDIT:
Same story for #collapsible => FALSE: Works in browsers that don't support the details element, doesn't work in browsers that do support it.

EDIT:
never mind: #1852104: #type details: Remove #collapsible property

sun’s picture

Issue tags: +polyfill
sun’s picture

Issue summary: View changes

...added #1858600

sun’s picture

collapse.js still exists in HEAD. Why? :)

nod_’s picture

Because everything except chrome.

klonos’s picture

Because everything except chrome.

Yeah, but #1252178: Add Modernizr to core is in, so we can use if (Modernizr.details) and attach a polyfill to a .no-details class. No? What is our policy on using polyfills?

klonos’s picture

Issue summary: View changes

;)

nod_’s picture

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

Nice, but that that important.

Wim Leers’s picture

s/that that/not that/? :)

mgifford’s picture

There is also this link here that should be considered when we write this polyfill:
http://accessibleculture.org/articles/2012/03/screen-readers-and-details...

jwilson3’s picture

^ The author of that post claims that he himself cannot find a use case for having a link nested inside a summary. I would be of a mind to just allow it (since the html5 spec currently does) but not to take any opinion on it, or to go out of our way to write or customize our own polyfill that somehow handles the special case. Basically, take the stance of "you're shooting yourself in the foot if you do this, so if you're sure you want to do that, you can also handle the consequences".

So at most, this concern becomes a candidate documentation issue, something as simple as a note along the lines of:

Though the HTML5 spec allows links inside the <summary> tag, browsers, polyfills, and screen readers don't support this consistently. If you absolutely need to put a link inside a summary tag, follow the recommendations here (insert link from previous comment here).

Cottser’s picture

That sounds very sensible.

mgifford’s picture

Agreed, thanks @jwilson3.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

FatherShawn’s picture

Looking through the list in #17, I found a lot to recommend https://github.com/kapitancho/logifill-details. Since we our Modernizer script already tests for details support, I tweaked Marian's script to use Modernizr, and it's working well supporting a D8 custom template in FireFox, IE 11 and Edge: https://github.com/FatherShawn/logifill-details

droplet’s picture

I don't think we should always push Drupal to an area that looks like more HTML5 friendly but actually it's not. Drupal is going to be a bigger boat. Details even not supporting by 2 big brother. Firefox is the one we all known they always does good things. But they didn't on the list.
http://caniuse.com/#search=details

1. Do Drupal really needed Details tag? Or Details like features only. If latter, we do it in CORE already.
2. Do Drupal going to support fully Details tag ? Or part of features drupal needed only. If latter, we do it in CORE already.
3. Elevate the lib we going to be picked is really fully support Details. If there's one, let's move on. If no one, I think we should consider what features in Details we really needed for CORE now and do it.

mgifford’s picture

Details was needed for accessibility. That's not the only reason it was done, but fieldsets can't be nested and despite Firefox's failure to get onboard with this, it is the right pattern.

Here's the firefox bug. Would be good for folks here to +1 this getting into this popular browser:
https://bugzilla.mozilla.org/show_bug.cgi?id=591737

catch’s picture

Reading that issue, support is already in the firefox codebase, but it's not enabled yet (has to be enabled with a preference explicitly):
https://bugzilla.mozilla.org/show_bug.cgi?id=591737#c265

So potentially not too far off

droplet’s picture

@mgifford,

Thanks for the info. I'm not familiar with accessibility. But when I working on #2493957: Add back errors & summary support to native HTML5 details tag, I found current CORE (non-patch) provided more info than native Details tag. eg. it added Show/Hide:

   $('<span class="details-summary-prefix visually-hidden"></span>')
        .append(this.$node.attr('open') ? Drupal.t('Hide') : Drupal.t('Show'))

Will screen reader given extra hints on native Details automatically ?

https://html.spec.whatwg.org/multipage/forms.html#the-details-element
Reading this ref, there's 1 thing missing in Drupal:
- a `toggle` event.

I don't see any ref in SPEC but Chrome does it:
- handle SPACE key

Our collapse.js is a Polyfill lib already. We can do it further to move main code out of Drupal.behaviors.

(Noted that: No libs in above list supporting `toggle` event)

mgifford’s picture

Thanks @droplet - I've been looking for time to dig into this and just not finding it.

From what I recall, we did test this behavior to verify that it was read out properly by a screen reader, but don't have the issue handy.

There are other docs on other approaches:
http://html5doctor.com/the-details-and-summary-elements/
https://mathiasbynens.be/notes/html5-details-jquery

I'm not a JS guy, so not sure what our options are. We can also leverage Drupal.announce() to announce this information.

FatherShawn’s picture

Native support for details-summary in FireFox is slated for v.49 - September 2016: https://bugzilla.mozilla.org/show_bug.cgi?id=591737#c275

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

andrewmacpherson’s picture

Issue tags: +accessibility

Native support for details/summary was indeed turned on by default in Firefox 49, and I've confirmed it works when JS is disabled.

The native Firefox 49 <details> seems to be implemented using ARIA roles, states and properties; just like Chrome/Opera (and Safari? I think it was introduced before the Blink fork).

The native Firefox behaviour matches our own collapse.js:
- The <summary> element has a role=button attribute (and is announced as such).
- When focussed, the summary element responds to enter and spacebar key presses.
- aria-collapsed AND aria-pressed state attributes are present on the element, and are updated when the <details open> state changes.
- The <summary> element has an aria-controls attribute, pointing at the parent <details> element.

This leaves IE and Edge as the only major browsers which don't support <details>. It's currently "under consideration" for Edge, but I don't think we can expect it in any version of IE.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.