‘Add media’ in MediaLibraryWidget should be a button, not a link

This issue has a broad title, and is a spin-off from a review issue with a similar broad title, to fix some of the issues found there.

Manual accessibility testing is time consuming, and having an issue where points 1-5 here relate to points A1-A2 and A11-A13 on the parent is just going to hold up accessibility review. There's a good chance reviewers will get lost, or confused about what's been addressed.

Can we give this a specific title about what it is fixing? Also narrow the scope and use separate child issues for the parent.

Also, please tag the spin-off issues from the parent, otherwise the implementation won't get the attention people who want to contribute to accessibility.

I'm getting lost already, keeping track of the numbers when jumping between the problem and resolution lists,

Please separate this into 5 issues. None of them are remotely related to each other, and they a mix of major and minor. There are possible quick-wins, and some which will require a bigger manual testing effort across multiple assistive tech or input modalities.

I've started breaking this up into separate issues. Created these two..
#3035434: Improve name of MediaLibraryWidget dialog
#3035435: Make the show/hide row weights button more informative for assistive tech users.

I think we can treat #3016807: Improve refocus on submit buttons of Media Library Widget modals as the issue for "After selecting items from the library the focus shifts to the save button of the parent form. I would expect to be back in the widget to verify my selection". There's already plenty analysis there.

Filed #3035446: Inform assistive tech users about the outcome of using the MediaLibraryWidget dialog to address "The text 5 items remaining is part of the element description but below the add button. After changing the selection the number of remaining items are not communicated to the user."

Which leaves this issue as "‘Add media’ in widget should be a button, not a link".

