Collapsible fieldset have duplicated and wrong aria-expanded

Here's a first patch for this

This second patch should fix the duplication issue, too. Regarding the JavaScript thing, I've no idea about the right place for the necessary additions. I had a look at collapse.js, but this file doesn't seem to be involved.

Check as Need review to run tests.

@dom patch #2 includes patch #1! Not sure if this is best practice?

Info for the JS part: the surrounding div has an attribute 'open'. It gets toggled even when js is disabled. If we find the place where this magic happens, we're almost done.

Ah, that's why I didn't get it. I've used Chrome :-)

We need a separate js file for that, we can remove all the aria stuff from the fallback script and handle that for both native and polyfilled in a new file (that file can be added to the same library, just needs to be in a different file).

@dom It's up to you, I have no idea about this modernizr stuff, sorry!

Talked this through, Dom. is working on it.

Missing the actual aria js file.

1. +++ b/core/misc/details-aria.js
   @@ -0,0 +1,16 @@
   +  Drupal.behaviors.detailsaria = {
   

   to make it easier to read I'd prefer detailsAria

2. +++ b/core/misc/details-aria.js
   @@ -0,0 +1,16 @@
   +      $(context).find('details').on('click.details_aria', function(event) {
   

   This is missing the .once('detailsAria') after the find.

   I'd rather use event delegation on body, something like $('body').once('detailsAria').on('click.detailsAria', 'summary', function () {});

3. +++ b/core/misc/details-aria.js
   @@ -0,0 +1,16 @@
   +        this.$summary = $(event.currentTarget).find('summary');
   

   Since listening to clicks on summary is more direct we can do something like var parent = event.currentTarget.parentNode;

   I don't think we need to add anything to this to make it work.

4. +++ b/core/misc/details-aria.js
   @@ -0,0 +1,16 @@
   +        var isOpen = !!$(this).attr('open');
   

   On firefox this doesn't work so well, probably a event listener thing. Works fine in chrome.

We really don't want to use weights. Thanks for finding out the root issue with FF/chrome. Feel free to upload the patch you have now even if it doesn't solve everything :)

I'll look into that last issue when I can.

We have eslint errors:

core/misc/details-aria.js
   9:10  error  Expected indentation of 10 characters  indent
   9:24  error  Expected '===' and instead saw '=='    eqeqeq
  12:22  error  Expected '===' and instead saw '=='    eqeqeq

✖ 3 problems (3 errors, 0 warnings)

About the code, I'd still like to use event delegation because of performance. On the module page (which has a lot of details element) we go from 6ms to 1ms for init.

Here is an updated patch. It solves the issue with firefox and chrome too. It's not really novice after all so I'm removing that :)

better with the new file…

+++ b/core/misc/details-aria.js
@@ -0,0 +1,28 @@
+        // We're basing the value of

Half a sentence

Should it be this:

// We're basing the value of the open aria attribute.

I think that's what's happening. Thanks for catching the half sentence.

Updating tags to indicate that this is a feature to improve accessibility and that the issue needs accessibility review.

Yup, sorry about that, fixed the comment.

Again, forgot the new file.

I've tested this and it looks ready to be RTBC again. It's much more descriptive now.

+++ b/core/misc/details-aria.js
@@ -0,0 +1,28 @@
+        // The aria state is based on the 'open' attribute value of the parent.
+        function setAriaAttr() {
+          // It was open and we're closing it.
+          return $parent.attr('open') === 'open' ? 'false' : 'true';
+        }
+
+        $summary.attr('aria-expanded', setAriaAttr);
+        $summary.attr('aria-pressed', setAriaAttr);

It feels weird to call a function setAriaAttr() when in gets an attribute. Also we appear to call this method twice for little reason.

You're right, missed the big picture.

@Dom. you should get the same result with @nod_'s code as $variables['summary_attributes']['aria-pressed'] = $variables['summary_attributes']['aria-expanded']; and you're using the same logic in the line above. I think the patch in #45 would just be more efficient. The end result in both looks like this from what I can tell:

Committed the patch in #45. 98cddda and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.