Fragment links to children elements in closed grouping elements don't work

in firefox, the tab is opened - not in chrome

regarding the link,

it uses an anchor
As an example :
/node/1/edit#edit-path-0

One should add a jquery behaviour to open the tab from the anchor
I think we could use the toggle function available from collapse.js in core/misc

@andrewmacpherson rediscovered this issue. And the problem is still actual and wrongly closed as duplicate.

I manually tested this for detail elements. If they are closed the quick links don't work. So this is probably a problem for all grouping elements that can be closed (with JS).
Styling for grouping elements with errors should be a separate issue I think, because it has nothing to do with IFE. Without IFE closed grouping elements are already not showing that they have 'problems inside'.
Changing the title accordingly.

Also marking this issue as 'needs product manager review' because the impact on IFE in core needs to be assessed.
Summary needs an update as well.

Created another related issue for indicating a grouping element has children errors: see #2848507: Indicate that grouping elements have child element errors for ux and a11y.

Thanks @dmsmidt. Based on @andrewmacpherson's description, I think that this is needed to meet the accessibility gate, and it also has usability impact. So I would consider this a "Must" have. Since it can be fixed within the module itself, this can go in any time during the alpha or beta phases. I'll update the roadmap accordingly.

(I'm not a product manager but I think this is more release manager territory since it's about a core gate.)

Tag clean up: "accessibility" is the preferred one. The "a11y" tag doesn't have many issues so I'm moving them all to "accessibility".

I'm trying to track this issue down related to the vertical tabs, and I'm looking for some leads as to where to reproduce this with existing code. Can anyone help? Thanks.

FYI there's Drupal\system\Tests\Form\ElementsVerticalTabsTest which might be useful to build upon.

Please note that we should fix all grouping elements, hopefully in a generic way.

I'm thinking of a JS Patch that does something like:
- Listen to URL hash changes
- On hash change:
-- Find targeted field
-- Find all closed parent of that field grouping elements (we can potentially have fieldsets in fieldsets)
-- Open all closed parent grouping elements
-- Jump to target field that is now visible

Anyone wants to try this approach?
There are some examples of working with hash changes and jumping to fields in the final #2729663: Fragment link pointing to <textarea> should be redirected to CKEditor instance when CKEditor replaced that textarea patch.

IS updated

Making a start on screenshots and gifs...

I'm not sure how to identify "All affected grouping elements" though - is there a list somewhere? Should I start one?

In the meantime, here's a gif for the URL Path setting in Article.

When I try to reproduce #15, the details with the errored URL path are open and the error is displaying in Chrome 56.0. Though, once you close it, you can no longer open it with the link in the error message.

Seems like it will need to be a patch for a core js that opens parents of a referenced child element. Would be nice to have for use even besides the IFE. Which is the js file we are talking about?

Thanks for those gifs.
Concerning the gif in #15/#16, that has been fixed in core recently, jay!
So just manually close the details element and then click the link. The links shouldn't break after manually closing an element.

Grouping elements:
- Details
- Vertical Tabs

Fieldsets are not collapsible anymore (replaced by Details).

We can enhance both:
- core/misc/vertical-tabs.js
- core/misc/collapse.js

The fix should be independent from IFE, a hash change targeting a field that is not visible should become visible.

I have looked closer at the code and the form. What I see in this specific case is that the url hash seems to be wrong it's edit-path-0. The path module raises error on form item edit-path-0. However, that element itself does not get rendered. What gets rendered is it's child with id edit-path-0-alias. Thus the url hash is not referencing any element. Am I missing something?

To reference the element we need to have the actual error element rendered. Then, we would need to create a patch for Path and possible other modules that set error on element parents. Another option is to add a reference-able wrapper in IFE itself.

@alexrayu, that sounds like a new/different issue. Could you create a new issue for that and write down the steps to reproduce it?
Let's continue with items that do work now.

Created an issue at #2851786: Path validation sets error on wrong element to address the path part. This issue can be blocking the current issue, because unless it is resolved, IFE won't work with Path.

I have looked at this and played with js code some.

