Let users choose what to do after selecting and/or adding items in the media library

making it configurable seems like a good idea.

> You need extra clicks to upload new and reuse existing items at the same time

Seems like a < 10% (or less ;) use case to need a mix of existing and new media on the same field at the same time?

Adding #3009114: [Regression] Editing a media entity now always redirects you to a media overview list as related. I'm maintaining a site where the editors use the canonical view of media entities, and I've had to work-around the change that always forces them back to the media library after every edit. Please continue to consider this use case as you make changes / assumptions about how all this should work.

I know *this* issue is about "add" not "edit", and I'm okay with add forcing you back to the library (in their world, they add via an entity browser and IEF, so they never would hit this), but still...

Thanks,
-Derek

At @lauriii's behest, I need to cross-update this issue with some feedback discovered in #3023802-21: Show media add form directly on media type tab in media library, which was deferred to this issue to be actually solved.

Lauri found a few major UX problems with that patch:

1. Media entities selected prior to uploading new media file are lost. I would have expected those to remain attached.
2. I found the second step of the upload confusing; I wasn't sure if the file was already saved or not. This was relevant because I accidentally first uploaded a wrong file. I'm wondering also if the text on the submit button should be changed from "Save" to something like "Save and attach" to be more clear of the action.
3. There's no way to cancel upload without losing selected files in the media listing. This is again relevant because I uploaded a wrong file, and the only way to restart the process was to close the dialog.

@seanB replied in #3023802-22: Show media add form directly on media type tab in media library to say that all of those issues will (probably) be addressed by this issue. But I raise it here so that, whatever solution we come up with here, we remember to test all of these cases.

As per #3023802-28: Show media add form directly on media type tab in media library, this is now a follow-up for that issue based on feedback from the UX team.

Once it's in, this issue will:

add a second button the metadata screen, which will give the user two options (actual wording may vary): "Save and return to library", and "Save and insert selected items". ... As part of that work, we will add an area to the metadata screen which will display small thumbnails of all the items you have already selected from the library. This is important from both an accessibility and usability perspective. @ckrina has promised to create a design for this area.

Kicking this back for designs.

Sorry it took me longer than expected! I've been giving a thought to the current proposed solution with the selected items below in a grid and it's a really good and unobstructive way to solve it, so I don't think more design efforts to find something completely different are needed.

- But the items on the grid look huge and take a lot of space. Would it be possible to make them smaller? It's a problem for longer names, thought. But maybe the hover info and the possibility to review selection ("Save and select" button) would be enough to compensate?

- I feel the difference between the text for the 2 buttons is not big enough. I know it's too long, but I would expect to be able to deduce from the primary one that I'll "Save and select more items" or I'll "Save and go back to library" or I'll "Save and modify selection".

- Also, would it be possible/make sense to add a hover or active background when you're editing an item?

Filed the follow-up issues:

• #3034242: Hide "Save and insert" and "Additional selected media" from users by default
• #3034243: Improve selection text in Media Library
• #3034244: Add a cancel or back button to the media library

1. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -81,7 +82,8 @@
   -.media-library-view.view-display-id-widget .media-library-item__name a {
   +.media-library-view.view-display-id-widget .media-library-item__name a,
   +.media-library-add-form-selection .media-library-item__name a {
   

   I wonder why we wouldn't just reduce this selector to .media-library-item__name a?

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -96,6 +96,20 @@
   +.media-library-add-form-selection .media-library-item {
   +  width: 120px;
   +}
   +
   +.media-library-add-form-selection .media-library-item .field--name-thumbnail img {
   +  height: 100px;
   +}
   

   Should these maybe be max-width and max-height?

3. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -33,4 +33,44 @@
   +  Drupal.behaviors.ClickToSelectHover = {
   +    attach(context) {
   +      $('.js-click-to-select-trigger, .js-click-to-select-checkbox', context)
   +        .once('media-library-item-hover')
   +        .on('mouseover mouseout', ({ currentTarget, type }) => {
   +          $(currentTarget)
   +            .closest('.media-library-item')
   +            .toggleClass('is-hover', type === 'mouseover');
   +        });
   +    },
   +  };
   +
   +  /**
   +   * Adds focus effect to media items.
   +   *
   +   * @type {Drupal~behavior}
   +   *
   +   * @prop {Drupal~behaviorAttach} attach
   +   *   Attaches behavior to add a focus effect to media items.
   +   */
   +  Drupal.behaviors.ClickToSelectFocus = {
   +    attach(context) {
   +      $('.js-click-to-select-checkbox input', context)
   +        .once('media-library-item-focus')
   +        .on('focus blur', ({ currentTarget, type }) => {
   +          $(currentTarget)
   +            .closest('.media-library-item')
   +            .toggleClass('is-focus', type === 'focus');
   +        });
   +    },
   +  };
   

   I think these should both be added to the main click-to-select behavior, if it can be done generically, rather than be implemented as additional behaviors.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -123,6 +126,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        $form['#attributes']['class'][] = 'media-library-add-form';
   +    $form['#attributes']['class'][] = 'js-media-library-add-form';
   

   These can be combined into a single line. Also, I don't understand why we're using the same class name twice, just prefixed with 'js-' the second time. That could maybe use a comment.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -139,8 +144,22 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $form['selection'] = $this->buildSelection($form, $form_state);
   

   I think this should be renamed buildCurrentSelectionArea().

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,6 +266,67 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +    $media_ids = array_filter(explode(',', $form_state->getValue('current_selection')));
   

   I think we should pass the array of loaded media items into this method directly, as the first argument: buildCurrentSelectionArea($media_items, $form, $form_state).

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,6 +266,67 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +      $selection[$media_id] = [
   +        '#type' => 'container',
   +        '#attributes' => [
   +          'class' => [
   +            'media-library-item',
   +            'js-media-library-item',
   +            'js-click-to-select',
   +          ],
   +        ],
   +        'select' => [
   +          '#type' => 'container',
   +          '#attributes' => [
   +            'class' => [
   +              'js-click-to-select-checkbox',
   +            ],
   +          ],
   +          'select_checkbox' => [
   +            '#type' => 'checkbox',
   +            '#title' => $this->t('Select @name', ['@name' => $media->label()]),
   +            '#title_display' => 'invisible',
   +            '#return_value' => $media_id,
   +          ],
   +        ],
   +        'rendered_entity' => $view_builder->view($media, 'media_library'),
   +      ];
   

   This should all be moved into a single protected method, for easy overriding. Something like $this->buildSelectedItemElement($media_item);

8. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -429,6 +518,41 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +    $selected_media_ids = array_filter(explode(',', $form_state->getValue('current_selection')));
   +    $added_media = $form_state->get('media') ?: [];
   +    $added_media_ids = array_map(function (MediaInterface $media) {
   +      return $media->id();
   +    }, $added_media);
   +    $media_ids = array_merge($selected_media_ids, $added_media_ids);
   

   I think this should be moved to a new protected method. Something like $this->getAllSelectedMediaIds($form_state).

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -139,8 +154,27 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Allow the current selection to be set in a hidden field so the selection
   +    // can be passed between different states of the form.
   +    $form['current_selection'] = [
   

   Can we add a @see here referencing the relevant JavaScript?

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,6 +281,71 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +    $selection = [
   +      '#type' => 'container',
   +      '#attributes' => [
   +        'class' => [
   +          'media-library-add-form-selection',
   +        ],
   +      ],
   +    ];
   

   I wonder if this should be an open details element, so that users can collapse it if they want to. (Assuming such a thing would be kosher from an accessibility standpoint.)

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,6 +281,71 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +      $selection[$media_id] = $this->buildSelectedItemElement($media);
   

   I wonder if we should also pass $form and $form_state to this method?

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,6 +281,71 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +  protected function buildSelectedItemElement($media) {
   

   $media needs to be type hinted.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -429,6 +537,33 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +   *   The form array when there are form errors or a AJAX response to select
   

   Should be "The form array if there are validation errors, or an AJAX response to add the created items to the current selection."

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -444,4 +579,24 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +  protected function getAllSelectedMediaIds(array &$form, FormStateInterface $form_state) {
   

   $form isn't used for anything, so I think we can remove it.

7. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -176,6 +176,7 @@ public function validateUploadElement(array $element, FormStateInterface $form_s
        $element['upload_button']['#submit'] = ['::uploadButtonSubmit'];
   +    $element['upload_button']['#limit_validation_errors'] = FALSE;
   

   This seems questionable; won't this disable validation for the upload entirely? Will extensions, sizes, etc. still be checked? If they will, we have tests to assert that, right?

point 3 in #21-22

I don't really follow what this issue is about, however if you are changing JS code which involves focus, please tag for an accessibility review.

I think this looks like re-arranging code to be neater, rather than intending to change any behaviour, is that right?

All the same, in the past we have had some very serious keyboard accessibility regressions from well-meaning refactorings, and keyboard accessibilty has been broken for many releases. Eventually I'd like to get more functional javascript tests in place for keyboard behahiours, but for the time being please request an accessibility review if messing with focus in JS.

Applying a class when focus happens is something that could still potentially fail WCAG "focus visible" (at level AA) if it broke, even though it still passed keyboard-operable at level A. Hence worth reviewing.

Tagging for front-end framework manager review due to CSS and JavaScript changes.

1. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -24,12 +24,29 @@
   +        // Adds focus effect to media items.
   +        .on('focus blur', ({ currentTarget, type }) => {
   +          $(currentTarget)
   +            .closest('.media-library-item')
   +            .toggleClass('is-focus', type === 'focus');
   +        });
   

   So, question -- in this event handler, we are targeting the closest .media-library-item, but in the 'change' handler, we're targeting the closest .js-click-to-select. Surely we want to make those the same? If not, can we add a comment explaining why?

2. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -24,12 +24,29 @@
   +      $('.js-click-to-select-trigger, .js-click-to-select-checkbox', context)
   +        .once('media-library-item-hover')
   +        .on('mouseover mouseout', ({ currentTarget, type }) => {
   +          $(currentTarget)
   +            .closest('.media-library-item')
   +            .toggleClass('is-hover', type === 'mouseover');
   

   Same here. The click-to-select stuff was originally meant to be generic, so why are we targeting the closest .media-library-item? Can we use a more generic selector?

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -139,8 +154,28 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      $media_ids = array_filter(explode(',', $form_state->getValue('current_selection')));
   

   I think we should also put this into its own method, and have getAllSelectedMediaIds() call it. That way, we can confine all the somewhat fiddly $form_state stuff to individual methods.

   So my suggestion for the method names are getPreSelectedMediaItems() -- to get the stuff that was already selected before the add form was used -- and getAllCurrentMediaItems(), which returns the pre-selected ones _and_ the ones being added by the form. Both of these methods should return an array of fully loaded media entities, keyed by ID.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,6 +282,75 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +  protected function buildCurrentSelectionArea($media_items, array $form, FormStateInterface $form_state) {
   

   If we follow my previous suggestion, we will no longer need to pass $media_items here; this method can just call $this->getPreSelectedMediaItems($form_state) and loop over them.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -444,4 +584,22 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +  protected function getAllSelectedMediaIds(FormStateInterface $form_state) {
   

   If my renaming suggestion is followed, this will become getCurrentMediaItems(). We'll also need to change the doc comment to account for the fact that this will return the combination of the pre-selected ones and the added ones.

6. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -176,6 +176,14 @@ public function validateUploadElement(array $element, FormStateInterface $form_s
   +    // Limit the validation errors to make sure
   +    // FormValidator::handleErrorsWithLimitedValidation doesn't remove the
   +    // current selection from the form state.
   +    // @see Drupal\Core\Form\FormValidator::handleErrorsWithLimitedValidation()
   +    $element['upload_button']['#limit_validation_errors'] = [
   +      ['upload'],
   +      ['current_selection'],
   +    ];
   

   Very nice! This is a lot clearer.

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -606,36 +604,31 @@ public function testWidgetUpload() {
   +    $assert_session->elementNotExists('css', '.media-library-add-form-selection');
   

   I think we can use $assert_session->hiddenFieldNotExists() here.

+++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
@@ -24,12 +24,29 @@
+            .closest('.media-library-item')
...
+            .closest('.media-library-item')

We should use a js- prefixed class here.

Other than that, the CSS and the JS changes look fine to me so I'm removing the tag.

This is a big UX improvement and makes the current state clearer. I feel like we could be even clearer about the state on this screen when user has pre-selected media. The problem with this screen is that it combines entities that haven't been saved with entities that have been saved without a clear distinction. When I saw the UX improvements, I checked again if the functionality had been changed so that the media was already saved at that stage since it was unclear to me what the state was. Potential solutions to this could be a title that describes the purpose of that screen, or a message / description with instructions.

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -146,20 +146,23 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        '#value' => $this->formatPlural(count($added_media), 'The media item has been created. Fill in any required fields and save to make it available in the media library.', 'The media items have been added. Fill in any required fields and save to make them available in the media library.'),
   

   Why does the singular string say "created", but the plural string say "added"?

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -585,21 +593,38 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +   * @return |Drupal\media\MediaInterface[]
   +   *   An array containing the pre-selected media IDs.
   +   */
   +  protected function getPreSelectedMediaItems(FormStateInterface $form_state) {
   

   There's a rogue | here, and the comment is not accurate -- this is returning media entities keyed by ID, not IDs only.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -585,21 +593,38 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +  protected function getCurrentMediaItems(FormStateInterface $form_state) {
   

   I think this should be returning media entities too, not just IDs.

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -149,7 +149,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        '#value' => $this->formatPlural(count($added_media), 'The media item has been created. Fill in any required fields and save to make it available in the media library.', 'The media items have been created. Fill in any required fields and save to make them available in the media library.'),
   

   "Make them available in" is a bit unwieldy. I think we could just say "...to add it/them to the media library."

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -570,8 +570,11 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +      $current_media_ids = array_map(function (MediaInterface $media) {
   +        return $media->id();
   +      }, $this->getCurrentMediaItems($form_state));
   

   This could be a simple array_keys() call if getCurrentMediaItems() returned the media items keyed by their ID. Is that possible?

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -598,8 +601,8 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +   * @return \Drupal\media\MediaInterface[]
   +   *   An array containing the pre-selected media items.
       */
      protected function getPreSelectedMediaItems(FormStateInterface $form_state) {
   

   Let's return them keyed by their ID, and mention that in the doc comment.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -615,16 +618,13 @@ protected function getPreSelectedMediaItems(FormStateInterface $form_state) {
   +   * @return \Drupal\media\MediaInterface[]
   +   *   An array containing all pre-selected and added media items.
       */
      protected function getCurrentMediaItems(FormStateInterface $form_state) {
   

   Same here.

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -570,6 +570,9 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +      // The added media items get an ID  when they are saved in ::submitForm().
   

   This is the nittiest nitpick of all freaking time: There is an extra space after "ID" which shouldn't be there.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -619,7 +622,9 @@ protected function getPreSelectedMediaItems(FormStateInterface $form_state) {
   +   *   An array containing all pre-selected and added media items. The
   +   *   pre-selected media items are keyed by ID and the added items are keyed by
   +   *   delta.
       */
      protected function getCurrentMediaItems(FormStateInterface $form_state) {
   

   So, I don't think we should combine IDs and deltas. It really needs to be one or the other. The scenario I'm envisioning is this: you pre-select a media item whose ID is 1, and you create two media items, the second of which will have the delta of 1. Which will win? Something will get clobbered, or at the very least, this method becomes harder to read and understand.

   I suggest that getPreSelectedMediaItems() always key by ID, and getCurrentMediaItems() is a simple numerically indexed array -- it should not key by either ID or delta. If someone calls getCurrentMediaItems(), they can filter out the pre-selected or created ones by calling $media->isNew().

   The doc comments of each method should make clear how each method keys its return value.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -247,6 +285,80 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
+  /**
+   * Returns a render array containing the current selection.
+   *
+   * @param \Drupal\media\MediaInterface $media
+   *   The media item.
+   * @param array $form
+   *   The complete form.
+   * @param \Drupal\Core\Form\FormStateInterface $form_state
+   *   The current form state.
+   *
+   * @return array
+   *   A render array of a selected media item.
+   */
+  protected function buildSelectedItemElement(MediaInterface $media, array $form, FormStateInterface $form_state) {

The doc comment is not accurate. It should be "Returns a render array for a single pre-selected media item."

That's all I found. I think, pending accessibility sign-off, this is ready.

Read the interdiff. Extra test coverage is always good!

From my perspective, this is RTBC once @andrewmacpherson signs off.

seanB and phenaproxima demo'd this with me for accessibility review today, as well. What is working well:

• The DOM is well ordered and the screen reader will announce the move from element to element.
• The keyboard navigates between all items in a logical flow, including properly understanding the use of the space bar to select or deselect the checkboxes on items in the previously selected section

One suggestion that I made:

Include aria-labels on the wrappers that will receive focus so that the user understands the element they are on. This means that the area where media is being added would need an aria-label presenting the section as "Add new media" or "Media to add," while the div containing the pre-selected items would need an aria-label presenting the section as "Preselected Media."

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
@@ -670,7 +670,9 @@ public function testWidgetUpload() {
+    $this->assertSame('Added media items', $assert_session->elementExists('css', '.media-library-add-form-media')->getAttribute('aria-label'));

This seems like a good place for $assert_session->elementAttributeContains().

Otherwise, I think this looks pretty damn good. RTBC once that's done.

Thanks!

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

Let's retitle to reflect what actually happened here and any updates to the issue summary needed, so it makes sense in a commit log, etc :)

How's this for a re-title?

Found nitpicks, but overall this looks pretty good. I think we need a demo video, though -- or at least screenshots -- demonstrating the functionality at different screen sizes.

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -321,7 +360,7 @@
   -.media-library-item--grid.checked {
   +.media-library-item--grid.checked:before {
      border-color: #0076c0;
    }
   

   Why did this change?

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -139,14 +154,47 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Allow the current selection to be set in a hidden field so the selection
   +    // can be passed between different states of the form.
   +    // @see Drupal.behaviors.MediaLibraryItemSelection
   +    $form['current_selection'] = [
   +      '#type' => 'hidden',
   +      '#default_value' => '',
   +      '#attributes' => [
   +        'class' => [
   +          'js-media-library-add-form-current-selection',
   +        ],
   +      ],
   +    ];
   

   The comment should explain why no default value is set here, and mention how/where the value is updated.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -253,6 +301,82 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +      'select' => [
   +        '#type' => 'container',
   +        '#attributes' => [
   +          'class' => [
   +            'js-click-to-select-checkbox',
   +          ],
   +        ],
   +        'select_checkbox' => [
   +          '#type' => 'checkbox',
   +          '#title' => $this->t('Select @name', ['@name' => $media->label()]),
   +          '#title_display' => 'invisible',
   +          '#return_value' => $media->id(),
   +        ],
   +      ],
   

   Not a big deal, but I think this could potentially be refactored to use #theme_wrappers to create the container around it. It might make this code a bit more succinct.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -267,9 +391,18 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +      'save_submit' => [
   +        '#type' => 'submit',
   +        '#button_type' => 'primary',
   +        '#value' => $this->t('Save and select'),
   +        '#ajax' => [
   +          'callback' => '::updateLibrary',
   +          'wrapper' => 'media-library-add-form-wrapper',
   +        ],
   +      ],
   +      'insert_submit' => [
            '#type' => 'submit',
   

   I don't know if we need the '_submit' suffix for these buttons, or the word 'save' (both operations save the media items, after all). 'select' and 'insert' might be clearer.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -344,25 +477,31 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
   +    $response->addCommand(new InvokeCommand('#media-library-add-form-wrapper .media-library-add-form__added-media', 'focus'));
   

   Not sure we need the '#media-library-add-form-wrapper' part of this selector.

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -472,4 +642,51 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +    $pre_selected_media = $this->getPreSelectedMediaItems($form_state);
   +    $added_media = $form_state->get('media') ?: [];
   +    return array_merge($pre_selected_media, $added_media);
   

   $pre_selected_media_items are keyed by ID, so we probably need to do array_merge(array_values($pre_selected_media), $added_media). Additionally, a comment mentioning why we're doing that might be useful.

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -676,38 +676,38 @@ public function testWidgetUpload() {
   +    $this->assertJsCondition('jQuery("#media-library-add-form-wrapper .media-library-add-form__added-media").is(":focus")');
   

   I don't think we need #media-library-add-form-wrapper in this selector.

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -759,69 +782,101 @@ public function testWidgetUpload() {
   +    $media_storage = $this->container->get('entity_type.manager')->getStorage('media');
   +    $media_items = $media_storage->loadMultiple();
   

   Nit: Let's just use Media::loadMultiple() here.

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -906,21 +963,101 @@ public function testWidgetOEmbed() {
   +    $media_storage = $this->container->get('entity_type.manager')->getStorage('media');
   +    $media_items = $media_storage->loadMultiple();
   

   Same here.

@seanB and I demoed this patch to the UX team again today. @jhodgdon was there and provided some very interesting feedback, from which we have some good news and some bad news.

The good news is that this patch is not far from done. The bad news is that the UI is still somewhat confusing -- the separation of newly added media items vs. preselected items isn't very clear. So we need to improve that. We decided to do two things:

1. We need to slightly improve the text above the new media items so that it's clear that they haven't been saved yet, and will be as soon as the required fields are filled in.
2. We need to make it more obvious that the pre-selected items are a secondary concern. To do this, we will wrap them in a collapsed details element with a heading like "Additionally selected items".

These changes are going to necessitate another round of UX and accessibility review, but once we have that, I think we can get this in. So let's do that.

1. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -81,7 +82,8 @@
   +.media-library-view--widget .media-library-item__name a,
   +.media-library-add-form__selected-media .media-library-item__name a {
      pointer-events: none;
    }
   

   Using pointer-events: none; isn't a sufficient way to disable links since it only works for pointer devices. The link is still considered as a link for other input methods, such as screen readers.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -95,6 +95,10 @@
   +.media-library-add-form__added-media {
   +  outline: none;
   +}
   
   +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -139,14 +154,48 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          'tabindex' => -1,
   

   What do we need this for? Especially given that we later remove the outline.

#65.1 +1 for not fixing it here!
#65.2 Thanks for the explanation. I can see why it's needed now. Could we add documentation that explains this in the CSS and in the render array? This would make it easier for anyone doing maintenance to understand why this is needed.

+++ b/core/modules/media_library/css/media_library.theme.css
@@ -129,6 +137,20 @@
+.media-library-add-form__selected-media .media-library-item .field--name-thumbnail img {

@@ -263,16 +285,26 @@
+.media-library-add-form__selected-media .media-library-item--grid {
...
+  .media-library-add-form__selected-media .media-library-item--grid {

@@ -280,23 +312,33 @@
+  .media-library-add-form__selected-media .media-library-item--grid {
...
+  .media-library-add-form__selected-media .media-library-item--grid {

Could we add another variation to the .media-library-item element? Something like .media-library-item--small?

@seanB and @phenaproxima demo'd this with me this morning:

Making the label of the selected media section visible to all and collapsing it is a great solution! This looks good from UX standpoint. We verified the following a11y considerations:

1. The label (which is a button) is read by the screen reader
2. The button is set to aria-expanded="false" when the section is collapsed
3. The button resets to aria-expanded="true" when the section is expanded
4. The tab functionality progresses in the expected order, from the add new media form, to the selected media section, to the items in the selected media, where they can be toggled with the expected keyboard commands and are read as expected

I raised the same concern that has apparently been raised a few times about the selection text (x of y selected) from issue #3034243: Improve selection text in Media Library and will add a separate comment on that related issue.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -422,6 +422,9 @@ protected function buildSelectedItemElement(MediaInterface $media, array $form,
+          // Force disable the checkbox. It should only be enabled via
+          // JavaScript.
+          '#value' => FALSE,

After a quick chat with Sean, I understand what we're doing here. The actual selected media IDs are compiled by JavaScript into a hidden field, and that is submitted to the server; ever thus has it been since we rebuilt the media library UI in 8.7.x. From a form processing standpoint, these checkboxes are purely decorative (although they serve an important purpose for usability and accessibility). So setting #value => FALSE is okay here, but let's have a comment explaining why we do it.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -422,8 +422,11 @@ protected function buildSelectedItemElement(MediaInterface $media, array $form,
+          // The checkbox’s value is never processed by this form. It is present
+          // for usability and accessibility reasons, and only used by
+          // JavaScript to track whether or not this media item is selected. The
+          // hidden ‘current_selection’ field is used to store the actual IDs of
+          // selected media items.

Nit: These are "smart" apostrophes. I wonder if they should be ASCII?

@SeanB walked me through this patch today, we tested all of the interactions (WebchickTestCase FTW ;)):

- Inserting Media directly from the library
- Selecting Media and adding new at the same time.
- Only selecting new files
- Inserting new media into the library
- Inserting new media directly into the post
- Canceling out and inserting nothing
- Deleting new media before it's uploaded
- Unselecting existing media to remove it during the interstitial

Here are some updated screenshots (also updated in issue summary) of how it's working now.

Just adding new files:

Adding new files, along with some already existing ones selected (details collapsed by default):

And expanded:

I can confirm addresses all of the (great) feedback in UX call last week. Therefore, removing the "Needs usability review" tag.

Way to knock it out of the park!! :D SO excited for this patch to land!!

Oh, one thing we could use a "normal"-level follow-up for (if one doesn't already exist), and do some "real world" asking of users at DrupalCon...

If you select, say, 3 media, then go back and select, say, 2 media... the media library will only show you the two you just selected, not the 5 you've selected altogether. My spidey sense says that might be confusing for people, and could also cause them to accidentally select the same image twice, since they can't see the already-inserted images when the dialog is up in their face. (In fairness, @SeanB did point out that there could be a use case for inserting the same media item twice, such as filling up a carousel, or promoting a particular ad multiple times... but it feels like it's more often you'd do this by accident, and maybe we move the ability to select an image twice to a contrib module for those who need it).

Random small nits. Please send this issue directly to RTBC once these are addressed.

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -504,7 +542,7 @@
    .media-library-add-form__media {
      position: relative;
      display: flex;
   -  padding: 20px 0 20px 0;
   +  padding: 1em 0 1em 0;
   

   Nit: We can use the shorthand 1em 0 here.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -309,6 +350,90 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +          // The checkbox’s value is never processed by this form. It is present
   

   "checkbox's" is still using a smart apostrophe.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -572,6 +707,39 @@ protected function buildMediaLibraryUi(FormStateInterface $form_state) {
   +        ->addCommand(new InvokeCommand("[data-media-library-widget-value=\"$field_id\"]", 'val', [implode(',', $current_media_ids)]))
   +        ->addCommand(new InvokeCommand("[data-media-library-widget-update=\"$field_id\"]", 'trigger', ['mousedown']))
   

   I kinda wish that these things were their own AJAX command. However, we can leave that for another time.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -607,4 +775,52 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +    $media_ids = $form_state->get('current_selection');
   

   Can we add a @see, referencing where in this class the current_selection value is added to $form_state?

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -607,4 +775,52 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
   +    $pre_selected_media = $this->getPreSelectedMediaItems($form_state);
   +    $added_media = $form_state->get('media') ?: [];
   +    // Using array_merge will renumber the numeric keys.
   +    return array_merge($pre_selected_media, $added_media);
   

   This function could be reduced to one line: return array_merge($this->getPreSelectedMediaItems($form_state), $this->getAddedMediaItems($form_state));. Obviously we'd want to keep the comment; :)

Opened #3041147: Preserve the selected items across multiple uses of the media library to discuss @webchick's spidey-sense from #78.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -785,6 +785,8 @@ protected function getSourceFieldName(MediaTypeInterface $media_type) {
+    // Get the current select from the form state.

This needs to be "Get the current selection..."

I think the way it is right now is a lot easier to read and understand. Can we keep it?

We're duplicating the exact logic of getAddedMediaItems() -- we shouldn't do that. We can keep it on three lines, but let's at least call getAddedMediaItems().

Thanks. This is beautiful. Completely RTBC once green.

Ok, @seanB and @phenaproxima walked me through the patch today. Front-end framework manager review tag was already removed and @lauriii's feedback looks to be addressed, mostly skimmed over the CSS/JS parts. (A lot of the JS changes are related to consolidating logic since the display logic is now called in two places.)

+    // Allow the current selection to be set in a hidden field so the selection
+    // can be passed between different states of the form. This filled is filled
+    // via JavaScript so the default value should be empty.

(nitpick) "filled is filled" ;) Should be "field is filled" ... fixed on commit.

+    // Allow the current selection to be set in a hidden field so the selection
+    // can be passed between different states of the form. This filled is filled
+    // via JavaScript so the default value should be empty.
+    // @see Drupal.behaviors.MediaLibraryItemSelection
+    $form['current_selection'] = [
+      '#type' => 'hidden',
+      '#default_value' => '',
+      '#attributes' => [
+        'class' => [
+          'js-media-library-add-form-current-selection',
+        ],
+      ],
+    ];

We spent some time on this, since normally these types of internal properties would be passed around via #type => value, but in this case we need them exposed in the source so the JS can manipulate them. There should be no security issue here (e.g. concerns with value tampering), because basically access to media items is currently "all or nothing" ... if you can see the media library at all, you can see everything in it. However, something to add explicit test coverage for once we get to more access control stuff.

+        'select_checkbox' => [
+          '#type' => 'checkbox',
+          '#title' => $this->t('Select @name', ['@name' => $media->label()]),
+          '#title_display' => 'invisible',
+          '#return_value' => $media->id(),
+          // The checkbox's value is never processed by this form. It is present
+          // for usability and accessibility reasons, and only used by
+          // JavaScript to track whether or not this media item is selected. The
+          // hidden 'current_selection' field is used to store the actual IDs of
+          // selected media items.
+          '#value' => FALSE,
+        ],
+      ],

The extra comments here were helpful, because at a glance it seems like this would be an error.

   public function processUploadElement(array $element, FormStateInterface $form_state) {
     $element['upload_button']['#submit'] = ['::uploadButtonSubmit'];
+    // Limit the validation errors to make sure
+    // FormValidator::handleErrorsWithLimitedValidation doesn't remove the
+    // current selection from the form state.
+    // @see Drupal\Core\Form\FormValidator::handleErrorsWithLimitedValidation()
+    $element['upload_button']['#limit_validation_errors'] = [
+      ['upload'],
+      ['current_selection'],
+    ];

Also spent a bit of time here, since #limit_validation_errors is normally one of those "eeek" properties. In this case we are oddly using it to include more data than would otherwise happen by default, in order to preserve the keys. Unconventional, but it works, and it's commented as such.

KUDOS for all of the great test coverage!! Basically everything enumerated in #77 is covered, in addition to some other things (like multiple file uploads, testing the oEmbed flow as well, etc.)

Not the fault of this patch, but the "type Three / type Five" stuff in tests would be vastly helped by using actual names of things (such as the default media types in Standard profile), vs. placeholder names like this. :P Follow-up material, however.

Overall, all of my potential concerns were addressed, the flow works, the a11y team and frontend framework managers signed off, and it's the last big UX patch the Media team was targeting for 8.7... LET'S DO IT!

Adding credit for people from recent UX meetings who have helped review this functionality. And actually removing the tag I meant to back in #77. :P

Committed and pushed to 8.8! WOOHOO!! And, because this code is against an experimental module, also backported to 8.7.x.

GREAT work, everyone!!