Improve keyboard focus behaviour of vertical tabs in MediaLibraryWidget

Please remember to add the accessibility tag! Some of these can be fixed by community members who would like to work on accessibility, but that won't happen if it isn't tagged.

Please split this into 2 issues. They can be addressed separately.

Splitting this into 2 issues.

I filed #3035408: Identify purpose of vertical tabs in MediaLibraryWidget dialog for assistive tech users. for "Vertical tabs menu is not presented as navigation".

This one is for "Clicking vertical tabs doesn’t shift the user to the new content, next element becomes the ‘Select media’ button"

@seanb - I can't replicate this.

You just say "using VoiceOver" #2988431-12: [Meta] Accessibility plan for Media Library Widget. Can you say which browser and OS you used as well? (I guess you mean macOS, but iOS VoiceOver is quite different in many ways).

Using a keyboard but no assistive technology:

1. Pressing the link causes it to lose focus.
2. Now, there is no clear indication of what has focus.
3. Pressing tab after that causes the "add files" file input to gain focus.

I get the same result using all of these browsers:

• Edge 42 (EdgeHTML 17) on Win 10
• IE 11 on Win 10
• Firefox 64 on WIn 10
• Chrome 72 on Win 10
• Opera 58 on Win 10
• Firefox 65 on Kubuntu 18.10
• Chrome 72 on Kubuntu 18.10

In the click handler for the vertical tab links we have:
document.getElementById('media-library-content').focus();
My manual testing tallies with this, it's currently behaving as intended.

Whether it's the right behaviour is another matter. I think it might be better to shift focus to the first tabbable control inside #media-library-content, i.e. the add-files input. We can do this with the :tabbable selector from jQuery.ui.core

I'll do manual testing with some screen readers next.

Another round of manual testing, this time with Win 10 browsers from #7, and JAWS 2018

• Edge - using a vertical tab link causes it to loose focus. Pressing TAB moves focus to the add-files input.
• IE11 - using a vertical tab link causes it to loose focus. It isn't visually clear where focus is. JAWS announces "show as link show as table" (bizarrely). Pressing tab moves focus to the add-files input.
• Firefox 64 - using a vertical tab link causes it to lose focus. JAWS starts reading out the whole content of #media-library-content. I press control to make JAWS stop talking. Next tab press causes the add-files input to gain focus.
• Chrome 72 - Same as IE11.
• Opera 58 - Same as IE11.

There are a few unusual announcements, however the keyboard focus tab behaviour is the same as in #6 without JAWS.

I think the Firefox is reading the entire of #media-library-content because that's the element we focus. This will likely be improved by focussing the file input instead as I suggested in #6.

The IE/Chrome/Opera announcements puzzle me. I don't know why they announce the show as table/grid links. I hope this might also be improve by focussing the add-files input instead.

One of the microsoft browsers (didn't note which) announced "entering form" at one point, which is the first meaningful role inside #media-library-content. It didn't do it again. I think I might have accidentally pressed a key to advance to the next form. I'm not so proficient at JAWS as I am at NVDA.

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -216,10 +216,18 @@ public function testWidget() {
+    // Assert the focus is set to the first tabbable element when a vertical tab
+    // is clicked.
+    $this->assertJsCondition($this->getSession()->evaluateScript('jQuery("#media-library-content :tabbable:first").is(":focus")'));

This is an incorrect use of assertJsCondition(). I think we're supposed to just pass it the JavaScript string to evaluate, not the evaluation result.

Additionally, I feel like this issue might benefit from a fail patch.

I have tested 3035331-11.patch using Mac OS X Safari, VoiceOver, and keyboard tab / space key only, and it is working well.

My Media field allows for Audio, Image, and Video. When I use the space key on the Add Media button in the field on a clean form, it loads the modal, with the Audio tab selected.

The screen reader tells me which tab is selected, and the visuals clearly indicate which tab is focused.

Pressing the tab key gets me to the next vertical tab, and tells me which one it is ("link, Video") and so on.

The thing that I wish, however, is that once I get into the select media button, that it would tell me what type of media I'm selecting. I have to remember that I was on Audio first, and then tabbed through the image and video options without selecting them, if I want to be sure of what type of content I'm adding.

The thing that I wish, however, is that once I get into the select media button, that it would tell me what type of media I'm selecting. I have to remember that I was on Audio first, and then tabbed through the image and video options without selecting them, if I want to be sure of what type of content I'm adding.

I talked about this with @phenaproxima and @seanb, and we agreed that we should file a separate followup issue for this part. Thanks @rainbreaw!

I retested what I tested above a few months ago, and think this one is in good shape. Everything is still working in a logical manner when interacting with just the keyboard or just a screen-reader+keyboard.

Looks like we still need the followup filed (or linked here if it already exists) and the patch also needs a reroll I think. Thanks!

Here's a reroll.

I dropped this line, as it looks like it's been refactored away.

@@ -290,7 +298,6 @@ public function testWidget() {
     $assert_session->pageTextNotContains('Turtle');
     $page->clickLink('Type Three');
     $assert_session->assertWaitOnAjaxRequest();
-    $assert_session->elementExists('named', ['link', 'Type Three (active tab)']);
     $assert_session->pageTextNotContains('Dog');
     $assert_session->pageTextNotContains('Bear');
     $assert_session->pageTextNotContains('Turtle');

Filed #3083090: In the media library, announce the type of media being selected to address @rainbreaw's request in #14.

Fixing the test. I don't know why, but it looks like the '.active-tab' text changed?

<span class="active-tab visually-hidden"> (selected)</span>

The test was looking for $this->assertTrue($menu->hasLink('Type One (active tab)')); . Which doesn't happen now.

I tried $this->assertTrue($menu->hasLink('Type One (selected)')); and that didn't work, but this worked:

+    $this->assertNotEmpty($type_one = $menu->findLink('Type One'));
$this->assertTrue($type_one->find('css', '.active-tab'));

I don't see any precedence for this in core. Is this good enough? Any suggestions on how to better check active tab?

This replaces #22 with something closer to the original patch. The reason " (active tab" wasn't working was due to changes here:
#3035408: Identify purpose of vertical tabs in MediaLibraryWidget dialog for assistive tech users.

diff --git a/core/modules/media_library/js/media_library.ui.js b/core/modules/media_library/js/media_library.ui.js
index 0a29f7818f..8a7a229279 100644
--- a/core/modules/media_library/js/media_library.ui.js
+++ b/core/modules/media_library/js/media_library.ui.js
@@ -70,7 +76,11 @@
 
         $menu.find('.active-tab').remove();
         $menu.find('a').removeClass('active');
-        $(e.currentTarget).addClass('active').html(Drupal.t('@title<span class="active-tab visually-hidden"> (active tab)</span>', { '@title': $(e.currentTarget).html() }));
+        $(e.currentTarget).addClass('active').html(Drupal.t('<span class="visually-hidden">Show </span>@title<span class="visually-hidden"> media</span><span class="active-tab visually-hidden"> (selected)</span>', { '@title': $(e.currentTarget).data('title') }));
+
+        Drupal.announce(Drupal.t('Showing @title media.', {
+          '@title': $(e.currentTarget).data('title')
+        }));
       });
     }
   };

Yup, this is the right reroll, and the updated test coverage matches the changes that have occurred in core. :) This is ready to go!

Removing the "needs accessibility review" tag, since that has long since been completed and, indeed, re-verified.

Issue credit.

Awesome! Thanks for the fix, as well as the thorough testing here!

Committed and pushed to 8.8.x. Thanks!