1. +++ b/core/modules/media_library/media_library.module
   @@ -67,23 +67,6 @@ function media_library_theme() {
   -    if ($view->current_display === 'widget') {
   -      $query = MediaLibraryState::fromRequest(\Drupal::request())->all();
   -      // If the current query contains any parameters we use to contextually
   -      // filter the view, ensure they persist across AJAX rebuilds.
   -      // The ajax_path is shared for all AJAX views on the page, but our query
   -      // parameters are prefixed and should not interfere with any other views.
   -      // @todo Rework or remove this in https://www.drupal.org/node/2983451
   -      if (!empty($query)) {
   -        $ajax_path = &$output['#attached']['drupalSettings']['views']['ajax_path'];
   -        $parsed_url = UrlHelper::parse($ajax_path);
   -        $query = array_merge($query, $parsed_url['query']);
   -        $ajax_path = $parsed_url['path'] . '?' . UrlHelper::buildQuery($query);
   -        if (isset($query['media_library_remaining'])) {
   -          $output['#attached']['drupalSettings']['media_library']['selection_remaining'] = (int) $query['media_library_remaining'];
   -        }
   -      }
   -    }
   

   It turns out this is dead code which should have been removed when we added vertical tabs to the media library. Keeping it here breaks this patch anyway, so I'm glad this is getting cleaned up now.

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -281,7 +283,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +        'library' => ['media_library/widget', 'core/drupal.dialog.ajax'],
   

   I get the feeling that the media_library/widget library should declare an outright dependency on core/drupal.dialog.ajax; I'm not sure under what circumstances it wouldn't need it. On the other hand, if we do that, we'll also need post-update hook in order to ensure that the library definition is refreshed. So maybe that is out of scope for now, and can be done in a follow-up (linked in the code here).

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -388,23 +390,29 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // Prevent errors from opening the library.
   +      '#limit_validation_errors' => [],
   

   This comment is wacky :) It should say "Allow the media library to be opened even if there are form errors."

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -481,7 +491,9 @@ public static function updateWidget(array $form, FormStateInterface $form_state)
   -    return $element;
   +
   +    return (new AjaxResponse())
   +      ->addCommand(new ReplaceCommand("#$wrapper_id", $element));
   

   Why is this needed? Couldn't we still just return $element, as long as the #ajax wrapper is set elsewhere?

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -519,6 +531,26 @@ public static function removeItem(array $form, FormStateInterface $form_state) {
   +    $library_ui = \Drupal::service('media_library.ui_builder')->buildUi($triggering_element['#media_library_state']);
   

   The service should be injected.

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -519,6 +531,26 @@ public static function removeItem(array $form, FormStateInterface $form_state) {
   +      ->addCommand(new InvokeCommand("#drupal-modal", 'focus'));
   

   This line could use a comment. Also, is there any constant somewhere that refers to #drupal-modal, so that we don't need to hard-code it here?

7. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -53,6 +53,9 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +    // Add the view to the form state.
   +    $form_state->set('view', $this->view);
   

   I think this comment needs to be expanded, at least with a @see so we know where it's being used, and how.

#12 - right, that all sounds like a big job which has benefits outside of a11y.

As far as the a11y triage, this one is a should-have rather than a must have. Confusion between links and buttons is a frustration for lots of assistive tech users, but not a brick-wall barrier. Many screen reader users are accustomed to the fact that developers use links and buttons interchangeably.

If it's too difficult to change this to a proper HTML <button>, another option is to use <a role="button"> but then it needs an extra keypress handler so it responds to the spacebar as expected. A HTML <button> is still preferred; the first rule of ARIA is don't use ARIA.

Updating issue summary, and parent issue with a11y should-have triage.

Also re. #12:

I needed to make extra changes to make sure the MediaLibraryState object can be passed around to various parts of the library. I think this it's a good thing we found this.

If you need to rescope the issue as an architectural change for robustness, that's ok. The button-vs-link issue can be called the initial use-case which needs it.

Also implemented a better fix for #2805219: Some dialogs do not receive focus when opened on the open button now I actually know what the problem was.

Can you please explain this? There are a bunch of other dialogs that need fixing too - please comment at [#1805219].

Also, both of those changes seem out of scope for this issue.

Update: sorry, I understand what the test fixes are about now.

Needed a re-roll since #2989503: Add tests to prove that the media library widget works when target_bundles is NULL was committed today.

Patch #20 affects a couple of files in Layout Builder module, which surely isn't right.
The interdiff in #20 isn't right. Here's what I get by comparing #18 and #20.

This patch updates #20.

• It needed a re-roll after #2989503: Add tests to prove that the media library widget works when target_bundles is NULL
• I undid the changes to layout_builder module which I think were unintended in #20.

The exact way I did it was:

git apply --3way 3035316-20.patch
git reset core/modules/layout_builder
git checkout core/modules/layout_builder

+++ b/core/modules/media_library/media_library.module
@@ -69,8 +69,8 @@ function media_library_theme() {
+    if (strpos($view->current_display, 'widget') === 0) {

Nice future-proofing. :)

Can't find anything else glaringly wrong with the code in this patch. Once accessibility review is complete, this is RTBC.

I'm having trouble applying this patch for manual testing...

None of the patches from #24 or #25 apply now.
I've lost track of exactly which patch is which, so I've assumed that @seanb's patch from #24 is the best one.
It's currently green, but fails to apply on my machine, and I currently get a 403 access denied when I try to requeue tests here.
I got confused looking at git *.rej trying to re-roll it.

I'll have to come back to this later. Can one of you try a re-roll if needed?

+  /**
+   * AJAX callback to open the widget when the selection changes.

I don't think this makes sense. Isn't it a "callback to open the widget when the button is pressed"?

Rerolling #24. There was one conflict in MediaLibraryTest around line 489. I think #24 was adding an extra assertWaitOnAjaxRequest() (which I kept).

$page->find('css', '.ui-dialog-titlebar-close')->click();
    $assert_session->assertWaitOnAjaxRequest();
    $assert_session->elementExists('css', '.media-library-open-button[name^="field_unlimited_media"]')->click();
    $assert_session->assertWaitOnAjaxRequest();

I think testbot is out of commission today, FYI. Also, I was just re-rolling and adding text change based on #31. I haven't tested the patch.

seanB and phenaproxima showed this one to me on a google hangout today for accessibility review, as well. In this patch, the code is using input type="submit" (which is a native HTML button), and behaving as expected.

We discussed the relationship of this issue to #3016807: Improve refocus on submit buttons of Media Library Widget modals. #3016807: Improve refocus on submit buttons of Media Library Widget modals is, in essence, blocked by this issue unless doing a workaround with a fake item, which increases the priority of this item.

This now officially blocks #3016807: Improve refocus on submit buttons of Media Library Widget modals, so I'm escalating it to the same priority.

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -420,7 +420,7 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   -    $state = MediaLibraryState::fromRequest($this->getRequest());
   +    $state = $form_state->get('media_library_state');
   

   After this change, I think AddFormBase.php will no longer need to use the MediaLibraryState class, so can we remove the use statement?

Looks correct to me otherwise. Now that we have run this through an accessibility review, RTBC once this is fixed.

Nice.

Tagging for backport to 8.7.x since this is a major accessibility improvement in an experimental module, and therefore may be eligible to land in alpha.

Committed and pushed 58d6b44288 to 8.8.x and 379e676d9b to 8.7.x. Thanks!

This looks like a good change and unblocks more media library work - go team :)

Credited @phenaproxima for code review and @rainbreaw for jumping on a call to discuss accessibility - thanks!

@phenaproxima asked for a revert as an AJAX bug has been found.

Tagging for tests and a fail patch.

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -114,6 +115,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Since the media library can be loaded via AJAX, we force the action URL
   +    // of the add form to the media library controller so we now for sure the
   +    // form is actually on the page.
   +    $form['#action'] = Url::fromRoute('media_library.ui', [], [
   +      'query' => $form_state->get('media_library_state')->all(),
   +    ])->toString();
   

   This comment is a bit confusing. I would suggest we rephrase it thusly: "The media library is loaded via AJAX, which means that the form action URL defaults to the current URL. However, to add media, we always need to submit the form to the media library URL, not whatever the current URL may be."

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -628,18 +629,23 @@ public function testWidgetUpload() {
   +    // Assert the upload form is not visible for default tab type_three without
   +    // the proper permissions.
   +    $assert_session->fieldNotExists('files[upload][]');
   +    $assert_session->pageTextNotContains('Maximum 2 files.');
   

   Wouldn't $assert_session->elementNotExists('css', 'media-library-add-form-selector') be a little more robust here?

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -628,18 +629,23 @@ public function testWidgetUpload() {
   +    // Assert the upload form is not visible for the non-file based media type
   +    // type_one.
   +    $page->clickLink('Type One');
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->fieldNotExists('files[upload][]');
   

   Same here?

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -431,6 +434,23 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
+  /**
+   * Get the media library state from the form state.
+   *
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The current form state.
+   *
+   * @return \Drupal\media_library\MediaLibraryState
+   *   The media library state.
+   */
+  protected function getMediaLibraryState(FormStateInterface $form_state) {

This needs a @throws clause to explain why the InvalidArgumentException is thrown.

My only other question here is this: I don't quite see how the tests are covering the regression we are trying to fix since this got reverted. As I understand it, the problem would manifest if you open the media library and immediately try to add media, without changing to another tab. Are our tests actually testing that interaction? I imagine that they are not, because if they were, we would presumably have caught this bug before the initial commit.

Okay. I feel pretty confident about this. The fail patch makes sense; @seanB showed me where we are changing the tests to ensure that an upload works even if you have not changed tabs (which is what manifested the AJAX bug we have subsequently fixed). Sean also tells me that we test what happens if, after that initial upload, you click to another tab and add more media (it's the part preceded by the comment Verify we can only upload the files allowed by the media type.).

RTBC when green.

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -658,16 +663,15 @@ public function testWidgetUpload() {
     // Assert we can upload a file to media type three.

I think it is worth adding a comment that we are testing uploading from the initial media library form before clicking on any tab. It'd be pretty easy to add this click back in and lose this test coverage.

Committed and pushed 05fa218275 to 8.8.x and b159f8162e to 8.7.x. Thanks!

Thanks for addressing the regression so quickly and new test coverage ftw.

For the benefit of any watchers, I've filed a follow-up bug which I believe was caused by this #3094059: The media library widget "Add media" button incorrectly submits form handlers of forms it is embedded in.