Allow newly uploaded files to be deleted from the media library without saving them

Tagging for screenshots. We will definitely need usability and accessibility review on this one.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -299,6 +314,23 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
+    $triggering_element = $form_state->getTriggeringElement();
+    $mid_slice = array_slice($triggering_element['#array_parents'], -2, 1);
+    $mid = reset($mid_slice);
+    $added_media = $form_state->get('media');
+    unset($added_media[$mid]);
+    $form_state->set('media', $added_media)->setRebuild();

Yikes. This needs a big comment.

Ret-titling for clarity.

RTL display points to a problem with the counter being two strings rather than one. However, that doesn't seem to be introduced here.

Otherwise the patch looks awesome. Just a few nits:

1.)

+   * The input element needs to have a submit handler to create media items from
+   * the user input and store them in the form state using
+   * ::processInputValues().

First line has 81 characters, maybe even them out.

+    // When the source field input contains errors, replace the existing form to
+    // let the user change the source field input. If the user input is valid,
+    // the entire modal is replaced with the second step of the form to show the
+    // form fields for each media item.

Same twice.

2.)

+    $media_items = $form_state->get('media');
+    $added_media = $form_state->get('media');
+    $added_media = $form_state->get('media');
+    $media_items = $form_state->get('media') ?: [];
+    $media_items = $form_state->get('media') ?: [];

Do we want to settle with one or the other variable name?

3.)

+    foreach ($media_items as $delta => $media) {
+    foreach ($media_items as $delta => $media) {
+    foreach ($media_items as $i => $media) {

$delta is so much more descriptive than the generic $i. You purged three or four $i keys, so how about bringing the last one in line, too?

4.)

+    $mids = array_map(function (MediaInterface $media) {
+      return $media->id();
+    }, $media_items);

Not your fault, but $mids is so confusing, because unlike $nids, $tids or $vids, it resembles an actual word. I was surprised to see our coding standard now allows camelCase variables, though not mixed. In a slight stretch of the coding standards, I'd propose $mIds for improved clarity, and clarity should IMHO always be the first goal.
If considered a new convention, this would have to be separately discussed in general (think this would improve readability for $nIds, $tIds or $vIds as well). Otherwise it might just be a solution for $mids.

As I'm not yet confident enough to catch everything, we should have another review. But I think this is very close to RTBC.

Indeed, I did. No problem, though. I was just a bit surprised where you took the whole amount of new code in a quite good condition... :)

Moved nitpick from #3023802-60: Show media add form directly on media type tab in media library:

There is an afterglow after hovering a delete "X" button that is larger than the "X" even after the "delete" hover text disappeared, see 00:18 or 00:50

@seanB's in #61:

the "glow" is showing where the focus is, and this is standard behavior. I think removing the focus styling from a button is an a11y issue.

I'm not saying we should remove the focus styling. Rather we should make sure the button is reset to its original "X" (without the "close" hover) before the "afterglow" kicks in.

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -179,6 +179,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    $parents = $form['#parents'];
   +    $id_suffix = $parents ? '-' . implode('-', $parents) : '';
   

   Is this really needed? I think the calling code sets #parents on the entity form element, so won't remove_button get a workable name generated automatically?

   If we _do_ need it, this should get a comment too.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -324,6 +343,26 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
   +    $delta_slice = array_slice($triggering_element['#array_parents'], -2, 1);
   +    $delta = reset($delta_slice);
   

   This can be one line:

   $delta = array_slice($triggering_element['#array_parents'], -2, 1)[0].

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -179,6 +179,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+    // We need to make sure each button has a unique name attribute. The default
+    // name for button elements is 'op'. If the name is not unique, the
+    // triggering element is not set correctly.

Can we add a @see referencing removeButtonSubmit()?

Apart from this, I think the code looks good to me.

This issue is a security and privacy improvement, because without it: say you accidentally upload a file into the media library that you shouldn't. Right now, you would have to add the file to your media library, and actually save it into the media library -- which means it would be visible to anyone who has permission to see the media library -- and you can't remove it without going to another screen and deleting the media item (which you might not have permission to do).

With this patch, you can remove the bad upload right away, without needing to save it into the media library (for all to see) first.

> This issue is a security and privacy improvement, because without it: say you accidentally upload a file into the media library that you shouldn't

Well. It would be more useful if we'd actually delete the file that you uploaded, but we don't. #2821423: Dealing with unexpected file deletion due to incorrect file usage. Maybe that whole topic should be moved under the media umbrella, so it receives some attention again? Working only through media entities vastly simplifies the possible file usage problems as it is really just tracking the media entity usage and it is left to the user if and when he wants to delete that.

Well. It would be more useful if we'd actually delete the file that you uploaded, but we don't.

True, but a cruft file that is marked temporary and not saved to the media library is, at least, not immediately visible to anyone who has permission to use the media library, and will be deleted later by cron (as far as I know).

Yes, it is not listed, that is true. *But* it is not marked as temporary with the default settings and will therefore also not be deleted. That was disabled by default in 8.4 or 8.3 and nobody touched the file usage issues anymore since that happened :)