So, if I detect a hash change, it will open the closed summary just once. If I then close that summary and click it again, it won't open it any more (because the hash is already there). Is it better to detect the links that point to the current page with hash tags, and add listeners to them, rather than react on hash changes? (This seems to be more consistent).

So this is the code that works for details. Tell me if this is a good idea.

  /**
   * Set all hash links that target details to expand them.
   *
   * @type {Drupal~behavior}
   *
   * @prop {Drupal~behaviorAttach} attach
   *   Attaches behavior for the details element.
   */
  Drupal.behaviors.watchHashLinks = {
    attach: function (context) {
      $(context).on('click', $('a[href^="#"]'), function(event) {
        var $item = $(event.target.hash);
        if (event.target.hash != '#' && $item.length) {
          $item.parents('details').attr('open', true).find('summary').attr({
            'aria-expanded': true,
            'aria-pressed': true
          });
        }
      });
    }
  };

Need to know if I am moving in right direction, or if you still prefer to have hashchange event.

Attaching a proof of concept patch that works with details and vertical tabs (attaching gif for expected behavior).

Since vertical tabs and details are handled differently, I had to put the code in one place, rather than splitting it.

The path error part presupposes #2851786: Path validation sets error on wrong element.

Some clean up.

Correct comment.

I did some manual testing of patch #27, for details + vertical tabs elements. Works as intended...

To test links pointing inside a details element, I created a node, then went to node/1/edit and deleted the authored-on time, and attempted to save the node. I did this with three core themes:

• Stark theme, and narrow breakpoint. Authoring information is a details element, not a vertical tab.
• Seven theme. Authoring information is a details element no matter what breakpoint.
• Bartik theme, and narrow breakpoint. Authoring information is a details element, not a vertical tab.

For each theme, I tested that the link worked whether the details element was open or close. Normally, details elements with errors are automatically open on page load, but if you deliberately close the authoring info details, then use the error link, it opens again.

For testing vertical tabs, I used the Stark and Bartik themes, with wide viewport. On the node edit form, I attempted to save the node with a blank authored-on time. Before saving I made the "Promotion options" tab active. When the node form was returned with errors, the promotion-options was still the active vertical tab (expected), and clicking the error link opened the authoring-information tab to reveal the errors.

Note: With keyboard-only behaviour it was necessary to press TAB one more time after following the error link. This is because the error link points at ID edit-created-0-value which is a div wrapper to combine the date and time fields. This is expected behaviour where the link points at an ID on a div, but it would be better if the error link pointed directly at a text input. This probably deserves a follow-up issue similar to #2851786: Path validation sets error on wrong element and I would NOT consider it a blocker for this issue.

Thanks for all that great work and testing.

So listening to a hash change is not enough. But can we skip it altogether?
A good test would be a ckeditor enabled field in a details element.
I'm also not 100% convinced of mixing vertical tabs and details logic in the JS file for details.
But I currently don't see any other option without duplication logic or making things unnecessarily complex.
We need a JS maintainers feedback on this I guess.
At least we need to document which cases we support.

Furthermore, some extra feedback:

1. +++ b/core/misc/collapse.js
   @@ -140,6 +140,32 @@
   +   * Expand grouped elements if a contained child gets focused with a hash link.
   

   This description doesn't seem totally correct. The grouped elements don't expand, the grouping element is expanded.
   But in case of vertical tabs it is not really expanding.

   Maybe something like: "Reveal all children of a grouping element when a child gets focus."

