Accessibility bugs with vertical tabs

Thanks for filing this @id.aleks

Drupal's vertical tabs started out in D6 contrib, and hasn't changed a great deal since. I think we should actually replace it with an implementation of the WAI-ARIA tabs pattern, which has good support now. That's a bigger plan forming at #2893640: Modernize ARIA usage, in line with ARIA 1.1 and the ARIA Authoring Practices guide..

The fact that our vertical tabs is a special case of a responsive wrapper around details groups makes this a bit awkward.

The individual issues you describe here are things we could address separately in the meantime. Maybe we'll split this into separate issues.

@huzooka Yep, I'm working on it.

This patch covers the four reported bugs. It may ultimately be necessary to split these into individual issues, but they're pretty easy to isolate if that need arises. Going to try the 4-in-1 for now since this is already a child issue.

Item 1:

Drupal.verticalTab doesn't take care of the right aria attributes. Whatever happens, every details summary element (of a single VerticalTabs) are described with aria-expanded="false" and aria-pressed="false".

The two changes that fix this are in vertical-tabs.es6.js and are both accompanied by this comment:

+          // The aria attributes for <summary> must be explicitly set. This is
+          // typically acheived via logic in details-aria.js, but will not work
+          // with vertical tabs as open/closed state is not triggered via the
+          // <summary> element.

Item 2:

Drupal.verticalTab marks active vertical tab menu items with an element that text is active tab. The element has a non-unique CSS id #active-vertical-tab.....

Changed this to use a data-attribute and make the scope more precise instead of targeting the whole page