I agree it would be ideal to avoid leaving the file languishing around, if we can. This could be a follow-up improvement though, if you feel it doesn't belong here.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -324,6 +347,25 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
+    unset($added_media[$delta]);

Sorry if I'm missing part of the story, but at this point isn't it safe enough to assume that the file $added_media[$delta] points to is not being used anywhere else (because it was just uploaded), and then we can just delete it right here?

This would avoid the need to track any usage at all, as long as this form is not used for in-line editing (which I don't think it will, but I could be wrong here).

Sorry if I was unclear, I did definitely not expect to solve that problem here, I'm just trying to bring some light into a rather dark corner of Drupal core that everyone is ignoring and that we should really improve (anyone trying to follow GDPR is going to have a problem with that eventually).

But yes, in this specific case, we can possibly just mark the file as temporary or even actually delete it directly.

Needs a reroll after #2996029: Add oEmbed support to the media library.

Rerolled patch #18 against the latest 8.7.x codebase.

@Berdir++ for drawing attention to #2821423: Dealing with unexpected file deletion due to incorrect file usage :) I agree with #23 that it doesn't belong here. Ideally, we could do what @Berdir suggests in #24. But … the good news that this is already happening: we can possibly just mark the file as temporary — this is already true :) The "removal" functionality is really only available immediately after creating media, BEFORE it is saved, so status=0 is by definition still true. So, AFAICT @Berdir's concerns are already addressed.
(I just tested it manually while reviewing the patch and was pleasantly surprised this just happens to already work as one would hope!)

1. 👎 Accessibility concern discovered during manual testing: I don't see any focus style for the "remove" button? For a non-visually-impaired but keyboard user, that's essential.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -372,6 +377,50 @@
   +.media-library-add-form .media-library-add-form__remove-button[type="submit"] {
   +  position: absolute;
   +  top: 25px;
   +  right: 6px; /* LTR */
   +  width: auto;
   +  margin: 0;
   +  padding: 2px 20px 2px 2px; /* LTR */
   +  text-transform: lowercase;
   +  color: transparent;
   +  border: 0;
   +  border-radius: 0;
   +  background: transparent url(../../../misc/icons/787878/ex.svg) right 2px no-repeat; /* LTR */
   +  font-weight: normal;
   +  line-height: 16px;
   +}
   

   🤔 This is setting a lot of stuff. I would like to see a review from @lauriii on this.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -179,6 +179,13 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // triggering element is not set correctly.
   

   🤔
   … and the wrong media item is removed.
   Right?

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -186,6 +193,22 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   +        // Prevent errors in other media items from preventing removal.
   

   🔍🐛 Nit: "Prevent … preventing …" is hard to read, how about "Ensure … preventing …"?

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -348,6 +390,14 @@ public function updateFormCallback(array &$form, FormStateInterface $form_state)
   +    // When the list of added media is empty, return to the media library.
   +    $added_media = $form_state->get('media');
   +    if (empty($added_media)) {
   +      $response = new AjaxResponse();
   +      $response->addCommand(new ReplaceCommand('#media-library-add-form-wrapper', $this->buildMediaLibraryUi()));
   +      return $response;
   +    }
   

   👍This is surprisingly elegant!

+1 on a front-end review from Lauri.

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -344,6 +345,10 @@
   +.media-library-add-form__media:first-child .media-library-add-form__remove-button[type="submit"] {
   +  top: 5px;
   +}
   

   I think this needs a comment.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -372,6 +377,54 @@
   +.media-library-add-form .media-library-add-form__remove-button[type="submit"] {
   

   Because we're using BEM in this style sheet, I think the selectors can simply be .media-library-add-form__remove-button, plus whatever modifiers are called for.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -179,6 +179,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // We need to make sure each button has a unique name attribute. The default
   +    // name for button elements is 'op'. If the name is not unique, the
   +    // triggering element is not set correctly and the wrong media item is
   +    // removed.
   +    // @see ::removeButtonSubmit()
   +    $parents = $form['#parents'];
   +    $id_suffix = $parents ? '-' . implode('-', $parents) : '';
   

   Thanks for this excellent and useful comment :)

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -326,6 +350,25 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
   +    $form_state->set('media', $added_media)->setRebuild();
   

   Question -- should we re-key the media items (i.e., array_values($added_media)) here?

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -768,6 +768,25 @@ public function testWidgetUpload() {
   +    // Assert removing an uploaded media item before save works as expected.
   +    $assert_session->elementExists('css', '.media-library-open-button[href*="field_unlimited_media"]')->click();
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->pageTextContains('Add or select media');
   +    $page->clickLink('Type Three');
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $page->attachFileToField('Add files', $this->container->get('file_system')->realpath($png_image->uri));
   +    $assert_session->assertWaitOnAjaxRequest();
   +    // Assert the media item fields are shown and the vertical tabs are no
   +    // longer shown.
   +    $assert_session->elementExists('css', '.media-library-add-form__fields');
   +    $assert_session->elementNotExists('css', '.media-library-menu');
   +    // Press the 'Remove button' and assert the user is sent back to the media
   +    // library.
   +    $assert_session->elementExists('css', '.media-library-add-form__remove-button')->click();
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->elementNotExists('css', '.media-library-add-form__fields');
   +    $assert_session->elementExists('css', '.media-library-menu');
   

   Should we also assert that the file entity itself is marked temporary?

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -381,25 +385,42 @@ public function removeButtonSubmit(array $form, FormStateInterface $form_state)
   +    $clicked_button = end($triggering_element['#array_parents']);
   +    if ($clicked_button === 'remove_button') {
   

   We don't need to store $clicked_button in its own variable.

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -785,6 +785,10 @@ public function testWidgetUpload() {
   +    $this->assertJsCondition($this->getSession()->evaluateScript('jQuery("#media-library-add-form-wrapper :tabbable:first").is(":focus")'));
   

   This is not a correct usage of assertJsCondition(); I think you're just supposed to pass the JavaScript directly to $this->assertJsCondition().

Kicking back for those things.

We demoed this to the UX team today, in the presence of the mighty @andrewmacpherson. Feedback was very positive and, with a few changes, we have sign-off from both the usability and accessibility angles.

• The only commit-blocking problem we turned up is the fact that, when you upload a file, the focus is immediately moved to the remove button. That's really dangerous from a usability standpoint. The solution we came up with is to move the Remove button after the required metadata fields, then style it so that it shows up in the top right. @andrewmacpherson confirmed that this doesn't really constitute a violation of the WCAG DOM-order provision because the design basically has three columns (thumbnail, fields, remove button), and the natural way to read that is to go from top-to-bottom in each column before advancing to the next one. This holds up even at smaller screen sizes.
• It's okay for us to keep the hover state and color change for the remove buttons, because it is still accessible to screen readers and people who cannot use pointer devices.
• It would be very helpful, though not a blocker if it's hard to do, for us to add a "foobar.jpg was deleted" message after an item is removed. If this is tricky or has special implications (and it might, since the media item might not have a name by the time it's removed), we can deal with it in a follow-up.

I think the path forward is clear; let's get this done!

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -231,6 +214,22 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
+      'remove_button' => [
+        '#type' => 'submit',
+        '#value' => $this->t('Remove'),
+        '#name' => 'media-' . $delta . '-remove-button' . $id_suffix,
+        '#attributes' => [
+          'class' => ['media-library-add-form__remove-button'],
+          'aria-label' => $this->t('Remove @label', ['@label' => $media->getName()]),
+        ],
+        '#ajax' => [
+          'callback' => '::updateFormCallback',
+          'wrapper' => 'media-library-add-form-wrapper',
+        ],
+        '#submit' => ['::removeButtonSubmit'],
+        // Ensure errors in other media items do not prevent removal.
+        '#limit_validation_errors' => [],
+      ],

To guarantee the correct ordering, maybe we should set explicit weights on the preview, remove_button, and fields sub-elements.

Interdiff looks good otherwise.

• The focus is still immediately moved to the remove button when uploading files to media types that don't contain additional fields.
• Wondering if this was given consideration before, but when a single file is being uploaded, the remove button is hard to distinguish from the close button since they look exactly the same.
  

• +++ b/core/modules/media_library/css/media_library.theme.css
  @@ -446,6 +453,55 @@
  +.media-library-add-form__remove-button[type="submit"] {
  ...
  +[dir="rtl"] .media-library-add-form__remove-button[type="submit"] {
  

  Let's add a todo to remove [type="submit"] from the selector when this is moved to Seven.

Skip the remove button and shift focus to the submit button.

I'm very much in favor of this option. It makes the most sense from a usability perspective, and if it doesn't compromise the accessibility of the Remove button(s), it's pretty much perfect.

@seanB and @phenaproxima demo'd this to me via screencast today to review accessibility concerns, and the functionality seems to pass accessibility checks with the current patch:

• Remove is now the last item in the DOM for each item wrapper, rather than the first
• The focus moves to the wrapper itself, which will receive an aria-label so that the screen reader user knows what they are encountering
• When focus lands on the remove item, the word remove becomes visible and it has its own focus state
• We also talked about adding aria-live="polite" so that the user is updated when the change occurs

A couple new questions:

1. One question I didn't think to review when we were discussing: where does the focus go after an item is removed? If it focuses on the next non-removed item in the list, or (in the event no items remain) the add media button, then this should be acceptable.
2. And another question that I now have from reviewing this issue and all of the comments after the demo: why was the outline removed from the remove item in the focus state? For keyboard only users, leaving the outline enabled would probably be preferable because the gray with the x for focused is subtle, and the user might not realize that this is the focused item. A combination of the color change + the outline would follow expectations and make the state easier to identify.

+++ b/core/modules/media_library/css/media_library.theme.css
@@ -104,13 +112,17 @@
+.media-library-add-form-description {
+  margin: 0;
+}

This seems like it should use BEM (media-library-add-form__description)...?

Nothing major found!

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -109,7 +110,7 @@ protected function getMediaType(FormStateInterface $form_state) {
   +    $form['#prefix'] = '<div id="media-library-add-form-wrapper" class="media-library-add-form-wrapper">';
   

   Should we add a todo here pointing out that we can remove the ID once the AJAX system allows targeting by selectors that aren't IDs? (With a @see to the issue.)

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -194,6 +210,7 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
          'preview' => [
            '#type' => 'container',
   +        '#weight' => 1,
   

   To allow extending classes to more easily insert things between these controls, I think the weights should be 10, 20, and 30 respectively.

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

   I don't think this needs the #media-library-add-form-wrapper prefix.

4. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -122,8 +122,16 @@ protected function buildInputElement(array $form, FormStateInterface $form_state
   +    // Add a container to group the input elements.
   +    $form['container'] = [
   +      '#type' => 'container',
   +      '#attributes' => [
   +        'class' => ['media-library-add-form-input-wrapper'],
   +      ],
   +    ];
   

   Should this class name be using BEM? (media-library-add-form__input-wrapper)?

   Also, I'm not really clear on why this container element is needed...if it's really needed, can the comment explain that better?

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -822,6 +822,33 @@ public function testWidgetUpload() {
   +    $page->clickLink('Type Three');
   

   Is this still necessary?

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

   I don't think this needs the #media-library-add-form-wrapper prefix.

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -822,6 +822,33 @@ public function testWidgetUpload() {
   +    $filename = $png_image->filename;
   +    $assert_session->pageTextContains("The media item $filename has been removed.");
   

   Nit: There is no need for $filename to be its own variable. The string can just be "The media item $png_image->filename has been removed."

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -921,6 +948,33 @@ public function testWidgetOEmbed() {
   +    // Assert removing an uploaded media item before save works as expected.
   

   Copypasta :) We're testing oEmbed here, so the item was not uploaded.

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -921,6 +948,33 @@ public function testWidgetOEmbed() {
   +    $this->assertJsCondition('jQuery("#media-library-add-form-wrapper .media-library-add-form__added-media").is(":focus")');
   

   #media-library-add-form-wrapper not needed here.

Okay! Thanks for fixing my feedback.

This has now been code reviewed multiple times, demoed to the UX team more than once (and approved), and been through an accessibility review thanks to @rainbreaw. It's a major UX improvement with (as the issue tags suggest) privacy implications, and there is extensive test coverage, so I see little reason to wait longer to land this one.

RTBC once green on all backends.

Thanks so much, @rainbreaw for jumping in here on the accessibility side! Adding credits for everyone here, plus the participants in the March 5 UX meeting https://www.youtube.com/watch?v=t_UVusCsurI

Sorry to bump this back to NW, but since this patch contains some duplicate changes with #3023797: Let users choose what to do after selecting and/or adding items in the media library, the feedback I posted there is relevant here as well.

Looks like Lauri's feedback is, indeed, addressed. I think we are good to go.

1. When uploading multiple files at once and you remove a media, the focus is shifted to removing the next item. We could avoid this, and make using this view more intuitive by making the list of files role="list" and the list items role="listitem. We should also add aria-label attribute to the list items, as well as tabindex="-1". This would allow us to focus on the previous media item after removing media from the list.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -84,17 +84,31 @@
   +  outline: none;
   

   I also couldn't figure out what is this needed for. I removed this and tested manually different possible scenarios and couldn't get the outline visible. If this is indeed needed, let's add some documentation here as well.

3. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -84,17 +84,31 @@
   + * need the outline.
   

   Could we include an explanation on why the outline isn't needed?

4. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -476,6 +491,12 @@
   +/* The first item doesn't have a top padding, change the location of the remove
   +   button as well. */
   
   @@ -504,6 +525,57 @@
   +/* @todo Remove [type="submit"] when styles are moved to the seven theme in
   +     https://www.drupal.org/project/drupal/issues/2980769 */
   

   Nitpick: let's format these according to our coding standards.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -139,6 +142,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +          // Add the tabindex '-1' to allow the focus to be shifted to the
   +          // container.
   

   I think this comment would be the most benefitial if we explained a scenario where the focus is shifted here (and why) 🙂

I gave this a quick manual test and found a little bug that needs fixing. Try this:

1. Upload multiple images at once.
2. Enter alt text for one of them (NOT the last one), then remove it (without saving).
3. You'll see that the one you removed is gone, but the alt text you entered is transferred into the one that came after it.

This won't be automatically testable since Mink doesn't support multi-file uploads. So I'm marking this for manual testing.

The good news is, I gave the widget a quick test using the keyboard. It seemed to work quite nicely -- the tab order made perfect sense. In addition to that, the interdiff makes sense; the added comments really do a lot to clarify some of the quirks of our implementation. So, thanks!

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -449,20 +449,24 @@ public function updateFormCallback(array &$form, FormStateInterface $form_state)
   +        $nth_child = 0;
   +        foreach ($added_media as $delta => $media) {
   +          $nth_child++;
   +          if ($delta > $removed_delta) {
   +            break;
   +          }
   

   Couldn't we just use max() here? (Since it's a variadic function, we'd probably need to do something like $nth_child = call_user_func_array('max', array_merge(array_keys($added_media), [$delta])) + 1;

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -848,6 +844,59 @@ public function testWidgetUpload() {
   +    $assert_session->assertWaitOnAjaxRequest();
   +    $assert_session->pageTextContains('Add or select media');
   

   This seems like a good place for $this->assertNotEmpty($assert_session->waitForText('Add or select media')).

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -848,6 +844,59 @@ public function testWidgetUpload() {
   +    // Set alt texts.
   +    $page->fillField('media[0][fields][field_media_test_image][0][alt]', $filenames[0]);
   +    $page->fillField('media[1][fields][field_media_test_image][0][alt]', $filenames[1]);
   +    $page->fillField('media[2][fields][field_media_test_image][0][alt]', $filenames[2]);
   +    // Remove the second file and assert the focus is shifted to the container
   +    // of the next media item and field values are still correct.
   +    $page->pressButton('media-1-remove-button');
   +    $this->assertJsCondition('jQuery(".media-library-add-form__media:nth-child(2)").is(":focus")');
   +    $assert_session->pageTextContains('The media item ' . $filenames[1] . ' has been removed.');
   +    $assert_session->fieldValueEquals('media[0][fields][name][0][value]', $filenames[0]);
   +    $assert_session->fieldValueEquals('media[0][fields][field_media_test_image][0][alt]', $filenames[0]);
   +    $assert_session->fieldNotExists('media[1][fields][name][0][value]');
   +    $assert_session->fieldNotExists('media[1][fields][field_media_test_image][0][alt]');
   +    $assert_session->fieldValueEquals('media[2][fields][name][0][value]', $filenames[2]);
   +    $assert_session->fieldValueEquals('media[2][fields][field_media_test_image][0][alt]', $filenames[2]);
   

   When I encountered this bug, I left one of the alt text fields empty. Can we do that here, just to verify that it works in that case (it will also serve as proof that the remove button does not try to validate the form).

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -848,6 +844,59 @@ public function testWidgetUpload() {
   +    $assert_session->pageTextContains('The media item ' . $filenames[2] . ' has been removed.');
   +    $assert_session->fieldValueEquals('media[0][fields][name][0][value]', $filenames[0]);
   +    $assert_session->fieldValueEquals('media[0][fields][field_media_test_image][0][alt]', $filenames[0]);
   +    $assert_session->fieldNotExists('media[1][fields][name][0][value]');
   +    $assert_session->fieldNotExists('media[1][fields][field_media_test_image][0][alt]');
   +    $assert_session->fieldNotExists('media[2][fields][name][0][value]');
   +    $assert_session->fieldNotExists('media[2][fields][field_media_test_image][0][alt]');
   

   This test uses a LOT of very long selectors, and that makes it hard for human brains to parse. I think we might want to do something like $first_media = $assert_session->elementExists('css', 'selector-for-first-media-item'), and then run further asserts on that (like $assert_session->fieldExists('Name', $first_media)). This might be a lot more readable.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -223,6 +223,10 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
+        // Add a data attribute containing the delta to allow us to easily shift
+        // the focus to a specific media item.
+        // @see ::updateFormCallback()
+        'data-media-library-added-delta' => $delta,

Even though this isn't strictly necessary for things to work, I think I like it. I think it increases clarity of the code, and improves future flexibility (and potentially themeability). +1!

I think these interdiffs look right to me, and we have put this patch through its paces enough times at this point. The privacy hole is filled -- if there are other bugs after this, we can deal with them in follow-up issues. Go team!

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -332,6 +389,33 @@ protected function prepareMediaEntityForSave(MediaInterface $media) {
   +    $form_state->set('media', $added_media)->setRebuild();
   

   would it be better to use array_values here instead so we've always got a sequential set of deltas and avoid doing it during submit - or does that mess with the parents logic?

2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -822,6 +818,87 @@ public function testWidgetUpload() {
   +    $this->assertJsCondition('jQuery("#media-library-add-form-wrapper :tabbable").is(":focus")');
   

   should we be checking this is the first tabbable element using .get(0)?

Superb, thanks all! I really like how this came together with feedback from all angles.

This issue was a big improvement for the usability of the Media widget! However, a vanilla installation will still leave the removed file for 6 hours before it is removed with a cron task. I think we can improve this situation by removing the file immediately when pressing the 'Remove' button. This option was actually already suggested by Berdir here in #24:

But yes, in this specific case, we can possibly just mark the file as temporary or even actually delete it directly.

To this end I have added a patch at #3073734: Immediately delete a media's file when pressing 'Remove' in the media creation modal to remove the file attachment immediately after clicking 'Remove' in the media creation modal.