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)

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