-      $('#active-vertical-tab').remove();
+      this.item
+        .siblings()
+        .find('[data-drupal-active-vertical-tab]')
+        .remove();
       this.link.append(
-        `<span id="active-vertical-tab" class="visually-hidden">${Drupal.t(
+        `<span data-drupal-active-vertical-tab class="visually-hidden">${Drupal.t(
           '(active tab)',
         )}</span>`,
       );

Item 3:

Auto-focus bug: When a vertical tab is activated (triggered to be shown) by pressing Enter on the tab menu link Drupal.verticalTab tries to focus the first visible :input element in a vertical tab content. But the implementation is wrong...

Made the scope more precise instead of targeting the whole page

 // Set focus on the first input field of the visible details/tab pane.
-        $('.vertical-tabs__pane :input:visible:enabled')
+        self.details
+          .find(':input:visible:enabled')
           .eq(0)
           .trigger('focus');

Item 4:

The label of a vertical tabs element has invalid for attribute value...

The solution is largely explained in the additions to VerticalTabs.php

+    // Despite being in a `form_element` theme wrapper, vertical tabs themselves
+    // are not focusable, they are a container for other form elements. A few
+    // attributes must be changed to make this distinction apparent to
+    // assistive technology.
+    // Define the vertical tabs container as a group of related form elements.
+    $element['#attributes']['role'] = 'group';
+
+    // Since vertical tabs aren't focusable, they don't need an ID. Move this ID
+    // to the label, so it can be referenced by aria-labelledby.
+    $element['#label_attributes']['id'] = $element['#id'];
+    $element['#attributes']['aria-labelledby'] = $element['#label_attributes']['id'];
+
+    // Remove the id from the element itself so Drupal will not add a `for`
+    // attribute to the label.
+    unset($element['#id']);

The solution also includes some changes to the initialization in vertical-tabs.es6.js when the group must be expanded to include newly added tabs.

// Move the group role and its label association  to the new wrapper
+          // that is added as part of tab column creation.
+          const labelledby = $this.attr('aria-labelledby');
+          $this.removeAttr('aria-labelledby');
+          $this.removeAttr('role');
+
           // Create the tab column.
           const tabList = $('<ul class="vertical-tabs__menu"></ul>');
           $this
-            .wrap('<div class="vertical-tabs clearfix"></div>')
+            .wrap(
+              `<div class="vertical-tabs clearfix" role="group" aria-labelledby="${labelledby}"></div>`,
+            )
             .before(tabList);

The aria attributes are changed on the summary element, but this summary element is in "display:none", is it useful then? shouldn't the aria labels be on the <li>?

The aria attributes are changed on the summary element, but this summary element is in "display:none", is it useful then? shouldn't the aria labels be on the

?

You're right. I've changed that item in the issue summary to a more generic "Drupal.verticalTab doesn't properly communicate its functionality to assistive technology."

To address this, I refactored this to as an aria tablist/tabpanel, using the modified approach described in https://www.deque.com/blog/a11y-support-series-part-1-aria-tab-panel-acc... which permits tabs to be accessed via tab navigation.

This indirectly solves item 2 as well. Adding "active tab" to the tab text is not necessary when the tablist has the correct aria attributes, so this has been removed entirely.

Adding "Needs accessibility review" since this has been refactored to follow WAI-ARIA tab panel practices (with slight changes suggested by the Deque article in #14)

Reviewed the patch and found that the aria attribute and id thing working.

Revisited #14 with a screenreader and it isn't quite doing what it ought to be doing. Fixing that now.

I reviewed this against some examples again:
https://www.w3.org/TR/wai-aria-practices-1.1/examples/tabs/tabs-1/tabs.html
and
https://www.deque.com/blog/a11y-support-series-part-1-aria-tab-panel-acc... (which deviates from the pattern a bit to allow tabs to be reached via the tab key, something implemented here as well).

Some of the changes needed to get this working like the examples:

• Removing the refocus behavior when selecting a tab. In the pattern the tab remains the focused item. This included removing the href attribute from the links to prevent handleFragmentLinkClickOrHashChange from responding to hash events.
• Had to explicity added tabindex=0 to the tab links since href was removed to stop the hash/focus behavior.
• Adding the tab ids to an aria-owns attribute in the tablist, otherwise voiceover would see each tab as "tab 1 of 1", instead of part of a list.
• Added tabindex=0 to tab panels so they are focusable. This then required a few changes to vertical tab CSS so the focus outline is positioned correctly.

Here's a recording of this patch in use with voiceover.
https://youtu.be/qLTikOfosYg

Had to change a few tests since the href is removed from the "tabs" to prevent issues related to the hashchange event. The clickLink() testing method requires an href attribute, not just an <a>.
Note that yhe <a> "tabs" should actually be <button> elements, but I believe that would be a BC break.

Just a few points in the comments.

1. +++ b/core/lib/Drupal/Core/Render/Element/VerticalTabs.php
   @@ -137,6 +137,24 @@ public static function processVerticalTabs(&$element, FormStateInterface $form_s
   +    // Despite being in a `form_element` theme wrapper, vertical tabs themselves
   

   Should be a single quite "'" and not a back tick.

2. +++ b/core/lib/Drupal/Core/Render/Element/VerticalTabs.php
   @@ -137,6 +137,24 @@ public static function processVerticalTabs(&$element, FormStateInterface $form_s
   +    // Remove the id from the element itself so the Form API does not add a `for`
   

   > 80 chars and back ticks instead of single quotes.

3. +++ b/core/misc/vertical-tabs.es6.js
   @@ -80,8 +51,24 @@
   +          // Remove the `role="group"` from this container as these elements will
   

   > 80 chars and back ticks instead of single quote.

4. +++ b/core/misc/vertical-tabs.js
   @@ -127,7 +132,7 @@
   \ No newline at end of file
   

Maybe there is a reason for back ticks instead of single quotes?

Backticks are to output code tags when the doc is generated (or displayed in PhpStorm for example). See the wildly outdated: https://read.theodoreb.net/drupal-jsapi/Drupal.html#.attachBehaviors

In this case they are all used in line comments, so they won't ever get displayed in the generated JSDoc. I'd keep them for consistency with the rest of the comments though.

Regarding the backtick feedback in #24, I've been using backticks as an equivalent to <code> in JS comments based on it being used that way throughout much of core. However, I could not find documentation that would confirm this is or isn't proper usage. Based on the JS maintainer feedback in #25 and many existing examples of this kind of backtick use in core JS, I believe these can stay as-is unless there is documentation of the standard indicating otherwise.

If there are still concerns with backtick usage, this would be a good opportunity to open a followup issue to seek clarity on how this should be standardized.

I can confirm #1 in the description by looking at /admin/config/content/formats/manage/restricted_html in simplytest.me

You should be able to see that here:

Both the highlighted & hidden text has:

<details data-drupal-selector="edit-filters-filter-html-settings" id="edit-filters-filter-html-settings" class="js-form-wrapper form-wrapper seven-details vertical-tabs__pane" open="open" role="tabpanel" aria-labelledby="edit-filters-filter-html-settings-tab" tabindex="0" style=""> <summary role="button" aria-controls="edit-filters-filter-html-settings" aria-expanded="true" aria-pressed="true" class="seven-details__summary">

<details data-drupal-selector="edit-filters-filter-url-settings" id="edit-filters-filter-url-settings" class="js-form-wrapper form-wrapper seven-details vertical-tabs__pane" open="open" role="tabpanel" aria-labelledby="edit-filters-filter-url-settings-tab" tabindex="0" style="display: none;"> <summary role="button" aria-controls="edit-filters-filter-url-settings" aria-expanded="true" aria-pressed="true" class="seven-details__summary">

semantically, why are we making the summary a button? If anything is semantically a button it would be the vertical tabs menu:

<ul class="vertical-tabs__menu"><li class="vertical-tabs__menu-item first" tabindex="-1"><a href="#edit-filters-filter-html-settings"><strong class="vertical-tabs__menu-item-title">
      Limit allowed HTML tags and correct faulty HTML
    </strong><span class="vertical-tabs__menu-item-summary">Enabled</span></a></li><li class="vertical-tabs__menu-item last is-selected" tabindex="-1"><a href="#edit-filters-filter-url-settings"><strong class="vertical-tabs__menu-item-title">
      Convert URLs into links
    </strong><span class="vertical-tabs__menu-item-summary">Enabled</span><span id="active-vertical-tab" class="visually-hidden">(active tab)</span></a></li></ul>

I'm still trying to wrap my head around this again. It's been a while since I've looked at this.

semantically, why are we making the summary a button? If anything is semantically a button it would be the vertical tabs menu:

Good catch! I tracked this down to an issue from ~8 years back, created by a nice fellow with the username @mgifford #1848684: Add ARIA role to DETAILS. My hunch is assigning role="button" was something advised before browsers had agreement ondetails/summary. Unless I'm misremembering, button is the implicit role of summary, so removing this should be fine but it's presence is doing no harm either. I recommend removing that in a separate issue as it doesn't directly impact the bugs reported here and is a straightforward solution that can likely committed without much fuss.

Tagging for DrupalCamp Colorado DCCO2021 which is Sunday August 8.
https://2021.drupalcampcolorado.org/contrib-day

This patch can be re-rolled into a Merge Request to make maintainer review easier, thanks!

I was trying to understand what needs to be done/reviewed here but from the comments I’m not able to get clear instructions. Tagging this as Needs issue summary update to see if someone could help making it easier to know where to help of if there are things that are still being discussed.

Just re-rolled into a Merge Request as requested in #31 and addressed #24.2 & #24.3. Rest of the points in #24 are discussed in #25 & #26.

Thank you!

Back to needs work as merge request is not usable.

(DrupalCon contrib room)

Trying changing version back to 9.3.x because of dependency/version constraint issues.

No composer errors when I used 9.3.x-dev, so I think that means we need a reroll so that the MR works with 9.4.x, ya? -- as I write this, I think what I'm saying is basically the same as the "not currently mergeable" message!

Adding "needs reroll" -- I hope to do that this week at DrupalCon, but if someone else gets to it first, awesome!

MR does need reroll, but the files with rebase conflicts are JavaScript, and the conflicts *seem* substantive as far as I can tell -- and I don't have the JS expertise to work on a reroll!

If anyone wants to take a stab at it, maybe this will help:

• Rerolling patches > Dealing with rebase conflicts
• Files with conflicts: core/misc/vertical-tabs.es6.js and core/misc/vertical‎-tabs.js‎
• DrupalPod snapshot I started to work on this issue -- feel free to use it! -- I'll pin it so it stays around for a while: https://gitpod.io#snapshot/215d9854-73ed-4fa7-b0a8-807dc1f982e3

(Also, sorry for the issue notification noise!)

Updated attribution

Rerolled patch against 9.4.x

Tried to fix custom code error at #46

I'm so glad people are working on this issue, thank you, everyone!

Unfortunately, we have a mix of a MR and patches, and I'm not sure the best way to move forward. I'll post on Slack > #contribute channel for advice. Meanwhile! --

-------
I can see the MR has rebase conflicts:
- There's an error when I try the rebase button on GitLab: https://git.drupalcode.org/project/drupal/-/merge_requests/2169
- Also when I try to rebase via command line, there are conflicts: https://gitpod.io#snapshot/dc078644-d6cb-47be-b7c8-958e29760109

-------
@immaculatexavier (#46) Which patch did you reroll? -- please link to the comment or patch file, or if you rerolled a patch based on a MR, please link to that.

@ranjith_kumar_k_u (#49) Could you please summarize what your changes did?

Continuing to look at our MR/patch situation.

@bnjmnm I know you just created a new MR, but the branch you were working on isn't new, and also, there are a lot of commits not meant to be kept around, I'm sure? (file permissions, mainly)

How about creating a new branch for the issue, straight from 9.4.x (or new fork + branch, whichever is easier -- as long as it's created from the latest 9.4.x), and cherry-picking your commits, if possible? -- it might not work, the rebase conflicts might mean you need to redo the changes, but I think a fresh branch from 9.4.x (and closing MR 2169) is the best next step.

(I'm trying to compare the changes in #46 with MR 2169 -- maybe a place to start on a new branch is with the patch reroll in #46? -- depending on what @immaculatexavier says they rerolled "from," so to speak.)

Re #52 I think we may be more aligned than it seems:

and also, there are a lot of commits not meant to be kept around, I'm sure? (file permissions, mainly)

These were temporary changes to address merge conflicts, they're part of the commit history but not part of the current diff as these changes were reverted after they were no longer necessary.

How about creating a new branch for the issue, straight from 9.4.x

The branch I'm working on 3081500-vertical-tab-94 was created last week to do exactly that 🙂

It does not look like the patches in #46 - #49 do anything additional that hadn't already been present in the MR, but they do remove handleFragmentLinkClickOrHashChange, which shouldn't be removed.

@bnjmnm Thanks for all the updates and explanations!

I am very excited to try the latest MR tomorrow, and I should be able to get a screen reader + keyboard nav review soon thereafter.

Thank you!

Shoot sorry, "accidental status change via refreshed old browser tab" -- back to Needs review.

Hi, thank you again for working on this issue! I have some preliminary feedback. Also, I updated the issue summary for content and overall clarity (hopefully).

About me: I'm reviewing the MR on a plain Drupal 9.4 site with the Umami install profile. Specifically, I'm reviewing the vertical tabs elements on a text format settings form (i.e. /admin/config/content/formats/manage/filtered_html) and a block layout config form (i.e. /admin/structure/block/manage/claro_page_title).

Fixed:

1. Items #1 + 4 in the original issue summary appear fixed (aria-expanded="false" and aria-pressed="false"; use of label for attribute).
2. Item #2 in the original issue summary: I don't see in a plain Drupal 9 site, maybe it was fixed between when the issue was submitted and now? (or if it's fixed in the MR and I missed it, hooray!) (non-unique ID, #active-vertical-tab)
3. Item #3 in the original issue summary appears fixed 🎉 (and what an awful issue!)

Also fixed:

• Item #5 in updated issue summary: I can now use enter or spacebar to activate a vertical tab ⭐
• Item #6 in updated issue summary: aria-selected was missing from tab elements -- now it's there, and properly true/false based on whether the tab is active ⭐
• Item #7 in updated issue summary: Attribute aria-controls (on each tab) points to the related tabpanel's ID ⭐

Question + remaining issues:

• Question: Could you clarify your decision not to use aria-labelledby in favor of aria-label?
• Issue - item #8 in updated issue summary: (related) tabindex should be -1 on all non-active tabs / on all tab elements that are aria-selected="false".
• Issue - item #9 in updated issue summary: When I tab through the form, it should take me into an "active" tabpanel and then out of the tablist -- right now, it takes me through the tablist, then into the tabpanel. (If I press spacebar/enter on a tab, it does move focus to the active tabpanel then out of the tablist; it should behave like this with tabs that are open by default/on page load, too.)
• Issue - item #10 in updated issue summary: Using up/down arrow keys while focused on a tab should take user to the next or previous tab.

New issue / side effect

I ran into trouble with vertical tab field groups. I don't know if this fix should/could fix the same accessibility issues on field group vertical tabs, but it actually made things a little broken: Basically, vertical tab field groups look like details fieldsets (maybe accordion groups), not vertical tabs.

Steps to reproduce:

1. New Drupal 9.4.0-alpha1 site with Umami install profile.
2. Add/enable field_group module.
3. Set up vertical tabs on a content type, like recipe or article.
4. Use the content entry form for that content type -- you'll see that the field groups are collapsed fieldsets, not tabs.

Screenshots + YML attached -- screenshots show vertical tabs on a plain Drupal 9.4.0-alpha1 site, and then with the patch/MR applied to the same site. (I have DrupalPod snapshots, but gitpod snapshots are a little under the weather at the moment, so hopefully the screenshots and YML are sufficient -- if a full config export would be helpful, I can provide that!)

That's all for now, I hope I'm making enough sense to make sense -- please lmk if you have any questions or if I can provide more info on anything. Thank you!

With all this complexity don't you think that Vertical Tabs component needs a brand-new render element made from scratch?

Mmmm that might make sense! -- I probably don't have the best skill-set to have a strong opinion about it, but my gut feeling is that seems worth strong consideration.

It doesn't seems like any of the remaining points are any more difficult to implement because they're on an existing render element/JavaScript class. Starting from scratch would certainly create productive-feeling work, but we'd eventually hit the same implementation details that this was waiting on, but instead of fixing an existing element, we'd need to require people migrate to something entirely new to enjoy the benefits. There would also need to be a backwards compatibility layer for anything changed in core.

I believe this current patch addresses all the current concerns, including the change to arrow navigation. That could potentially be moved to a followup if this gets stalled on details, but I think it may be worth bundling to minimize the number of reviews/commits needed to get our Vertical Tabs current with modern A11y standards.

The outlined area does not match the tab container in Claro.

WCAG 1.3.1 SC

It's worth noting that comment #63 is not pointing out anything wrong with the changes being made in this issue. It's an unrelated pre-existing issue that was not caused by any of the accessibility fixes here and has its own issue. Patch #61 is still ready for review.

@bnjmnm, That's correct, but patch #61 made the problem outlined in #3327848: Claro: Wrong background for active vertical tab much more noticeable. I propose we postpone this issue till #3327848: Claro: Wrong background for active vertical tab is resolved.

Testing patch #61 on /admin/structure/block/manage/claro_page_title
Notice the aria-expanded and aria-pressed still remain false.

#69 is expected behavior. When vertical tabs are visible, the details and summary elements are hidden and inert and the attributes should not change. I updated the issue summary to clarify this.

Also, #3327848: Claro: Wrong background for active vertical tab is complete.

For accessibility rule

Tested on /admin/structure/block by adding a block
I can tab to vertical tabs and using my up/down arrows cycle through them.
Pressing enter opens the tab and moves focus to the first focusable element.
aria-selected attribute correctly updates.
The "a" tag for each tab aria-controls correctly points to the aria-own attributes with matching values.
The "a" tag that is active currently has tabindex="0" while the rest are -1

Also tested on /admin/config/content/formats/manage/full_html where 2 tab groups exists and had no issues of focus going to the wrong group,

Going to mark this and ask #accessibility channel if my review was good enough.

@bnjmnm you tagged for follow up 2 years ago that still needed?

We discussed this issue briefly at #3350284: Drupal Usability Meeting 2023-04-07. That issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @benjifisher, @mgifford, @rkoller, @simohell, and @swirt.

We were not sure exactly what was supposed to change, since we did not follow the links in the "Proposed resolution" section of the issue summary. But we did notice one problem with vertical tabs in the block configuration page (same as the screenshot from Issue #63).

1. Use the mouse to select the Pages vertical tab.
2. Tab to select Roles.
3. Enter to select the vertical tab. Focus shifts to the checkboxes in that tab.
4. Shift-tab. The focus goes to the last vertical tab (Vocabulary).

In Step 4, we expected the focus to return to Roles, not Vocabulary. We saw the same behavior with or without the patch from Comment #61.

I am setting the issue status to NW to consider this behavior. If the actual behavior is what we want, or if it is out of scope for this issue, then feel free to change the status back to RTBC.