Add focus behaviour for media widget with max elements

Closed #3091297: Node edit form with media library widget defaults focus to media field if it has a value as a duplicate.

Patch works as advertised, thank you! This has been driving me nuts :)

Tests are green and this passed manual testing so marking RTBC.

Nice find! Thanks. This should help make the stable Media Library more usable while also still retaining our accessibility commitment.

We need the transpiled ES5 JS in the patch too for this to be committable. Here's a reference on the transpilation: https://www.drupal.org/node/2815083

Updated patch attached with the transpiled ES5 JS.

We probably need accessibility signoff for this change once we've finalized it.

Marking this needs work because it changes the focus behaviour as well.

Without the patch, the form focuses on and scrolls to the media library widget when loaded (which is the wrong behaviour). But it also allows focus on the media library widget when the modal is closed (which is the desired behaviour).

With the patch, the form does not focus on or scroll to the media library widget when loaded (which is the desired behaviour). But it also loses focus on the media library widget when the modal is closed - focus returns to the top of the page (which is the wrong behaviour).

So we need a patch that both:

1. Prevents focus and scroll from going to the media library widget by default; focus should be at the top of the form by default
2. Sets focus to the media library widget when the modal is open

The original code has no comment that describes what it's actually for, but from what I can gather it seems questionable and I don't think that just ditching the scroll is the fix here.

@pameela, you mention "Sets focus to the media library widget when the modal is open". Do you mean when the user closes the modal it should then set focus back to the library widget?

Is my understanding of what should happen correct?:
1. When a user closes the modal, if the open button gets the data-disabled-focus="true" attribute (because you can't add anymore items?) then the button should be disabled, but should also get focus.
2. When the page originally loads, no focusing on the media widget should happen at all, but if the data-disabled-focus is on, then the button should be disabled.

In that case, we shouldn't be calling the focus function at all during page load, only when the user returns from a modal.
Is there a way to act on the modal close event instead of blindly acting every time the behavior fires?

I'm also not sure about the fact we're setting the focus back to that button in the case that it is disabled because you can't add more things.
It doesn't sound too bad usiong the osx screen reader but I find it a little confusing.

I wonder if it would be better to set the focus to the item that was just added instead of to the button, although the only place to set the focus would be on the remove button of the item?
Note sure if there's somewhere better would could set the focus that would make more sense to the user.

Would this be good?
I'm trying to set focus and so scroll to the element when the modal window is closed only.

I think it is really rudimental, but i haven't found (yet) a way to kind of track the element where the popup was originally opened, so that the focus and scroll would go back there.

Sorry for the delay, was in the throes of conference organising and then offline for a few days.

I have updated the issue summary. @rooby, your questions led me to realise the problem was with the missing button!

Does this make more sense?

In MediaLibraryWidget we have this:

    // 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['open_button']['#attributes']['data-disabled-focus'] = 'true';
      $element['open_button']['#attributes']['class'][] = 'visually-hidden';
    }

We might want to check $form_state->getTriggeringElement() before we add the data-disabled-focus attribute. We only want to trigger the refocus when a user selected something in the library. The triggering element should be the media_library_update_widget button.

I think this might be the best way to fix this.

While someone builds a solution based on #15, here's a patch fixing the errors in the patch in #13.

Added patches for latest versions

New approach per #15

No interdiff, as its a brand-new approach.

Worked on this for bug smash.

Removing accessibility review tag, because we're not changing anything w.r.t the media library functions here, just the default state

Fail was in HEAD

We need to update the IS to indicate #15 is the proposed resolution

Just reporting that #20 still applies to 8.9.2, in case anyone needs this for D8.

Updated IS and removed the original report at it was conflicting. Revision history will keep it if we ever need it.

Thanks @larowlan, works as advertised. Screenshots don't really tell you anything so I've done some screencasts of before and after.

Screencast before patch - focus jumps to media library on load: https://www.screencast.com/t/oeoGznWclEA1

Screencast after patch - focus stays at the top of the page as expected on load: https://www.screencast.com/t/DCRDNamfA

Screencast showing focus works as expected when selecting from the media library widget: https://www.screencast.com/t/nCle4Sonam

Just needs a code review.

Added screencasts to IS and removed the screenshot as it wasn't really helping :)

The fix looks good to me. Just one suggestion which is not a deal breaker IMO.

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
   @@ -48,6 +57,31 @@ protected function setUp(): void {
   +    $this->assertJsCondition('jQuery("#field_twin_media-media-library-wrapper .js-media-library-open-button").is(":disabled")');
   

   This can be $this->assertNotNull($open_button->getAttribute('disabled'));

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -507,8 +507,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      if ($triggering_element && ($trigger_parents = $triggering_element['#array_parents']) && end($trigger_parents) === 'media_library_update_widget') {
   

   Let's add a comment here explaining what we are trying to determine.
   Ignore this there is a comment already.

Committed and pushed fb46d2747f to 9.1.x and 6096fe17dc to 9.0.x and b409e09e92 to 8.9.x. Thanks!

I agree with @larowlan that this fix doesn't adjust the accessibility features of the media library so removing the needs accessibility maintainer review seems fine.

It's great that the code now matches the code comment :)

    // 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

The code in HEAD was not accounting for the When the user returns from the modal part of the comment.

diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
index 6b1a95fdf4..28db60805c 100644
--- a/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
@@ -2,8 +2,6 @@
 
 namespace Drupal\Tests\media_library\FunctionalJavascript;
 
-use Drupal\node\Entity\Node;
-
 /**
  * Tests the Media library entity reference widget.
  *

Removed unused use on commit.