2. +++ b/core/misc/collapse.js
   @@ -140,6 +140,32 @@
   +   *   Attaches behavior for the details element.
   ...
   +      $(context).on('click', $('a[href^="#"]').not('.vertical-tabs__menu-item.is-selected a'), function(event) {
   

   I think $('a[href^="#"]', context) is a more common pattern, right?

   We also need a once('behavior-name') before binding the event listener.

   Can we get rid of .not('.vertical-tabs__menu-item.is-selected a')? We can check that later right?

3. +++ b/core/misc/collapse.js
   @@ -140,6 +140,32 @@
   +  Drupal.behaviors.watchHashLinks = {
   

   This behavior name should describe what the purpose is, "watchHaslLinks" is too generic.

4. +++ b/core/misc/collapse.js
   @@ -140,6 +140,32 @@
   +        if (event.target.hash != '#' && $item.length) {
   +          $item.parents('details').attr('open', true).find('> summary').attr({
   

   Testing for $item.length is enough right?

5. +++ b/core/misc/collapse.js
   @@ -140,6 +140,32 @@
   +          $item.parents('details').attr('open', true).find('> summary').attr({
   +            'aria-expanded': true,
   +            'aria-pressed': true
   +          });
   

   Are these aria attributes really in the scope of this patch. If so we should document why.

Thanks for reviewing and testing! Applied the changes.

Aria attributes are added in detailsAria behavior. The idea why I added them is to keep the expanded details identical to when you click on them. Should I remove the aria tags from this patch?

Typo fix and article missing correction.

js tag so it's in the right queue :)

$(context).once('revealChildrenOfGroupedElements').on('click', $('a[href^="#"]', context), function(event) {

Can be simplified to

$(context).once('revealChildrenOfGroupedElements').on('click', 'a[href^="#"]', function(event) {

It's almost never .parents() that should be used. It's almost always closest()

Having a .focus in a loop seems wrong, you can only have 1 element focused at the time.

Having a .focus in a loop seems wrong, you can only have 1 element focused at the time.

+1

@alexrayu, thanks for the changes. Could you also provide interdiffs when you create a new patch? Makes it easier to review the changes.

@nod_ what do you think about the file this code should reside in?

@andrewmacpherson could you answer the question in #30?

Edit from #32

Thanks @tameeshb.

Patch from #34 does exactly what @nod_ suggested in #32.

Providing an interdiff for completeness.

Re: #30 and #32:

Aria attributes are added in detailsAria behavior. The idea why I added them is to keep the expanded details identical to when you click on them. Should I remove the aria tags from this patch?

We're talking about this part:

+          // Keep aria attributes in sync as per detailsAria behavior.
+          $item.parents('details').attr('open', true).find('> summary').attr({
+            'aria-expanded': true,
+            'aria-pressed': true
+          });

Yes, let's remove this. I would prefer not to have duplicate logic for the ARIA states, because we'd have a maintenance burden to keep them in sync. Remember that ARIA attributes are a bridging technology intended to address situations where native markup and browsers don't provide these hints to assistive technology. Our collapse.js and details-aria.js are a polyfill-type approach, so it's something we might want to tweak (or remove entirely) later on.

Instead of setting attributes directly (open, aria-expanded, aria-pressed) I think we should:

1. test whether the details element is already open vs. closed, (i.e. the <details open> attribute)
2. then, if necessary, open the details element by firing toggle() or onLegendClick() from collapse.js, so the ARIA states will be handled by existing code.

JS experts - I'm assuming that's easy enough?

I'm working on a functional JS test.

Actually this is a bug that becomes apparent with IFE enabled, but it is a generic bug. Not sure to which component the bug should belong.

Here is a first test only patch (should fail) for fragment links which point to elements inside inactive (hidden) vertical tabs.
Todo:
- The same for detail elements.
- Add hash change test (I still think we should make it work for hash changes as well)

Setting back to needs review, because the test SHOULD fail, and it does! :-)

Great thank you! The Each() was there because the idea is that there can be a details element inside another details element. I realize that would be rare but not impossible to think of. So it would cycle through all parents and open/select them all to show the actual element.

Working on some improvements.

Part of this issue has been solved with this patch I think: #2493957: Add back errors support to native HTML5 details tag

@droplet, actually I think that issue is already resolved by #2346773: Details form element should open when there are errors on child elements. It doesn't solve the problem of this issue.

Another half should address on this issue first: #2752511: Update location.hash when clicking a vertical tab

I'd suggest to do it in a very diff way.

1. Adding the JS to inline module, not collapse.js or detail-*.js.
2. Adding a click listener to error links. I expected these error link with .error-link-class or something similar.
3. the click listener trigger an error event, eg. "inline-form-error-link-clicked"
4. each module listening on above error event and do their own things in that module JS.

Pros:

- For example, if vertical tab doesn't want to expose an event listener on hash changes. It can listen to the event and trigger internal private function to deal with it.

- If yon don't like the default way in CORE. It's more easily to hack it and turn off.

@droplet, we really don't want to focus on error links here. The Inline Form Error links just showed the problem, but we want a more generic solution. I'm already working on a modified version with event handlers you can remove if you want.

Here is a big rewrite that covers more cases, incorporates I hope all feedback and simplifies the actual working code (no interdiff).
It opens details and tab panes on a fragment link click and a hash change.

We still need a functional JS test for details elements, I can work on that tomorrow.
Manual testing and a code review would be appreciated.

As promised here is the complete patch including the extra details element tests.

This is actually pretty sweet behavior we created :-)

For the interested watcher some simple screenshots made during the tests. The details test, tests that the child field becomes visible after a fragment change or click, even when nested in multiple details.

Not visible:

Visible:

Added before after gifs to summary.

A shoot, forgot to remove debugging screenshot.

The big rewrite from #50 onwards fires the detailsAria handler, so we no longer have duplicated ARIA logic (satisfies #36).

The functional JS tests make sense. I haven't run them locally, but I read them to see what the approach is, and the automatic tests are green.

I tested this manually. I installed the Standard profile, enabled the form_test module, then added a custom block with a list of fragment links (#edit-element and #edit-element-2 from the form_test pages, and #edit-keys from the core search block which is enabled in the standard profile).

The vertical tabs fragment links behaved as expected with mouse and keyboard. There is a difference between Firefox 52 and Chrome 57 though.

• In Chrome 57 the textfields are focused and can be typed in (you get the blinking text caret).
• In Firefox 52 the textfields don't get the blinking caret or allow you to type in it. However pressing tab (or shift+tab) does move you to the next interactive element after (or before) the textfield, so it's as if the focus has at least moved to the right place in the DOM. This is a bit weird, but importantly this difference between browsers behaviour is the same before and after the patch in #54. You don't see many fragment links to text fields in the wild, so maybe this is a behaviour I just haven't noticed before. In any case, since the browser inconsistency is the same before and after the patch, I don't think it is a blocker here. It might turn out to be a known Firefox bug.

I noticed some odd inconsistencies between various Blink-based browsers:

• In Chrome 57: the behaviour of the nested details elements worked as expected. With the root, group, or both details elements collapsed, clicking the #edit-element fragment link revealed the text field and focussed it with a blinking cursor. Good.
• In Opera 43, Chromium 56, and Vivaldi 1.7: the textfield would be revealed if just one of the nested <details> was closed, but if both were closed then clicking the fragment link would only open the outermost ("root") details element. Watching the attributes in the HTML tree inspector, I could see the aria attributes correctly changed from false to true for both summary elements, but only the outermost <details> element gained an open attribute. Opera is a browser we support, so this seems like a problem! I'm marlking the issue needs work because of this.

FWIW, I've been testing with the Linux versions of these browsers. I haven't tested with Safari, Edge or IE yet.

Using ChromeVox with Chrome57, the behaviour works as expected. I had to use the CVOX + Space method of activating the links, but once I figured that out it worked fine. I haven't tested this with any other screen readers yet.

EDIT: apparently I have Chrome 57 and Chromium 56 on my machine (not Chromium 57). Updated the version number above.

Great testing again! So we have work to do for Opera. I've unassigned myself so that others can also work on this.

Let's see if we can easily make FireFox put the carrot in the field, but this is no hard requirement, since our main goal is to reveal the problematic field.

@andrewmacpherson, I can't reproduce the problem in Opera you describe.
I updated my github test module with nested details elements and they seem to open nicely in Opera (44) when I click a fragment link to the deepest nested item.

Could you provide step by step instructions on how to reproduce the problem?

+++ b/core/misc/collapse.js
@@ -140,6 +140,32 @@
+      $item.parents('details').not('[open]').find('> summary').trigger('click.detailsAria');

Wouldn't it be more logical to set aria after the actual loop? If there is an error and the loop does not work for some reason, the aria will be set anyway.

@alexrayu, to which loop do you refer? That click is not in a loop.
But, maybe we could just trigger a normal click instead of a click.detailsAria to fix the Opera issue (I can't reproduce).

Ok, this shouldn't be hard to re-roll but what happened to core/tests/Drupal/FunctionalJavascriptTests/Core/Form ? Should that directory exist?

In core/misc/collapse.js it looks like somehow this comment was removed (which threw off this patch):
"// Expose constructor in the public space."

That should be pretty straight forward.

@mgifford, sadly it was a bit more work since we now use ES6 in core.
So I did a manual re-roll and converted the code to ES6, and improved code documentation.

(And yes I know there is no newline at the end of collapse.js, but this is how it is transpiled)

I just did a whole bunch of testing in 6 different browsers using Browserstack.

In each test, I was logged in, and using the admin theme. I tried a link to open nested details (which I kept having to close before clicking the link, because they were open automatically on page load if a form error was inside) and a link to open a vertical tab that wasn't currently open. I was always testing with a required textfield that was submitted empty.

Unless noted otherwise, when I say "work fine" I mean that the details were opened or the vertical tab shown, the page jumped down to the control and the carrot appeared in the textfield.

• Firefox 54 / MacOS Sierra: worked fine, except I also saw the issue described above (in #55) where the carrot didn't appear in the textfield
• Safari 10.1 / MasOS Sierra: first time I tried, it expanded the details, but didn't move the page down or put the carrot in the textfield. Then, I tried again, and it worked fine! Not sure if that was just a fluke or an actual problem with Safari...
• Chrome 59 / MacOS Sierra: worked fine!
• Opera 46 / MacOS Sierra: worked fine! I also did not see the Opera issue described above in #55 which @dmsmidt couldn't reproduce in #59 either
• IE 11 / Windows 10: with the nested details, clicking the link changed the URL (hash was added) but the details weren't expand, the page didn't move, carrot didn't appear, etc. However, vertical tabs worked fine! There were no errors in the JS console
• Edge 15 / Windows 10: same results as IE 11

So, all I really found was that IE and Edge aren't opening closed details - otherwise everything seems to work as expected (the Firefox issue as noted above is not new to this issue). Marking "Needs work" for the IE/Edge issue.

IE 11 is ES6 incompatible. If we are using ES6 officially we should ditch IE 11.

IE 11 is ES6 incompatible. If we are using ES6 officially we should ditch IE 11.

We're transpiling to ES5 currently, so it should work with IE11! I don't think dropping IE11 is really a concern until we're talking about dropping the transpiling to ES5 (which I don't think we're talking about yet?)

I don't really have a solution, but I did some experimentation with IE and Edge, and this change fixes it completely on Edge and partially on IE11:

diff --git a/core/misc/collapse.es6.js b/core/misc/collapse.es6.js
index da1c6a4..e85c028 100644
--- a/core/misc/collapse.es6.js
+++ b/core/misc/collapse.es6.js
@@ -156,7 +156,7 @@
     if ($item.length) {
       // Open all (nested) parent details and set aria attributes on the summary
       // by triggering the click event listener in details-aria.js.
-      $item.parents('details').not('[open]').find('> summary').trigger('click.detailsAria');
+      $item.parents('details').not('[open]').find('> summary').trigger('click');
 
       // Loop over all (nested) parent vertical tabs and focus them in order
       // to reveal the children.

For some reason, they don't like the '.detailsAria' part of .trigger('click.detailsAria')? Would it be OK to remove that?

However, this change doesn't fix it 100% on IE11 -- it'll expand the details but won't jump the page down to the control. If I click the link again, it'll jump down to the control.

@dmsmidt I finally got around to looking at my results form #55 again.

I tried to replicate the problem I found with nested details, and couldn't replicate it. Back then, Chrome 57 worked fine for me, but Chromium 56 and the other Blink browsers didn't. Maybe there was a change between 56 and 57? Whatever the case, our patch has moved on a bit, and all these browser versions have gone up a few numbers.

I experienced the problem with Opera 43, and you couldn't replicate it with Opera 44 (the release date was 21 March). I wonder if some change was being rolled out across the Blink browser releases around that time?

Anyway, that inconsistency I found between Blink browsers seems to have gone :-)

oops, unassigned @finne accidentally, can't add them back

NP :-)

I solved the problems listed in #72. The expand, scroll down and focus now all work in both vertical tabs and sidebar detail elements. This has been tested in Chrome, Edge and IE11.

The underlying issues are several:
- on click of a anchor the browser scrolls the viewport, and sets focus. This is not javascript, and cannot easily be tested. In Edge and IE the timing of the onClick/onHashchange event in relation to the browser scroll/focus is unclear, but it seems that it is before the JS events. So for now the focus is done afterwards with a setTimeout.
- the onClick event is triggered in JS, and before we used a namespaced trigger event. These are not working in IE11 and Edge. Also, why would we simulate a named click event in JS, when you could never fire a named click using the mouse. I changed this to a generic click event.

Thanks @finne. This is an improvement. Back in #55 and #69 we noticed that Firefox wasn't showing the caret.

Now Firefox 54/linux is showing the caret focus in the textfield as expected. Earlier today I was also playing with $item.focus(), but didn't think about using a timeout.

Leaving at needs review, because I'm tired after #a11y-sprinting all day. Let's have another pair of eyes on it during the week?

Great work @finne, @dsnopek and @andrewmacpherson. Since the timing of opening details elements and focus is out of our hands I think the solution is neat.

Maybe @dsnopek can give it a final look so that we can finish this interesting issue!

Sure thing! I redid my testing from #69, and here's what I found:

• Firefox 54 / MacOS Sierra: worked fine, and even fixed the issue with the carrot not appearing in the textfield (originally from #55)
• Safari 10.1 / MasOS Sierra: worked fine, and I didn't experience the issue I saw once where the details expanded but the page didn't jump down. Thinking about it after seeing the solution to the IE/Edge problem, this might have been the same race condition between the browser focusing the element and showing it, so the latest patch probably fixes that too :-)
• Chrome 59 / MacOS Sierra: worked fine
• Opera 46 / MacOS Sierra: worked fine
• IE 11 / Windows 10: worked fine!
• Edge 15 / Windows 10: worked fine!

So, testing looks perfect!

Going backup through the comments I don't see any other unaddressed review with regard to the code or approach, so I'm going to go out on a limb and mark as RTBC. :-) But feel free to mark back to 'Needs review' if anyone thinks this needs more review first

Put on collapse.js just seems not right to me (#48). Want to hear feedback from @nod_ or @drpal

Here's a quick re-roll of the patch in #77. There are no actual code changes, just some offsets that caused the patch not to apply in the testbot.

If it doesn't go in collapse.js, then it still needs to somewhere in the core libraries. The ideas in #48 assumed it was only relevant for IFE, which @dmsmidt already answered in #49.

We might want to remove collapse.js one day, but we'd still want to open details for fragment links like IFE. (Native support for summary/details is now under development for MS Edge leaving IE11 as the only browser we need to support with a polyfill.)

1. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,47 @@
   +  const showGroupingElementChild = function (e) {
   

   Arrow function.

2. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,47 @@
   +    const $item = (e.type === 'click') ? $(e.currentTarget.hash) : $(`#${location.hash.substr(1)}`);
   

   Don't need to wrap the condition in ( )

3. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,47 @@
   +      // Open all (nested) parent details and set aria attributes on the summary
   

   Wrong format for multiline comments.

4. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,47 @@
   +      // Clicking the anchor should set focus, but timing issues in Edge need this.
   

   80 characters. Can we also create a followup for why this is happening?

5. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,47 @@
   +      setTimeout(function() {
   ...
   +   * Binds a listener on fragment links to handle clicks.
   

   Arrow function.

6. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,47 @@
   +   * Binds a listener on fragment links to handle clicks.
   

   Use single line comment format.

7. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,47 @@
   +   * Binds a listener to handle location hash changes.
   

   Use single line comment format.

I don't really have an issue with this being in collapse.es6.js. @droplet, do you have a suggestion on where this could go?

Updated patch for code review in #86.

JS subsystem maintainer feedback has been provided by @drpal and is addressed by @cboyden.

I just made a follow up for the setTimeout() for Egde: #2894958: Get rid of setTimeout() in collapse.js.

Since @drpal and others don't know of a better location for the code introduced than collapse.js I'm RTBC-ing this again.
I propose that if we find a better spot someday we can just move the code. Let's not forget this is a major blocking issue for IFE.

1. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,50 @@
   +      $item.parents('.vertical-tabs__pane').each((index, pane) => {
   +        $(pane).data('verticalTab').focus();
   +      });
   

   this part should put inside the vertical-tabs.es6.js IMO.

   1. We have a very clear logic code. Collapse only does Details tag things.
   2. Not every page has vertical-tabs.es6. On another side, not every page has collapse (check core.libraries.yml)
   3. I still suggested broadcasting an event. This is a generic solution, not designed for inline form, we don't know how things work inside collapse. For example, items may only initial when the collapse is opened
   4. it may not needed sometimes....

     Drupal.behaviors.verticalTabs = {
       attach(context) {
         const width = drupalSettings.widthBreakpoint || 640;
         const mq = `(max-width: ${width}px)`;
   
         if (window.matchMedia(mq).matches) {
           return;
         }

2. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,50 @@
   +  $(document).on('click.grouping-element-fragments', 'a[href^="#"]', showGroupingElementChild);
   

   Missing absolute URL

1. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,50 @@
   +        $item.focus()
   

   Missing `;`

2. +++ b/core/misc/collapse.js
   @@ -77,5 +77,25 @@
   +  $(document).on('click.grouping-element-fragments', 'a[href^="#"]', showGroupingElementChild);
   ...
   +  $(window).on('hashchange.grouping-element-fragments', showGroupingElementChild);
   

   It can drop click event. `hashchange` will always trigger.

   If not, we have to denounce it, not calling the showGroupingElementChild twice.

Thanks @droplet you have some valid points.

We can't drop the click event, because the hashchange is not triggered multiple times. So if someone triggers an hashchange, then closes a grouping element, and then clicks on the fragment link again, the hashchange is not triggered.

This is a pretty big rewrite in order to deal with @droplet's points. Please review carefully.
Details and vertical tabs functionality are now neatly split per file.
The generic event needed is placed in form.js, this file is a dependency for both.
Using debounce we now make sure we only process the interaction once.

Just rethought bit of the naming to making things more clear.

Ah, some timing issues due to the added debounce.
Updated tests.

Woop! Green again.
Adding some additional textual improvements.

Thanks

1. +++ b/core/misc/collapse.es6.js
   @@ -150,14 +150,14 @@
   +  const handleFragmentLinkClickOrHashChange = function openDetailsOnFragmentLinkClickOrHashChange(e, $target) {
   

   use function declaration?

2. +++ b/core/misc/form.es6.js
   @@ -279,7 +279,7 @@
   +  $(window).on('hashchange.form-fragment', debounce(handleFragmentLinkClickOrHashChange, 20));
   

   we can increasing the value a bit and passing true as 3rd arg to execute first call immediately

3. +++ b/core/misc/form.es6.js
   @@ -245,4 +255,38 @@
   +  $(document).on('click.form-fragment', 'a[href*="#"]', debounce(handleFragmentInteraction, 20));
   

   hmm. I think we can remove debounce for click event. and may be trigger `hashchange` event instead of a custom function `handleFragmentInteraction`?

Or even don't need to debounce them at all with `event.preventDefault()` on click?

1. +++ b/core/misc/collapse.es6.js
   @@ -137,6 +137,28 @@
   +  const handleFragmentLinkClickOrHashChange = function openDetailsOnFragmentLinkClickOrHashChange(e, $target) {
   

   `() =>`

2. +++ b/core/misc/form.es6.js
   @@ -245,4 +255,38 @@
   +  const handleFragmentLinkClickOrHashChange = function triggerFragmentLinkClickOrChange(e) {
   

   `() =>`

3. +++ b/core/misc/form.es6.js
   @@ -245,4 +255,38 @@
   +    const $target = e.type === 'click' ? (e.currentTarget.location ? $(e.currentTarget.location.hash) : $(e.currentTarget.hash)) : $(`#${location.hash.substr(1)}`);
   

   How do you feel about wrapping this to a new line? Something like?

   const $target = e.type === 'click' ? (e.currentTarget.location ?
     $(e.currentTarget.location.hash) :
     $(e.currentTarget.hash)) : $(`#${location.hash.substr(1)}`);
   

4. +++ b/core/misc/vertical-tabs.es6.js
   @@ -14,6 +14,23 @@
   +  const handleFragmentLinkClickOrHashChange = function showVerticalTabsPanesOnFragmentLinkClickOrHashChange(e, $target) {
   

   `() =>`

@droplet #99: 1 & @drpal #102: 1,2,4 have you seen https://github.com/airbnb/javascript#functions--declarations?

Don’t forget to name the expression - anonymous functions can make it harder to locate the problem in an Error’s call stack.

Aren't we following that?

@drpal #99-3: fixed

@droplet #99-2: fixed

#99-3, #100: Actually I'm quite happy with the two debounce calls to the same custom function. It nicely prevents multiple calls when both a click and hash change are triggered. Different browsers have different timings, like this we don't have to take that in account. Also if we trigger a hashchange on click we have to juggle more with the event data to be able to get the correct targeted node element. Now it is that handled clearly in one place. Could you write/show/test an alternative if you are really sure it can be much better implemented?

@droplet #99: 1 & @drpal #102: 1,2,4 have you seen https://github.com/airbnb/javascript#functions--declarations?

Yeah. read it. We accepted both.

It nicely prevents multiple calls when both a click and hash change are triggered.

OK. makes sense.

only one last point:

+++ b/core/misc/form.es6.js
@@ -245,4 +255,40 @@
+    const $target = e.type === 'click' ? (e.currentTarget.location ?
+      $(e.currentTarget.location.hash) : $(e.currentTarget.hash)) :
+      $(`#${location.hash.substr(1)}`);

Do you mind rewrite it into if-else?

@dmsmidt & @droplet

const handleFragmentLinkClickOrHashChange = (e, $target) => {
  $target.parents('.vertical-tabs__pane').each((index, pane) => {
    $(pane).data('verticalTab').focus();
  });
};

becomes,

var handleFragmentLinkClickOrHashChange = function handleFragmentLinkClickOrHashChange(e, $target) {
  $target.parents('.vertical-tabs__pane').each(function (index, pane) {
    $(pane).data('verticalTab').focus();
  });
};

I think we can use arrows in .es6.js.

+++ b/core/misc/collapse.es6.js
@@ -137,6 +137,28 @@
+  const handleFragmentLinkClickOrHashChange = function openDetailsOnFragmentLinkClickOrHashChange(e, $target) {

Also note, this line is technically too long, but that's a very minor nit.

Alright, discussed with @drpal, we go ahead with the arrow function. This also gets rid of the long sentence.

@dropel #104, fixed if/else structure

Sigh, editor didn't cleanup empty line. And I forgot one arrow funct. Sorry testbot ;-)
Hopefully everything is now gooood to go.

@dmsmidt

- Thanks for switching that nested ternary to an if/else.
- Looks like the arrow functions were transpiled as expected.

This looks good to me. 👍

Starting manual review.

RTBC++ from me too.

Updating issue credits:

• adding @droplet for numerous reviews that significantly shaped the patch
• adding @drpal for similar reasons
• adding @dsnopek for numerous reviews and manual testing on browserstack
• adding @nod_ for review that lead to improvements

Committed as bbc8743 and pushed to 8.4.x. Thanks everyone.

This is fantastic! Thanks to everyone for their hard work on this!

Yay! Great to see this fixed and with automated tests.

Unfortunately, the code/test added by this patch is now preventing us from updating to jQuery 3. If anyone here is able to help with #2898261: In jQuery 3, $('#') throws syntax errors, that would be wonderful. Thanks!