Problem/Motivation

Fragment links and fragment/hash changes pointing to elements inside closed or invisible grouping elements like vertical tabs and details don't work.

This is particularly problematic for Inline Form Errors. In case of an error in a hidden grouping element, the error summary links don't work anymore.

Proposed resolution

A JavaScript patch that does something like:
- Listen to URL fragment/hash changes
- Listen to fragment link clicks (in case the fragment of the URL stays the same)
- On fragment change/ link click:
-- Find targeted field
-- Find all closed parents of that field grouping elements (we can potentially have details in details)
-- Open/show all closed parent grouping elements
-- Jump to target field that is now visible and focus

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

Before

Details

Vertical tabs

After

Details

Vertical tabs

Remaining tasks

- Screenshots (but better gifs) that show the problem for all affected grouping elements (like fieldsets/details).
- Issue summary update based on latest comments
- JavaScript patch as suggested above.

Related issues

* #2395065: Certain URL fragments cause javascript error when collapsible fieldset is present on page
* #2848507: Indicate that grouping elements have child element errors for ux and a11y
* Parent issue: #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)

CommentFileSizeAuthor
#110 drupal-2531700-manualtest.gif646.56 KBkattekrab
#107 2531700-107-fragment_links_hidden_children.patch12.18 KBdmsmidt
#107 interdiff-2531700-106-107.txt894 bytesdmsmidt
#106 2531700-106-fragment_links_hidden_children.patch12.22 KBdmsmidt
#106 interdiff-2531700-103-106.txt3.44 KBdmsmidt
#103 2531700-103-fragment_links_hidden_children.patch12.22 KBdmsmidt
#103 interdiff-2531700-97-103.txt2.29 KBdmsmidt
#97 2531700-97-fragment_links_hidden_children.patch12.14 KBdmsmidt
#97 interdiff-2531700-96-97.txt6.95 KBdmsmidt
#96 2531700-96-fragment_links_hidden_children.patch11.9 KBdmsmidt
#96 interdiff-2531700-93-96.txt2.83 KBdmsmidt
#93 2531700-93-fragment_links_hidden_children.patch11.86 KBdmsmidt
#93 interdiff-2531700-92-93.txt4.83 KBdmsmidt
#92 2531700-92-fragment_links_hidden_children.patch11.8 KBdmsmidt
#92 interdiff-2531700-87-92.txt8.64 KBdmsmidt
#87 interdiff-2531700-83-87.txt2.15 KBcboyden
#87 2531700-87-fragment_links_hidden_children.patch7.71 KBcboyden
#83 2531700-83-fragment_links_hidden_children.patch7.67 KBcboyden
#77 interdiff-2531700-66-77.txt1.55 KBfinne
#77 2531700-77-fragment_links_hidden_children.patch7.65 KBfinne
#66 interdiff-2531700-55-66.txt3.5 KBdmsmidt
#66 2531700-66-fragment_links_hidden_children.patch7.42 KBdmsmidt
#54 interdiff-2531700-51-55.txt777 bytesdmsmidt
#54 2531700-55-fragment_links_hidden_children.patch6.31 KBdmsmidt
#52 details_fragment_click_after.gif262.44 KBdmsmidt
#52 details_fragment_click_before.gif182.58 KBdmsmidt
#52 tabs_fragment_click_after.gif329.24 KBdmsmidt
#52 tabs_fragment_click_before.gif250.58 KBdmsmidt
#51 child_visible.jpg20.47 KBdmsmidt
#51 child_not_visible.jpg10.96 KBdmsmidt
#51 interdiff-2531700-50-51.txt4.73 KBdmsmidt
#51 2531700-51-fragment_links_hidden_children.patch6.37 KBdmsmidt
#50 2531700-50-fragment_links_hidden_children.patch4.25 KBdmsmidt
#39 2531700-39-fragment_links_hidden_children-TEST_ONLY.patch2.09 KBdmsmidt
#35 interdiff-2531700-31-34.txt523 bytesandrewmacpherson
#34 2531700-34.patch1.17 KBtameeshb
#31 inline_form_errors-2531700-31.patch1.18 KBalexrayu
#30 inline_form_errors-2531700-30.patch1.17 KBalexrayu
#27 inline_form_errors-2531700-27.patch1.12 KBalexrayu
#26 inline_form_errors-2531700-26.patch1.1 KBalexrayu
#24 proof-concept.gif475.25 KBalexrayu
#24 2531700_hash-link-item-container-open_24.patch1.43 KBalexrayu
#16 2531700-16.gif794.23 KBalexrayu
#15 2531700-gif.gif597.27 KBkattekrab
2015-07-12_102523.png27.1 KBhass
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

hass’s picture

Title: Errors in vertical tabs not highlighted, jump link not working » Vertical tabs with errors not highlighted, jump link not working
bendev’s picture

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

hass’s picture

dmsmidt’s picture

Title: Vertical tabs with errors not highlighted, jump link not working » Inline form errors summary links don't work for closed grouping elements
Version: 8.0.x-dev » 8.4.x-dev
Component: forms system » inline_form_errors.module
Status: Closed (duplicate) » Active
Issue tags: +a11y, +Needs tests, +Needs product manager review, +Needs issue summary update

@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.

dmsmidt’s picture

xjm’s picture

Priority: Normal » Major
Issue tags: -Needs product manager review

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.)

dmsmidt’s picture

andrewmacpherson’s picture

Issue tags: -a11y +accessibility

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

smurrayatwork’s picture

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.

DamienMcKenna’s picture

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

dmsmidt’s picture

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.

kattekrab’s picture

Issue summary: View changes

IS updated

kattekrab’s picture

Issue summary: View changes
kattekrab’s picture

