Improve refocus on submit buttons of Media Library Widget modals

I can confirm the patch (#4) fixes this issue.

I can also confirm the patch from #4 fixes the issue.

This annoyed me as well. Thanks for fixing this. Instead of disabling the focus though, we could wait for #3026636: Allow AJAX links to replace a specific selector and set the focus to whatever we think is best. Let's ask the accessibility team what the most logical place would be, since this impacts screen readers the most.

Even though we can't test which element has the focus (as far as I know), we could at least assert that all the right data attributes are added to the link.

Ok just looked at this again. Waiting for #3026636: Allow AJAX links to replace a specific selector doesn't make any sense, the focus change is triggered by the form buttons on the modal, not a link. Sorry about that.

Disabling the focus seems like the best way to go for the modal form, so +1 on that. Specially if the modal is going to be used by the CKEditor and/or other (custom) forms in the future.

While writing a test (turns out we actually can test the focus) I looked at where the focus was changing to. Since the selection is the part of the form that actually changes, we should shift the focus to the updated selection div. The attached patch does exactly that.

The old patch needed a reroll as well, so the interdiff was broken.

Reroll was needed now #3023802: Show media add form directly on media type tab in media library has landed. That means there is currently only 1 button to go back to the widget.
New patch attached.

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -648,6 +658,11 @@ public function testWidgetUpload() {
+    $this->assertTrue($this->getSession()->evaluateScript('jQuery("#field_twin_media-media-library-wrapper .media-library-selection").is(":focus")'));

Nit: Can we use $this->assertJsCondition() in this test for clarity?

Fixed #13.

seanb asked me to look at this.

Which exact HTML element are you proposing to focus? The proposed resolution doesn't say. I can't tell which element has focus from watching the animated GIF either. That probably means it fails WCAG 2.4.7 Focus Visible.

Usually when dismissing or completing a dialog, focus should return to the control which invoked the dialog.. That way it's easy to reorientate yourself after dealing with the dialog; you're back where you came from. No surprise.

For some reason the add media button disappears - why? That's something we should fix. It's generally not a good idea for parts of the underlying page to disappear while you're in a dialog, esoecially the control that iprompted the dialog. (It's like going to the refrigerator, and finding you've run out of milk. So you go to the shop to buy some milk, and when you come home the refrigerator has gone.)

I didn't understand the discussion about how it determines which element to focus, or the part about the data-disable-refocus attribute. Are these part of the general Drupal AJAX, or specific to media library? What is tbe purpose and behaviour of the data-disable-refocus attribute? I'm confused about the name (it's a peculiar mix of "dis-" and "re-")

Update: I found the bits in RenderElement and ajax.js about the data-disable-refocus, will try to grok those.

Aside: I hid the animated GIFs. Now they're links. They're a serious WCAG failure, because they loop and there's no way to stop them. It was really hard to pay attention to one while the other was also playing. Movie files would be preferable.

Which exact HTML element are you proposing to focus? The proposed resolution doesn't say.

The last patch shifts the focus to the div element that contains all the selected media items. So you can directly tab to the selected items.

Usually when dismissing or completing a dialog, focus should return to the control which invoked the dialog.

Makes sense, we will fix that.

For some reason the add media button disappears - why? That's something we should fix. It's generally not a good idea for parts of the underlying page to disappear while you're in a dialog, especially the control that prompted the dialog. (It's like going to the refrigerator, and finding you've run out of milk. So you go to the shop to buy some milk, and when you come home the refrigerator has gone.)

This has been done because of the field cardinality, since you are not supposed to select more items if the field doesn't allow that. I can see how that can be confusing. If we need to shift the focus back to the button we probably have to solve that in this issue.

I didn't understand the discussion about how it determines which element to focus.

There are 2 reasons the media library should not try to set the focus in the parent form:

1. The media library is generic functionality that is not necessarily called from a entity reference widget (for example, we also want to add this to CKEditor).
2. The media library dialog currently updates a hidden field in the parent (node) form with a comma separated list of IDs. That triggers a second AJAX command in the node form entity reference widget, to show the media items in that widget. Since that second AJAX command is the last thing that happens, that command should be responsible for setting the focus (the current patch already does this).

Still to do (please correct me if I'm wrong):

1. If I understand correctly the focus should be returned to the button that opened the media library, not the div that contains the newly added media items.
2. The problem we currently have is that the button is removed when the field cardinality is reached. We should find a way to have the button, without allowing users to select more items. Any suggestions?
3. A question I have, which might be an issue). How does the user know that new items are added to the form when the focus is returned to the button? The button is also displayed below the added media items, so the user is already passed that point in the dom.

While working on this issue I found fixing this in an accessible way impacted way more than the original intention of the issue. Since I don't want to hijack this issue to fix everything, I created #3035316: ‘Add media’ in MediaLibraryWidget should be a button, not a link to improve the widget accessibility. Let's continue the discussion in that issue and use this issue to add a quick fix.

#3035316: ‘Add media’ in MediaLibraryWidget should be a button, not a link was very loosely scoped assortment of a11y issues, so I've broken it up into separate issues to make triage easier.

I think we should carry on here for the focus return when the dialog closes.

We already had #2988431: [Meta] Accessibility plan for Media Library Widget as a plan issue with some child issues, so let's make that the parent meta-plan for Media Library accessibility.

Here is a new patch to put the focus back in the widget as well. The open button is currently still removed when the maximum number of items reached. I tried having a disabled button, but these don't allow focus. Disabled buttons are also problematic for accessibility as far as I found.

As long as adding items is allowed, this patch keeps focus on the open button. When the maximum allowed items is reached the open button is removed and the focus is set to the remove button of the first media item.

Once #3035446: Inform assistive tech users about the outcome of using the MediaLibraryWidget dialog lands this might be a lot more clear, since that issue actually announces to the user the maximum number of items is reached.

+    else {
+      $focus_selector = "#$wrapper_id .media-library-item__remove:first";
+    }

Remove is a kind of "danger" button. I'm wary of moving focus there programatically. There's potential for unintended data loss and/or frustration, say in the case of a user who has a hand tremor and pushes enter accidentally. Heck, that happens to most of us at least once a week.

Some other places we can consider for this fallback:

1. Put a tabindex="-1" on the fieldset legend and use that as the return focus location.
   • Pros: it's the start of the fieldset, but isn't the container. Screen readers will (I think) read the legend, but we avoid reading the entire container.
   • Cons: it's not operable, so it won't have visible focus indicator. Sighted keyboard users may still be lost, until they press tab.
2. Put focus on the first :tabbable control in the fieldset. Curiously, this turns out to be the show/hide weights button.
   • Pros: it's not a danger button, it's a very harmless button. It has a visble focus indicator, so sighted users will see a focus indicator.
   • Cons: what about single-value media fields? I was surprised to find these have drag and weight behaviour, but maybe they shouldn't have that.

a11y triage: some kind of solution here is a must-have. Updating the a11y plan issue and roadmap.

Remove is a kind of "danger" button. I'm wary of moving focus there programatically. There's potential for unintended data loss and/or frustration, say in the case of a user who has a hand tremor and pushes enter accidentally. Heck, that happens to most of us at least once a week.

Excellent point. So let's not do this then.

Since you are essentially done with the field, can we maybe announce something like 'Added 4 items. Maximum number of items selected. Continue with the next field.', and shift focus to the next tabbable element in the form? Which would probably be the next field. That would maybe save users the trouble of tabbing through all elements they just added as well. You can always go back, but that might not be the preferred action.

#23:

Since you are essentially done with the field, can we maybe announce something like 'Added 4 items. Maximum number of items selected. Continue with the next field.', and shift focus to the next tabbable element in the form? Which would probably be the next field. That would maybe save users the trouble of tabbing through all elements they just added as well. You can always go back, but that might not be the preferred action.

No, that's all out of scope, and almost all of it is a bad idea.

Breaking it down:

1. "Added 4 items." - this is probably fine, but we have #3035446: Inform assistive tech users about the outcome of using the MediaLibraryWidget dialog for that. It's out of scope here. Keep manual a11y testing as simple as possible by not conflating issues.
2. "Maximum number of items selected" - I don't think we need this. Assume the user has done what they intended to do. They were told about the number of items every time we they checked a checkbox.
3. "Continue with the next field" - NO, we definitely don't want to announce this because:
   • That's what you normally do with a form. No need to patronize a screen reader user!
   • It's up to the user whether they want to deal with any other fields. The user may be editing an existing node, and the media reference field is the only thing they intend to change.
4. "shift focus to the next tabbable element in the form? Which would probably be the next field" - NO, the dialog was invoked from the media reference field, so it should go back to the media reference field. This risks confusing users about the form structure. Leave them to explore the form themselves.
5. "That would maybe save users the trouble of tabbing through all elements they just added as well."
   • However, it would also deny them the opportunity to review the items in the media reference field.
   • They may not be interested in any other fields.
   • If the next field is a text field, this could be frustrating for speech control users, if they aren't intending to dictate into that field, they'll need to take it out of editing mode.

Accessibility is about eliminating barriers, and providing an equivalent experience to all users (or as near as we can). It isn't about telling users what to do, or doing things for them that they didn't ask for.

The intended use for Drupal.announce() is to fill any gaps needed to achieve the equivalent experience. In particular, we use it to notify screen reader users about important visible changes that happen away from the element which has focus. It isn't for providing a detailed running commentary about everything that happens.

Ok sorry about that then. So what now, which option should I implement?

1. Put a tabindex="-1" on the fieldset legend and use that as the return focus location.
OR
2. Put focus on the first :tabbable control in the fieldset.

There isn't much in it, but I'm leaning to the first :tabbable - because that will have a visible focus.

Done! Focus is now set the first tabbable element in the widget.

1. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -97,6 +97,7 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +      'data-disable-refocus' => 'true',
   

   I think this line could use a comment.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -314,6 +314,19 @@ public function testWidget() {
   +    $checkboxes = $page->findAll('css', '.media-library-view .js-click-to-select-checkbox input');
   +    $checkboxes[0]->click();
   

   To prevent a potential "calling a function on null" fatal error here, it might be worthwhile to assert that $checkboxes has at least one item in it before calling click(). ($this->assertGreaterThanOrEqual(1, count($checkboxes)), I think, would do it).

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -314,6 +314,19 @@ public function testWidget() {
   +    $this->assertJsCondition($this->getSession()->evaluateScript('jQuery("#field_twin_media-media-library-wrapper .media-library-open-button").is(":focus")'));
   

   $this->assertJsCondition() should be passed a string of JavaScript code to evaluate, not the result of the evaluation.

Fixed #29.

Rerolled now #2981044: Unify the grid/table views of the media library landed.

After discussing this with JulezRulez the conclusion was that a disabled open button was better than removing it. We also showed the attached patch to rainbreaw and I think she agreed it was best to keep the open button, set the focus and disable it.

I learned we can put the focus to the disabled open button if we set the focus right before we disable the button, so added a JS trick to make that happen. I think this would address the concern from andrewmacpherson in #15. Needed to reroll this patch on top of #3035316: ‘Add media’ in MediaLibraryWidget should be a button, not a link to make that happen though, since we can't really make a link disabled.

While doing this, I also learned that the screen reader focus is not the same as browser focus, and there is very small delay between setting the browser focus and the shift in focus by the screen reader. Added a workaround for that, which probably needs frontend maintainer review. From what I found, there is no way around that though.

Attached is a video to show the focus interaction with voiceover enabled. Interdiff is a bit messed up since I rerolled on top of #3035316: ‘Add media’ in MediaLibraryWidget should be a button, not a link, but still added it to give an indication of the changes. And updated the IS.

As @seanB mentioned in #32 above, he and @phenaproxima demo'd this with me earlier today.

From an accessibility standpoint, I like the solution of returning the user to a disabled Add Media button. From a usability standpoint, I also like the idea of keeping the button present even for sighted users (in the disabled state), as this makes it clear to the user that they have reached their limit by showing them that nothing is broken, it simply isn't active anymore.

I'm hoping that @seanB's workaround of delaying the deactivation until after the button receives focus passes front end review. If it doesn't, however, another option might be to create a fake button (an item that looks just like an inactive button, but isn't really), make it look deactivated and focusable, and then use aria-label to tell a screen reader that it is a deactivated button.

One thing to keep in mind here, as well, is that re-focusing to the last place one was before a modal opens is not just to support screen reader users. This is also important for users who are relying on just the keyboard (rather than a mouse), as the location they are re-focused on is where they will have to continue tabbing from. If the tab focus does not go to where it was before the user opened the modal, the could be frustrated and have to do extra work.

Postponed on #3035316-34: ‘Add media’ in MediaLibraryWidget should be a button, not a link.

#3035316: ‘Add media’ in MediaLibraryWidget should be a button, not a link landed! This is now unblocked.

I'm also tagging this for backport to 8.7.x, since Media Library will hugely benefit from this UX / a11y improvement and, as an experimental module, this patch may be eligible to land in alpha.

+++ b/core/modules/media_library/js/media_library.widget.es6.js
@@ -70,4 +70,34 @@
+          // There is a small delay between the focus set by the browser and the
+          // focus of screen readers. We need to give screen readers time to
+          // shift the focus as well before the button is disabled.
+          setTimeout(() => {
+            $(this).attr('disabled', 'disabled');
+          }, 50);

This might need a bit more explanation. I found that without the setTimeout(), the browser would move the focus to the disabled button just fine, but the screen readers would move focus to the first element in the page.

After quite a bit of searching I found the following in https://webaim.org/discussion/mail_thread?thread=4561:

I've also found in playing around that adding a short delay using
setTimeout(), e.g., on the order of 200ms, before setting focus can
sometimes work to allow the JAWS virtual buffer enough time to update. I
don't particularly like this approach, but it has worked for me in the
past, and being a very short timeout, is mostly imperceptible to the
user. I'm not quite sure what is going on, but wonder if it has to do
with the increased efficiency of JavaScript engines in newer browsers
somehow working too fast for JAWS and its virtual buffer. But I'd love
to be educated if anyone has some insight.

So there is apparently a virtual buffer that takes time to update, and calling .focus(); and attr('disabled', 'disabled'); directly after each other is too fast for the virtual buffer to catch up. If anyone has a better way to solve this, please let me know. I don't like the setTimeout() at all, but I don't have any other solution at the moment.

Rerolled since we have landed some issues. Yay!

Also added a visually hidden class to the disabled button. We have shown a demo of this patch to rainbreaw and I think she agreed that this is ok, as long as tab users and screen readers still put the focus back on the disabled button.

@seanB and @phenaproxima demo'd this with me on a hangout this morning, AND I just had the pleasure of testing the patch using my keyboard and screen reader (VoiceOver on Mac OSX only).

From my review, this is working well! I did not use my mouse or eyes at all to get through the form, and was able to:

1. Add one new media item, save it
2. Press the space bar to activate the Add Media modal again
3. Add four new media items
4. Appropriately hear what was happening (there is a separate issue - #3034243: Improve selection text in Media Library - with the selection text, which is confusing)
5. Select those items to insert
6. Refocus on the "Add Media" button to know exactly where I am
7. Understand that I had now reached my maximum number of items and that the button was deactivated (and since visibility-hidden uses the clip method, it is not visible to sighted users, but still indicated to screen reader users)
8. Continue tabbing on to the next part of my form, in logical DOM order

After doing further research and cross-checking, I'm pretty sure that you will have to continue to use the time delay technique in order to give screen readers enough time, since this is an outlying use case for focusing on a disabled item. The only other option would be to not actually disable the button, and instead have it do something different than what it does by default. While typically you wouldn't want to focus on something that the user is not able to interact with, I think this case merits the behavior. To be fair, though, this is an area where a11y conformity becomes subjective, and real-world testing may indicate a better solution.

Based on my conversation with @seanB, I believe this issue will be RTBC once the frontend framework manager signs off.

Thanks for writing documentation to explain some of the uncommon things you've had to do on this patch, it makes it all very clear! Removing the tag since I don't have any particular concerns about the approach taken here.

Thanks, @lauriii! Setting to RTBC per @rainbreaw's feedback.

Quick question -- do we have test coverage asserting that, if you remove an item from the widget, the button appears again? If not, let's add that. Otherwise, back to RTBC.

Added the test for #42.

Two nitpicks.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -475,6 +475,10 @@ public function testWidget() {
   +    $this->assertFalse($assert_session->elementExists('css', '.media-library-open-button[name^="field_twin_media"]')->hasAttribute('data-disabled-focus'));
   +    $this->assertFalse($assert_session->elementExists('css', '.media-library-open-button[name^="field_twin_media"]')->hasAttribute('disabled'));
   

   Supernit: It'd be nice to get the element in a variable so we don't have to have this long selector twice.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -475,6 +475,10 @@ public function testWidget() {
   +    $this->assertJsCondition('!jQuery("#field_twin_media-media-library-wrapper .media-library-open-button").is(":disabled")');
   

   The ! sort of gets lost in the JavaScript string here. Maybe we could use .is(":not(:disabled)") instead?

Fixed #44.

Couple of questions

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -421,9 +428,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -      '#access' => $cardinality_unlimited || $remaining > 0,
        ];
    
   +    // When the user returns from the modal to the widget, we want to shift the
   +    // focus back to the open button. If the user is not allowed to add more
   +    // items, the button needs to be disabled. Since we can't shift the focus to
   +    // disabled elements, the focus is set back to the open button via
   +    // JavaScript by adding the 'data-disabled-focus' attribute.
   +    // @see Drupal.behaviors.MediaLibraryWidgetDisableButton
   +    if (!$cardinality_unlimited && $remaining === 0) {
   +      $element['media_library_open_button']['#attributes']['data-disabled-focus'] = 'true';
   +      $element['media_library_open_button']['#attributes']['class'][] = 'visually-hidden';
   

   doesn't this mean someone can manipulate the dom and set the button back to enabled? What impact does that have? Does that allow them to do anything nefarious?

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -497,7 +517,30 @@ public static function updateWidget(array $form, FormStateInterface $form_state)
   +    // Shift focus to the open button if the user did not click the remove
   +    // button and the open button is not going to be disabled. When the user is
   +    // not allowed to add more items, the button needs to be disabled. Since we
   +    // can't shift the focus to disabled elements, the focus is set via
   +    // JavaScript by adding the 'data-disabled-focus' attribute.
   

   this is a bit confusing - the 'is not going to be disabled' but could use some clarification.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -462,13 +494,16 @@ public function testWidget() {
   +    $this->assertGreaterThanOrEqual(4, count($checkboxes));
   
   @@ -527,6 +561,7 @@ public function testWidget() {
   +    $this->assertGreaterThanOrEqual(4, count($checkboxes));
   

   any reason for greater than here and not an explicit quantity? if so, should we add a comment saying why?

doesn't this mean someone can manipulate the dom and set the button back to enabled? What impact does that have? Does that allow them to do anything nefarious?

People can already manipulate the state of the media library, and its DOM -- we don't yet have a tamper-proof hash to validate the important state we are passing back and forth. The issue to add such a hash is open, with a patch: #3038241: Implement a tamper-proof hash for the media library state. So even if people can manipulate the DOM, it doesn't really open any more holes than we've already got.

That said, off the top of my head I can't think of anything especially nefarious this would allow you to do, beyond referencing more media items than are allowed by the field (which would, in turn, cause a validation error later).

#47

1. See #48
2. Fixed. Changed the comment a bit to hopefully better explain what is happening.
3. Fixed.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -497,7 +517,30 @@ public static function updateWidget(array $form, FormStateInterface $form_state)
+      $response->addCommand(new InvokeCommand("#$wrapper_id .media-library-open-button", 'focus'));

I tested the media library with a screen reader and this is a very nice improvement! Let's just change this to use a js- prefixed class instead.

Fixed #51.

Thanks! Feedback is addressed; back to RTBC.

Updated issue credits

Committed befdca5 and pushed to 8.8.x. Also cherry-picked to 8.7.x. Thanks!

+++ b/core/modules/media_library/js/media_library.widget.js
@@ -36,4 +36,18 @@
+  Drupal.behaviors.MediaLibraryWidgetDisableButton = {
+    attach: function attach(context) {
+      $('.js-media-library-open-button[data-disabled-focus="true"]', context).once('media-library-disable').each(function () {
...
+        $(this).focus();
...
+      });
+    }
+  };

This code causes the page to focus a visually hidden element on the first page load or when the form is rebuilt using AJAX (for example using Paragraphs). It is quite confusing.

There is an issue to fix this behavior at #3073023: Content Edit page jumps when media library item is placed.