Media library widget counter does not reset when removing and re-adding media

Updated title and summary based on my testing, I was only able to reproduce this when adding, removing and re-adding an image.

Write the patch, please review.

@flocondetoile ah ok - that is why it is very helpful to include steps to reproduce.

Can you post specific steps to reproduce the original issue?

Do you think it’s possible to fix both at once or should it be two different issues?

Tried to reproduce the mentioned issue but its working fine for unlimited cardinality. I have found only one discrepancy in count message for limited cardinality, If we already select any media and upload a new file from the add file section then the message is showing like this "2 of 1 item selected".

I think the issue https://www.drupal.org/project/drupal/issues/3092536 will take care of it. 

This bug definitely still occurs in 9.0.3 as described in my steps - only when you have selected an item already and removed it (for fields with 1 allowed item), or when you have selected an item and open the modal to select another one (for fields with multiple or unlimited).

I have updated the issue summary to reflect this.

@phenaproxima Write the patch, Please review.

+++ b/core/modules/media_library/js/media_library.ui.js
@@ -147,8 +147,8 @@
+        var selectItemsText = remaining < 0 ? Drupal.formatPlural(Drupal.MediaLibrary.currentSelection.length, '1 item selected', '@count items selected') : Drupal.formatPlural(remaining, '@selected of @count item selected', '@selected of @count items selected', {

The change looks straightforward. Maybe we should remove the line: const currentSelection = Drupal.MediaLibrary.currentSelection; entirely and always directly use Drupal.MediaLibrary.currentSelection.

Btw, this needs to be changed in the media_library.ui.es6.js file as well and then compiled as explained in https://www.drupal.org/docs/frontend-developer-tools-for-drupal-core.

Last but not least, can we also add a test (and a fail patch) to show the issue and prove the fix to make sure we don't have regressions later on?

Made the changes requested in #17. Please review.

Looks good, nice work! Only thing we have to add is a test and a test-only/fail patch to prove this fixes the issue. Easiest might be to add a couple of lines somewhere in Drupal\Tests\media_library\FunctionalJavascript\WidgetUploadTest.

Maybe an extra assert right after this?

    // Remove the item.
    $assert_session->elementExists('css', '.field--name-field-twin-media')->pressButton('Remove');
    $this->waitForNoText($png_image->filename);

    $this->openMediaLibraryForField('field_twin_media');

Like:

    // Remove the item.
    $assert_session->elementExists('css', '.field--name-field-twin-media')->pressButton('Remove');
    $this->waitForNoText($png_image->filename);

    $this->openMediaLibraryForField('field_twin_media');
    $assert_session->pageTextContains('0 of 2 items selected');

@seanB test added advised in #19, Please review.

Added test only patch, mentioned in #19

That is good enough for me, thank you. RTBC.

Hello @all,

I tested patch #20 and it is working as expected.
I attached the same patch again because the last patch from the issue is the test-fail one and if patch #20 will be committed, it needs to be last patch inserted in the issue.
Next time you can attach the test fail patch and the solution patch together in the same comment @vsujeetkumar :)

RTBC+1

Cheers, Paulo.

I have checked the above issue with patch at #20. It is working fine to address the media counter issue, however I found a small glitch:

When we click the "Insert Selected" button without selecting any media the popup closes (which is fine), but on reopening the media popup, it displays an error message as "No items selected" which is confusing.

Ideally in this scenario I believe the media popup should not close and must display an error message.

Attaching a screenshot for reference .

Not consider this patch and the interdiff.
Hello,
This error is not occurring because of patch #20.
I added a patch that disables the button while no media is selected.

Cheers, Paulo.

Patch #27 and interdiff are wrong.

Please consider what I attached in this comment.
This patch disables the "Insert selected" button while no media is selected.

Thanks.

This is not working as expected , the issue still persist.

Hello @janmejaig,

what are the steps that you did to test the patch?

I'm asking because I saw the image that you attached and no file is selected and the button "Insert selected" is not disabled.
Did you clear your cache after you applied it?

Cheers, Paulo.

Hello @paulocs ,
I have simply applied the patch at #28, after applying the patch I cleared the cache and hard refreshed the browser and progressed as per #25.
Hope I clarified your query , do let me know if anything specific I am missing here.

Here's a stab at backporting this to Drupal 8.9.x.

Not sure why #29 indicated this NW. With this patch, media library is working properly for us. The "Insert selected" button is only enabled when an actual selection is made -- even if a selection is made, the library is closed, and then the library is re-opened.

Setting to NR for additional confirmations just in case.

Rerolled as patch #29 for 9.2.x

Applied patch #36 and it is not fixing the problem.The 'No items selected' error still appears after applying this patch.
Adding recording

@Abhijith S, thank you for the screen recording! That's very helpful.

What's not clear, though, is if that issue is being caused by this patch. Can you reproduce that problem with a vanilla copy of Drupal core (without this patch applied)? Or does it only happen when this patch is applied?

Removing Europe2020 tag, since this issue was not worked on at Drupalcon Europe 2020.

Having reviewed the video at #38, I can verify that the issue shown is not caused by patch #36. I was able to reproduce the issue with a vanilla installation of Drupal.

Yesterday I helped tamsinfd with her first contribution, and we totally forgot to add that we did it during #ddd2022.

This is a reroll of the patch in #36.
The old patch was outdated, and contained some mistakes causing it to not apply correctly.
This patch works well, except for one edge case. When you upload a new file, instead of choosing an existing one, and then discard it.
I think that is because the check for disabling the button is not done when a newly uploaded file is discarded. You will see that the media library dialog is shown again, with an enabled button.

Added DrupalDevDays2022 tag

Fixed CS errors.

Verified and tested the issue by applying patch 3167799-48.patch. Patch was successfully applied and issue got fixed.
Please refer to the video attached.

I added the Portland2022 tag. I think the issue is almost RTBC, but the issue summary needs to be updated to reflect recent comments. Specifically it would be nice to have for a committer to see @priyanka.sahni 's manual testing animated image in the issue summary as well as to confirm testing on 9.4.x branch.

This definitely seems pretty good but I think it could use a little work, mostly to document the nature of the change.

1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -308,7 +307,7 @@
   +                Drupal.MediaLibrary.currentSelection.length,
   

   I'm a little unclear on why we're changing these variable references. Is this the main part of the fix? If so, it would be good to have a comment explaining why we're referencing the counter this way, so we don't accidentally regress it.

2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -317,12 +316,17 @@
   +        if (Drupal.MediaLibrary.currentSelection.length === 0) {
   +          $('.media-library-select').attr('disabled', true);
   +        } else {
   +          $('.media-library-select').attr('disabled', false);
   +        }
   

   🤔 I thought that disabled was a "present or absent" sort of attribute. Wouldn't we want this to be something like:

   if (length === 0) {
     $('.media-library-select').attr('disabled', true);
   }
   else {
     $('.media-library-select').removeAttr('disabled');
   }
   

   Also, we usually only want to target classes prefixed with js- in JavaScript. As I understand it, non-prefixed classes are intended for styling. Is there a js- prefixed class we could use here, or some other way to select it?

3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -331,15 +335,15 @@
   +        const position = Drupal.MediaLibrary.currentSelection.indexOf(id);
   

   As with the counter, why are we referring to this variable by its fully qualified scope?

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetUploadTest.php
   @@ -140,6 +140,7 @@ public function testWidgetUpload() {
   +    $assert_session->pageTextContains('0 of 2 items selected');
   

   This line could use a comment to explain why we're asserting this text. Also, if we can scope it a little more clearly (maybe using $assert_session->elementTextContains() or similar so that it's clear that we're looking at the selection counter of the media library, that would probably be helpful to future maintainers.

Rerolled patch for drupal core 9.5.x

Just ran into this issue and can confirm it still exists.

Adding patch for v10.1.x

Fixed failures in #58.

#51 still needs to be addressed. When someone is able to address it, this issue can be converted to a merge request (but credit will not be granted for converting this issue to an MR without addressing #51). Thanks!

The patch is not applying to D 11.2.10:
Why the patch wasn't applying:
Drupal 11.2.10 partially includes the fix: it removed const currentSelection = Drupal.MediaLibrary.currentSelection; and updated most references.
Lines 336-341 still referenced currentSelection (undefined), causing a bug.
The patch expected the old code structure (with the const declaration), so it couldn't apply.
What is fixed in attached patch :
Updated lines 334-342 to use Drupal.MediaLibrary.currentSelection instead of currentSelection.
Removed the patch from composer.json (line 299) since it's incompatible with D11.2.10.
The fix:
Changed currentSelection.includes(id) to use Drupal.MediaLibrary.currentSelection.indexOf(id) with position checking
Changed currentSelection.push(id) to Drupal.MediaLibrary.currentSelection.push(id)
Changed currentSelection.splice() to Drupal.MediaLibrary.currentSelection.splice()