FileSize
597.27 KB

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.

Animated gif demonstrating hidden error for URL Path settings

alexrayu’s picture

FileSize
794.23 KB

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.

Reproduce attempt.

alexrayu’s picture

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?

dmsmidt’s picture

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.

alexrayu’s picture

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.

dmsmidt’s picture

@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.

alexrayu’s picture

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.

alexrayu’s picture

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).

alexrayu’s picture

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.

alexrayu’s picture

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.

Proof of concept.

alexrayu’s picture

Status: Active » Needs review
alexrayu’s picture

alexrayu’s picture

andrewmacpherson’s picture

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.

dmsmidt’s picture

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.

alexrayu’s picture

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?

alexrayu’s picture

Typo fix and article missing correction.

nod_’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript

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.

dmsmidt’s picture

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?

tameeshb’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

Edit from #32

andrewmacpherson’s picture

FileSize
523 bytes

Thanks @tameeshb.

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

Providing an interdiff for completeness.

andrewmacpherson’s picture

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?

andrewmacpherson’s picture

Status: Needs review » Needs work
dmsmidt’s picture

I'm working on a functional JS test.

dmsmidt’s picture

Title: Inline form errors summary links don't work for closed grouping elements » Fragment links to children elements in closed grouping elements don't work
Component: inline_form_errors.module » javascript
FileSize
2.09 KB

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)

dmsmidt’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: 2531700-39-fragment_links_hidden_children-TEST_ONLY.patch, failed testing.

kattekrab’s picture

Status: Needs work » Needs review

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

alexrayu’s picture

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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt

Working on some improvements.

droplet’s picture

dmsmidt’s picture

@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.

droplet’s picture

droplet’s picture

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.

dmsmidt’s picture

@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.

dmsmidt’s picture

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.

dmsmidt’s picture

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:

dmsmidt’s picture

Added before after gifs to summary.

Status: Needs review » Needs work

The last submitted patch, 51: 2531700-51-fragment_links_hidden_children.patch, failed testing.

dmsmidt’s picture

andrewmacpherson’s picture

Status: Needs review » Needs work

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.

dmsmidt’s picture

Assigned: dmsmidt » Unassigned

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.

dmsmidt’s picture

Issue tags: +DevDaysSeville
dmsmidt’s picture

Assigned: Unassigned » dmsmidt
Issue summary: View changes
dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs work » Postponed (maintainer needs more info)

@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?

alexrayu’s picture

+++ 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.

dmsmidt’s picture

@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).

dmsmidt’s picture

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

Status: Needs review » Needs work

The last submitted patch, 54: 2531700-55-fragment_links_hidden_children.patch, failed testing. View results

mgifford’s picture

Issue tags: +Needs reroll

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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
dmsmidt’s picture

Assigned: dmsmidt » Unassigned
Status: Needs work » Needs review
FileSize
7.42 KB
3.5 KB

@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)

Jo Fitzgerald’s picture

Issue tags: -Needs reroll
dmsmidt’s picture

Issue tags: +sprint
dsnopek’s picture

Status: Needs review » Needs work

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.

alexrayu’s picture

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

dsnopek’s picture

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?)

dsnopek’s picture

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.

finne’s picture

Assigned: Unassigned » finne
andrewmacpherson’s picture

Assigned: finne » Unassigned

@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 :-)

andrewmacpherson’s picture

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

finne’s picture

Assigned: Unassigned » finne

NP :-)

finne’s picture

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.

andrewmacpherson’s picture

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?

dmsmidt’s picture

Assigned: finne » Unassigned

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!

dsnopek’s picture

Status: Needs review » Reviewed & tested by the community

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

droplet’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 77: 2531700-77-fragment_links_hidden_children.patch, failed testing. View results

cboyden’s picture

Status: Needs work » Needs review
FileSize
7.67 KB

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.

andrewmacpherson’s picture

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.)

drpal’s picture

Assigned: Unassigned » drpal
drpal’s picture

Assigned: drpal » Unassigned
Status: Needs review » Needs work
  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?

cboyden’s picture

Updated patch for code review in #86.

dmsmidt’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

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.

droplet’s picture

Status: Reviewed & tested by the community » Needs review
  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.

dmsmidt’s picture

Status: Needs review » Needs work

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.

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
dmsmidt’s picture

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.

dmsmidt’s picture

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

The last submitted patch, 92: 2531700-92-fragment_links_hidden_children.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 93: 2531700-93-fragment_links_hidden_children.patch, failed testing. View results

dmsmidt’s picture

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

dmsmidt’s picture

Woop! Green again.
Adding some additional textual improvements.

Status: Needs review » Needs work

The last submitted patch, 97: 2531700-97-fragment_links_hidden_children.patch, failed testing. View results

droplet’s picture

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`?

droplet’s picture

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

dmsmidt’s picture

Assigned: Unassigned » dmsmidt
drpal’s picture

  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) {
    

    `() =>`

dmsmidt’s picture

@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’s picture

@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?

drpal’s picture

@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.

dmsmidt’s picture

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

dmsmidt’s picture

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

drpal’s picture

Status: Needs review » Reviewed & tested by the community

@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. 👍

kattekrab’s picture

Assigned: Unassigned » kattekrab

Starting manual review.

kattekrab’s picture

Issue summary: View changes
FileSize
646.56 KB

RTBC++ from me too.

animated gif demonstrating successful manual test

kattekrab’s picture

Assigned: kattekrab » Unassigned
larowlan’s picture

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

  • larowlan committed bbc8743 on 8.4.x
    Issue #2531700 by dmsmidt, alexrayu, cboyden, finne, tameeshb, kattekrab...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

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

rootwork’s picture

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

effulgentsia’s picture

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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.