Problem/Motivation
When adding new media through the Media Library, it's possible to add more than the allowed number of media items.
Steps to reproduce:
1) In the Media Library, if you can select two items, select two.
2) Now upload a third item.
Expected: You should not be able to select more than the allowed number, or should receive a warning if you do.
Actual: no warning appears, and if you then insert the media you get nothing is attached. It fails without warning.
Proposed resolution
- When the number of selected items exceeds the maximum allowed after adding one or more items in the media library, display a warning.
- If the user then persists on inserting the media items with an overage, do not allow it, but display an error message.
Video:
https://www.drupal.org/files/issues/2019-12-21/3082690-71-walkthrough_0.mov
Remaining tasks
Discuss solutionWrite patchUX ReviewA11Y ReviewCode review- Frontend framework manager review
- Commit
User interface changes
Added error / warning messages to the media library.
Video of #21: https://www.drupal.org/files/issues/2019-07-17/media-library-exceeds-lim...
The warning after adding and exceeding the limit:
https://www.drupal.org/files/issues/2019-07-17/media-library-add-warning...
The warning after clicking 'Save and select':
https://www.drupal.org/files/issues/2019-07-17/media-library-save-and-se...
The error after clicking 'Save and insert' or 'Insert selected':
https://www.drupal.org/files/issues/2019-07-17/media-library-save-and-in...
API changes
Data model changes
None
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#182 | upload-pdf-not-working-issues-3059955-182.patch | 7.21 KB | Swayanka |
#178 | interdiff.txt | 1.86 KB | lauriii |
#176 | interdiff.txt | 717 bytes | lauriii |
#176 | 3059955-176.patch | 20.43 KB | lauriii |
#165 | aftr_164.mov | 3 MB | sonam.chaturvedi |
Issue fork drupal-3059955
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3059955-it-is-possible changes, plain diff MR !760
Comments
Comment #2
NikolaAt CreditAttribution: NikolaAt commentedComment #3
NikolaAt CreditAttribution: NikolaAt commentedComment #4
NikolaAt CreditAttribution: NikolaAt commentedComment #5
Lincoln-Batsirayi CreditAttribution: Lincoln-Batsirayi at CTI Digital commentedI'm having the same issue as well, I've also got a media field inside a paragraph. The issue for me occurs when the apply filters button is clicked, it allow's you to select more than one image (which is wrong).
I've been able to work out that when the Apply filters button is clicked, the MediaLibraryWidgetRemaining behaviour inside the media_library module, is ran BUT with the context of the filters and not the whole view. I thought about writing some js to rerun this behaviour but it is using a .once inside it which means once its initially ran on page load, it doesn't load again.
Comment #6
blazey CreditAttribution: blazey at Amazee Labs commentedSame here. The error shows up but only after the page is reloaded. Here's a video recorded on a clean 8.7.3 installation.
Comment #7
phenaproximaComment #8
AndySipple CreditAttribution: AndySipple commentedThis appears to be an issue with the media library 'Drupal.behaviors.MediaLibraryItemSelection' in 'core/modules/media_library/js/media_library.ui.js'. Switching over to another media tab from the primary one doesn't update the active tabs items.
The issue seems to be coming from this chunk of code:
The selections logic when switching tabs doesn't follow over.
Copying the code inside that on change function above it fixes the issue.
Comment #9
mikelutzSetting to NW for test coverage.
Comment #10
NikolaAt CreditAttribution: NikolaAt commentedThe patch is working 50%. When I select media and change the filter, I am not able to select another. This is very well. Thanks.
The problem that, when you select media and choose to add file from you local system, the two files are selected and you can select more. This is not ok.
Comment #11
phenaproximaRe-titling to make it a bit clearer what's going on.
Comment #12
phenaproximaI think the problem here is that the file upload field does not validate the number of incoming files. You can set a managed_file element to either allow one file, or many, but there's no way to limit it to a specific number of files.
Although, such validation does already exist in core, in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::validateMultipleCount(). But that's tied to the file field widget, rather than the managed_file element itself. Since the media library doesn't use the file field widget, it is affected by this problem.
I think this is something we should fix in the managed_file element itself. We could add a property which forces it to validate the number of uploaded files, if the #multiple property is set, and change the FileWidget code to use this property.
Comment #13
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThere might be things about #12 that's worth opening a separate issue for, but I don't think it's the root cause of this issue.
I think the root of this issue is: if you already have the maximum number of items selected, what should the "Add file" portion of the media library do? Should it be disabled until you've unselected something? Should it let you upload more but after upload not have them selected? Or something else? I don't see how adding more validation to the managed_file element type is part of solving that.
Comment #14
phenaproximaThe more I think about it, the more I think I agree with @effulgentsia. I think this is primarily a usability issue and should be brought before the UX team so we can find a solution to this problem.
Comment #15
phenaproximaThis was discussed in the usability meeting on July 2nd, 2019: https://www.youtube.com/watch?v=x3ztx9_kT88
In what has got to be the single fastest UX meeting consensus in history, we reached an agreement on how to approach this:
This will probably have accessibility implications too, so tagging for that.
Comment #16
AaronMcHaleRe #15:
Thanks @phenaproxima for summariesing the discussion. Just regarding:
The way I was picturing it during the UX call was that once the items were uploaded you would also get the warning on the uploaded screen and the button to save and insert would just be disabled not totally gone - I think that would be better from a UX perspective because it avoids the issue of the user wondering why the button they were expecting to see isn't there.
Essentially the logic should look like: Has the user gotten themselves into a situation where they have selected more items than the field allows (currently selected + currently being uploaded)? If yes, display a warning message on all of the Media Library Dialogue screens indicating how many items over the limit they are (update the warning each time their total selection changes, warning message should be contextually appropriate for each screen, so would be worded differently on selection screen vs upload screen), and disable any buttons which would result in the media being inserted until they are under the limit.
Thanks
Comment #17
seanBAdded the tag for A11Y review since I'm a bit worried disabling the 'Insert selected' button could be an issue.
We could potentially show a message in the modal and not disable the button, and trim some of the items when inserting in the widget (and show another message we did that. Not saying this is perfect, but this is also what the "regular" image field does. Specially if we already show a warning when the users gets in this situation (after uploading too many items), this might be better than disabling the button.
When a user has selected multiple items over multiple tabs, and also when using pagers/filters, requiring a user to deselect stuff in the modal could be difficult until we have #3023809: Add a selection overview to the media library widget modal.
The "regular" image field currently already allows more uploads when the cardinality is > 1. Below the screenshots of what that looks like.
After uploading 5 items:
Comment #18
seanBHere is a first patch to implement #15. And a nice video showing it :)
Tests are still missing.
Comment #20
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedI'm not keen on the disabled buttons idea at all.
The disabled state will not be obvious, for a lot of sighted users.
Comment #21
seanBWe have discussed this in the UX meeting yesterday: https://youtu.be/TlYSuIxU8ZA?t=532
Glad andrewmacpherson could join us to discuss and explain the accessibility implications of the disabled buttons. Couple of things we agreed upon:
Attached are a new video (https://www.drupal.org/files/issues/2019-07-17/media-library-exceeds-lim...), some screenshots and a patch to implement what we have discussed (including tests). Removing the tags for tests and A11Y review, but adding a tag for frontend framework manager review because of all the JS changes I had to make.
Screenshot of the warning after adding and exceeding the limit:
Screenshot of the warning after clicking 'Save and select':
Screenshot of the error after clicking 'Save and insert' or 'Insert selected':
*EDIT
Crap, missed that, will fix on next patch!
Comment #22
phenaproximaThis is really nice. Great work, @seanB!
I gave this patch a quick read and I think my main concern is the way the messages are handled. Honestly, there should be a generic AJAX command which can manipulate a Drupal.Message instance on the page. I don't know if that's a thing we need to block on and do in a separate issue, but I think it should be implemented correctly here the first time so that we don't have to bother with backwards compatibility stuff in follow-ups.
So what I think I'd prefer here is something like this:
js-media-library-messages-target
.js-media-library-messages-target
element.So, the JavaScript side:
And on the PHP side:
Comment #23
lauriii✌️
Is there a specific reason why we're adding the data-drupal-messages here? It shouldn't be necessary because we are already passing the message wrapper to the constructor.
Comment #24
AaronMcHaleBeen on holiday last couple of weeks, so haven't been keeping up.
I am very happy with displaying an error message instead of disabling the buttons, it seems to avoid the accessibility issues while still preventing the user from getting into an overflow state, great work :)
One small concern is that in the demo video while on the upload screen: when the user clicks the "Save and insert" button, they are taken to the media library screen if the error is triggered. This I think could be improved by not navigating away from the upload screen and instead displaying the error on that screen, this I think would provide a more consistent UX.
Thanks
-Aaron
Comment #25
xjmWhen you have too many items attached, what happens when you try to save the form? Is there a validation error? (This affects whether this is "only" a major UX bug or actually a blocking data integrity problem.)
Comment #26
seanBThat would mean you are not able to save the added media items, and forces the user to delete instead of de-select. I think saving their work first, and them allowing them to correct the selection is a lot nicer.
See the attached video of what happens when trying to insert too many item. The widget doesn't accept your selection. In the background and error message is generated but not shown. And when you re-open the library you see the error. It's quite the WTF.
Comment #27
bnjmnmFrom #22.3
I happened to see this a few hours after adding that functionality (with tests) to a separate issue: #3081526: Add announcement after clicking 'Save and select' in the media library.
If that issue gets stalled for some reason, we could create a separate issue for just the AJAX command so it can be made available here and elsewhere.
Comment #28
Wim LeersAre we considering this issue blocked on #3081526: Add announcement after clicking 'Save and select' in the media library. then?
Comment #29
oknateReroll of #21, as it doesn't currently apply to HEAD.
Comment #30
oknateMarking postponed on #3086096: Add a generic Ajax Message command
@bnjmnm already implemented the requested message command here #3081526-22: Add announcement after clicking 'Save and select' in the media library., but I think this should be moved to a separate non-media-library specific issue.
Comment #32
oknateI have tested out #21 and it makes some nice improvements. It needs some work on the Ajax returned messages. I'm getting double messages right now.
I updated the code based on the feedback in #22. I attach the message container when the library is loaded up and attach the message container to both places so we can drop the conditional placement code.
This includes the relevant parts of @bnjmnm's patch for #3081526-11: Add announcement after clicking 'Save and select' in the media library. that adds the new Message ajax.
I'll keep working on it tomorrow morning, unless Sean B or Bnjmjm wants to take another look.
Comment #33
oknateComment #34
oknateI was able to fix the repeating messages by adding an option to the Ajax command to clear out the previous messages:
The repeating messages happen when the ajax response adds a message, it doesn't delete the previous ones.
I didn't go with the suggestion in #22 -
>clearAll()
, as I didn't see any other Ajax commands that do similar things.One thing I preferred in #21 was that it took the message type directly as an argument rather than as an option:
$response->addCommand(new AddMessageCommand($message, 'maxItemsExceeded', 'error'));
rather than
$response->addCommand(new MessageCommand($message, '.js-media-library-messages', ['type' => 'error']));
But perhaps it's better to allow an array of options for those who want to override it.
Another change that's worth mentioning. I was having trouble preventing double '.js-media-library-messages ' wrapper elements from being added, and problems with the AJAX not adding messages to it, I guess because it gets replaced?
So I'm testing out adding it on the back end, like so:
Comment #35
oknateOops, needed the messages on the views form too, not just the upload form.
Comment #36
oknateThe last change was breaking paging. I didn't investigate, but I suspect because I was trying to use ['#prefix'] in the viewsForm for the messages. Also, the class ''js-media-library-messages-target' isn't being used now, so removing that.
Comment #37
oknateMy previous patch was missing MessageCommand::clearPrevious code. See #3086096-11: Add a generic Ajax Message command, without that, duplicate messages will display.
Comment #38
oknateReroll on top of #3086096-20: Add a generic Ajax Message command
Comment #39
oknateMarking postponed on #3086096: Add a generic Ajax Message command
Comment #40
oknateSimplifying the "Problem/Motivation" portion of the IS and adding before/after videos.
Comment #41
oknateComment #42
oknateFixing coding standards.
Comment #43
oknate#3086096: Add a generic Ajax Message command is RTBC, so one that goes in, this will need a reroll. This could use a review from @phenaproxima that his feedback in #22 has been adequately addressed.
Comment #44
Wim Leers#3086096: Add a generic Ajax Message command landed, this is now unblocked! Assigning to @phenaproxima per #43.
Comment #45
phenaproximaDo we really need these? They seem like very thin, and potentially unnecessary, wrappers around Drupal.Message.
What might be better, if we need to have these, is make them helper methods in Drupal.MediaLibrary.
This is a nice fix. 👍
This code is repeated elsewhere, which is not great. Ideally we'd be able to make it a method of Drupal.MediaLibrary, callable like so:
What is this class used for?
Same here.
We should be using MediaLibraryState::hasSlotsAvailable(), not checking the value of getAvailableSlots(). So this should look more like:
What message ID?
What is the reason for this change? This may necessitate accessibility review, if needed, and might need manual testing.
Are we using this for anything? This change might also constitute a front-end BC break in a stable module, so I'm not sure we should change it unless we need to. If we must, then we should add the
js-media-library-insert-button
class without removing the other one.So, here's the thing.
This is the third time we are effectively repeating this logic in this patch. Once in the add form, once in the view selection, and once in JavaScript. That's a lot of repetition, and a clear sign that our approach is probably not optimal.
At the moment, I'm not 100% sure how to handle this. But I'm leaning towards doing this in the opener's getSelectionResponse() method. That method is responsible for building the actual AJAX response and is given the media library state and the selected item IDs -- including newly added ones, I believe -- so it would be trivial for us to add a base class that both existing openers would extend, and which would do this validation.
I can write a proof of concept for this if you want, but I think this is my single biggest complaint with the patch as it currently exists.
Comment #46
phenaproximaHere's a do-not-test PoC of what I'm talking about.
Comment #48
oknatePostponing on #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate, which refactors Media Library widget and tests significantly, and will probably be committed before this one.
Comment #49
oknateignore this one, I failed to pull latest changes in the branch I was diffing against.
Comment #50
oknateReroll of 46. I'm dropping the test coverage temporarily in this patch. I'll reroll this on top of #3087456: Move some representational classes in Media Library to Classy and others to Seven and/or Claro, as appropriate, then add the coverage back.
Comment #51
oknateAddressing #45
1. ✅ Since we’re using the new functions in only one place, I think for now, it’s better to drop them.
3. ¯\_(ツ)_/¯ I’m not sure what to do with this feedback. This seems redundant:
since you could just check Drupal.MediaLibrary.currentSelection.length, and this seems out of place within the class:
Can you clarify why this is better than handling it outside the class. It seems to me, in the context of the add form, the new items should be calculated at that moment.
4. ✅ That class isn’t used, removing.
5. ✅ That class isn’t used, removing.
6.I changed this to use a new method ::hasUnlimitedSlotsAvailable(). I think this is worth adding even if we're using it one place for now. It allows for more readable/comprehensible code, without having to explain again that "a negative integer" means unlimited. it was checking $available_slots because ::hasSlotsAvailable() doesn't quite work. We need this to run if there are zero or more slots available. To make this more explicit I want to add hasUnlimitedSlotsAvailable(), because that's the only condition in which this shouldn't be validated. Doing
$state->getAvailableSlots() < 0
is confusing. I think adding a method makes the logic clearer. This could be dropped if you think it's a bad idea and a comment could explain the logic.7. ✅ The message ID was a vestige of an earlier iteration of the patch, added in 21. The new MessageCommand doesn’t use this parameter. So removing references to it.
8. ✅ Removing tab index change. That was added by me in 29, which was a reroll of 21, which didn’t have it. So I think it was patch noise.
9. ✅ ‘js-media-library-insert-button’ isn’t being used. I asked Sean B about this a while back and he said that it was to make it clearer which button was being pushed, butt that it was probably from iterating on the patch, and therefore can be removed.
10. @todo still need to evaluate your changes. I’m renaming the validate function to ::validateSelection(). It’s more general, in we want to add validation later on that isn’t related to available slots.
@todos:
Comment #52
oknateI'm working on rerolling this on top of #3082690-81: Mark Media Library as a stable core module and #3087305-26: Form validation error messages within the Media Library widget are not read by the screenreader. It's broken right now, but I wanted to post my work in progress.
Comment #53
phenaproximaTagging this extremely important bug fix to be worked on at DrupalCon Amsterdam.
Comment #54
bnjmnmAssigning to myself for a bit as I'm working on the reroll (not the easiest I've encountered...) and want to be sure nobody else has to duplicate this effort.
Comment #55
bnjmnmHere's a beast of a reroll 🐻
The patch includes #3082690-81: Mark Media Library as a stable core module and #3087305-26: Form validation error messages within the Media Library widget are not read by the screenreader so I also provided an diff against a branch with those two applied to provide an easy way to identify the changes specific to this issue.
Unassigning myself but can't re-assign to @phenaproxima who was the previous assignee
Comment #56
phenaproximaRe-assigning to myself to review.
Comment #57
phenaproximaMedia Library is now stable, so removing the PP-1 designation.
Comment #58
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
I tried patch #55 and it fails to apply with core 8.9.x. So, I am changing the status to "Needs Work".
Below is an error screenshot.
Thanks,
Ren
Comment #59
phenaproximaTagging for reroll.
Comment #60
oknateReroll, sorry about the issue number being wrong in in the patch filename. I don't know where I got that from!
Comment #61
oknateComment #62
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
I have reviewed the patch #60. Below is my update.
There are allowed no of an item to upload is one.
Then I have selected one image from uploaded media and clicked on "Choose File" to select another image.
I have not gotten a warning message as I saw in the video.
Then I clicked on the "Save and select" button.
Then I had got the warning message.
But when I clicked on the "insert selected" button then I didn't get an error message as I saw in the video.
Below is an output screenshot after clicking on the "insert selected" button.
So, it's not working as I saw in the video.
Thanks,
Ren
Comment #63
oknateMarking back to needs work based on Ren's review. The error message when the user has inserted more than the allowed number needs to be restored. It was working in an earlier version of the patch.
Comment #64
fkelly12054@gmail.com CreditAttribution: fkelly12054@gmail.com as a volunteer commentedTesting locally on 8.8 RC1. In Home/Administration/Structure/Media types/ Edit Image/Manage fields/ Image I set the allowed number of values to unlimited. Tried to upload seven images. Only the first one shows up in the media library. This first file shows up in the table media_field_data together with two other Image files I had uploaded (one file at a time) earlier. The other files do show up in sites/default/files/2019_12 and in the filename field of the file_managed table. So, they are getting uploaded and stored but not incorporated into the media library.
Interesting. Went into reports/recent log messages. See the line:
media 12/02/2019 - 15:08 image: added _MG_6080.jpg. frank View
The _MG_6080.jpg is the first file in the group of files I tried to upload. There are no messages for the other files. But If I click on View in that line it shows the first file and then the other six files I uploaded in the same batch.
Comment #65
fkelly12054@gmail.com CreditAttribution: fkelly12054@gmail.com as a volunteer commentedBehavior reported in #64 running RC1 is basically unchanged in 8.8.0 official release. Multiple files do get uploaded into sites/default/files ... but only one at a time gets put in the media library.
If this is confirmed maybe it should be mentioned more prominently in some of the release notes? It's going to take a cleanup effort when the behavior is fixed for users to go back through and delete files in the files directory that were added but not included in the media library so they can get them re-added and included.
Comment #66
phenaproximaI don't mean to sound like a jerk, so I apologize if I come off that way here, but this sounds like a totally separate problem from what this issue is about.
In fact, I think the behavior you describe might be intentional. Media Library was not meant to put multiple images into a single media item -- it creates a new media item for each image you upload, because that keeps the individual images reusable and is very much the "80% use case". As far as I know, attaching multiple images to a single media item is uncommon, and having Media Library support that out of the box could heavily compromise the reusability that is the main purpose of the media library. It would likely some very significant usability and accessibility questions as well, not to mention contradict some of the media library's fundamental design decisions.
That said, what you are asking for is very likely doable in a contrib module -- I can totally imagine something like Media Entity Slideshow needing to support a use case like this -- but I'm not sure it would be in scope for core. (At the very least, if you want to propose it as a feature for core, you are certainly welcome to open a separate issue to talk about that.)
Comment #67
oknateThe @todo here is to fix it so it works again like the video in the issue summary:
https://www.drupal.org/files/issues/2019-10-08/overflow-fix-demo.mov
Also, we need to restore test coverage, or add it.
Comment #68
oknateHere's a reroll of 60, minus the test coverage which doesn't apply. The @todos in #67 are still pending.
Comment #69
oknateI tested this a little tonight.
1. In the advanced UI, using select and insert it no longer prevents you from submitting when you have selected more than the allowed amount after uploading. The original bug is present where it submits, but then fails silently and the field displays 0 of 2 remaining. It should switch from a js error (yellow) to a form error (red). This was working in an earlier patch.
2. Removed this comment. I remember now what hasUnlimitedSlotsAvailable is for.
3. I think the behavior is pretty broken with the standard workflow (where there's just a save button). We were working on this before that was the default. Some of the behavior such as error messages don't appear in this workflow after upload: "There are currently 3 items selected. The maximum remaining for the field is 2. Please remove 1 item from the selection".
4. With the standard UI, if you have a cardinality of 2, and you select 2, it still displays 'Maximum 2 files.' under the add form field, and if you add more than 2 it fails and displays an error.
https://www.drupal.org/files/issues/2019-12-21/2-file-maximum.png
I think the idea here is now that you can upload as many as you want, so that should be fixed. The limit should basically never be there, once validation prevents you from inserting more than the limit in the media library view. It should always be unlimited in the add form, but should display a warning when you upload more than the allowed amount in the upload form. So it should let you upload three if you have already selected 2, and after they are uploaded it should display "There are currently 5 items selected. The maximum remaining for the field is 2. Please remove 3 items from the selection". If you then hit the insert button from the media library view, it should change to an error.
Comment #70
oknateComment #71
oknateGiven how broken the patch currently is, I wanted to build up a new patch, with apologies to what has come before. I kept the error message, but I'm outputting it differently. It seems to work (in Chrome, see below) and it's much simpler. (Update: it works in Safari, I just needed to upgrade, see below).
I started by changing the file upload to allow an unlimited number of uploads (#69.4). Then I was able to find a different place to output the errors, in
AddFormBase::UpdateLibrary
. For the 'Select and Insert', which callsAddFormBase::updateWidget
, we'd need to repeat this logic, and I came up with the (smart, if I do say so myself) idea to just callAddFormBase::UpdateLibrary
when the number is exceeded, which means we don't need to reuse the code that outputs the warning for both buttons (Yeah!). This works for both the Advanced UI and the basic UI.For the error when actually trying to submit the media library with an overage, I had to copy the logic into MediaLibrarySelectForm::updateWidget(). There may be a way to share the error message code between the two classes. I'm not sure it's worth it, as it's just one message, and it's an error in this case, but a warning in the other. But the message text is the same. An elaborate solution to prevent sharing the code may be more convoluted, and therefore not worth it, but I'm open to ideas there.
One fact that amazes me here, is that I don't use javascript at all, and I think I've covered all the use cases. Perhaps I'm missing something?
If not, then I can just work on adding the test coverage back.
I tried to test with VoiceOver on Safari, and the whole thing is busted due to a javascript error (see screenshot).
https://www.drupal.org/files/issues/2019-12-21/safari-errors.png
I was able to confirm this happens without the patch, so I created a separate issue for it: #3102572: Media Library upload broken in Safari versions less than 10.3
Here's a walkthrough of this patch:
https://www.drupal.org/files/issues/2019-12-21/3082690-71-walkthrough_0.mov
Comment #72
oknateUpdated the wording of the proposed solution slightly.
Comment #74
oknateFixed a bug in the approach from #71 with unlimited cardinality. Let's see what tests fail now.
Also, I tested with VoiceOver and Safari (with the patch for #3102572: Media Library upload broken in Safari versions less than 10.3), and I found that because the message is in the same place and has the same text, it wasn't read out when it switched from a warning to an error. I found if we change the text slightly, it will read out. So I just added the preface 'Error: '. I don't love this solution. I think perhaps we could update Drupal.announce to handle when a message is the same but switches from a warning to an error. But perhaps that could be a follow-up. It's better to have the message read out for now.
Comment #76
oknate1) I'm reverting the addition of the preface 'Error: ' to the error message. I was testing the screen reader functionality on a very old version of Safari (Safari 9). I upgraded my OS and Safari, so now I'm testing on Safari 13, and this just works now, i.e. it reads out the error message despite the text being identical to the warning. It also reads the Aria label first, so it prefaces the message with "Error message". So I don't need the work around added in 74.
2) I updated the tests to account for the fact that the approach in #71 allows unlimited uploads. We could move this change to a separate issue, if it proves contentious, because it not essential to this issue. But I think they go hand in hand. Adding validation of the number to be inserted means it's not necessary to return a form error when uploading more than the displayed maximum number of uploads.
3) This change addressed a random failure:
3) I want to make one unnecessary change:
This local variable is named differently in
MediaLibrarySelectForm::updateWidget
vsAddFormBase::updateLibrary
. I'd like them to use the same variable name, $media_ids. I don't feel too strongly about this, and to reduce the amount of changes, we could allow leave it alone.Comment #77
oknateAdding test coverage for the approach in #71-76. I use a data provider to demonstrate that the behavior is the same with the advanced UI, i.e. all three button options in the upload form.
One thing we haven't tested yet is that it works as expected with a field with cardinality of -1.
Comment #78
oknateAdding test coverage for unlimited cardinality. Basically demonstrating the new validation fields with unlimited cardinality.
I think this is in a good place for now. I'm assigning to myself in case any changes are needed.
Comment #79
oknateReplacing video in the issue summary to use the new placement.
I'm leaving the screenshots alone for now. Before #71, the messages appeared in two places. Now I'm putting them both above the thumbnails in the view for both the warning and the error. I'll wait for feedback on the slight change of approach before updating that part of the issue summary.
Comment #80
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
I have reviewed the patch #78. Below is my update.
There are allowed no of an item to upload is one.
Then I have selected one image from uploaded media and clicked on "Choose File" to select another image.
Then I clicked on the "Save and select" button.
Then I had got the warning message.
when I clicked on the "insert selected" button then I did get an error message
Below is an output screenshot after clicking on the "insert selected" button.
So, it's working great.
Also, I have checked the patch for Drupal coding standard. There was an issue in one file. I have solved that and uploaded the patch.
Thanks,
Ren
Comment #81
dorficus CreditAttribution: dorficus at Genuine commentedJust a tiny nitpick here, but should the language of the warning/error message be clarified a bit? When in the media library modal the text
There are currently @total items selected, but the maximum number of remaining items for the field is @max. Please remove @count item from the selection.', 'There are currently @total items selected. The maximum number of remaining items for the field is @max. Please remove @count items from the selection.<code> is a bit strange.
Maybe remove the word "remaining" so it reads more like
There are currently @total items selected, but the maximum number of items for the field is @max. Please remove @count item from the selection.', 'There are currently @total items selected. The maximum number of items for the field is @max. Please remove @count items from the selection.
Comment #82
phenaproxima+1, I think that is clearer.
Comment #84
bkosborneOne of my users reported this bug. I tested the patch above and it works to resolve the issue for us. I didn't review the code so leaving at needs review.
Comment #85
phenaproximaComment #91
phenaproximaTransferring credit from #3092536: Possible to select multiple images in a media field where only 1 value is accepted, which is the same bug.
Comment #92
janmejaig CreditAttribution: janmejaig at Srijan | A Material+ Company for Drupal India Association commentedAfter applying this patch #80 this is working fine for drupal 9.1.x , this is good to go for RTBC .
Comment #93
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedPatch #80 works fine for me as well on 9.1
This can be moved to RTBC
Comment #95
tyler-paavola CreditAttribution: tyler-paavola commentedI've applied Patch #80 after upgrading from 8.7.x to 8.9.x to resolve the overflow select. Thank you!
I came across an edge case which I wanted to share that allows selecting multiple images, aside from the "select image + upload image" scenario discussed in the thread.
After selecting 1 image by its checkbox, remaining images (and their checkbox) are greyed out as expected. However, I am able to select additional images by clicking their teaser image (rather than the checkbox). After selecting an image by its teaser image, all other images (and their checkbox) are no longer greyed out. Regardless, the error message from the patch is still displayed, but the different user experience with the teaser image vs checkbox is a unorthodox.
Comment #96
chr.fritschThis looks pretty good to me and we already got a bunch of +1's. I tried to reproduce #95, but I wan't able to get that error.
I updated the patch by only fixing #81.
Comment #98
paulocsIt looks random fail.
Locally tests are working.
Set back to RTBC.
Comment #99
phenaproximaHmm...I'm not so sure that's a random failure. It's specifically failing the test which tests this patch.
Upon occasion, a test will pass locally but fail on Drupal CI. That may be the case here. I'll try re-running the tests on Drupal CI, and if it is a random failure, we will need to trace and fix it before we can commit this. Random failures are not our friend, and we need to avoid introducing any new ones in core. :(
Comment #101
raman.b CreditAttribution: raman.b at OpenSense Labs commentedAddressing the remaining deprecation notice
Comment #102
chr.fritschNice, so it was because of the deprecation warnings. We didn't had the problem with #80 back in last December, because it was only running against D8.9.x
Comment #103
paulocsI didn't know deprecation notices make the tests fail in the Drupal CI.
Good to know!
Comment #104
alexpottComment #105
alexpottAll of this exploding and imploding looks funny. Can this not be:
Less string manipulation and less work.
Use \Drupal\Core\Field\FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED
Comment #106
paulocsI'll work on it.
Comment #107
paulocsI did the changes suggested on comment #105.
Comment #108
saurabh.tripathi.cs CreditAttribution: saurabh.tripathi.cs as a volunteer and at Acquia for Acquia commentedThere is one issue found with error message.
The error message is appearing saying, no item selected.
This only happens, when we let validation error appear, and then insert the image, and try opening the media window again.
Steps to reproduce:
Create a node of article type.
In WYSIWYG editor>Click on Media icon.
Select one file image type and then document type.
Click on Insert Selected button>Error message will come.
Unselect other files and select one>Click on the Insert Selected button.
Click on Media icon again
You will still see an error message, saying no item selected.
Possible root cause: Since we are showing that error message, and page is not refreshed, the error message box retains on media popup.
Comment #109
saurabh.tripathi.cs CreditAttribution: saurabh.tripathi.cs as a volunteer and at Acquia for Acquia commentedComment #110
nightlife2008 CreditAttribution: nightlife2008 commentedJust found another buggy situation:
- Create a content type with a single value media reference field
- Create a node
- Click "Add" in the media library reference field
- In the modal, first select any existing item
- Now select a file in your local drive for upload
=> This will trigger an autosubmit of the upload field
=> "Save & Insert" will now append _both_ items to the field
=> The UI Is broken, and the user can't remove the second item anymore
=> When submitting the node-form, the user gets an error about the number of references in the field, but they can't alter anything anymore
=> Final result: you should delete the node, and start over.
Comment #112
nikunj.shah CreditAttribution: nikunj.shah at QED42 for Drupal Association commentedI have created Reroll of #107 for 9.3.x-dev. Please review.
Comment #113
dmezquiaThis works for me the Reroll #112 !
Comment #114
kim.pepperComment #116
phenaproximaGave this a shallow review. There is almost certainly going to be more feedback, but for the moment, I think this is enough to start with.
Comment #117
phenaproximaHiding old/defunct patches, now that we have a merge request open.
Comment #118
phjouJust tried out the merge request, it fixes the issue for me :)
Comment #119
paulocsI'm on it...
Comment #120
paulocsComment #121
manojithape CreditAttribution: manojithape at QED42 for Drupal India Association commentedComment #122
manojithape CreditAttribution: manojithape at QED42 for Drupal India Association commentedVerified and tested patch "MR !760 mergeable" (i.e. https://git.drupalcode.org/project/drupal/-/merge_requests/760.patch) on the drupal 9.3.x-dev version. Patch applied successfully and looks good to me.
Testing Steps:
Expected Result:
The user should not able to select/upload more than the allowed number of files and if the user tries to do this user should get an error message.
Actual Result:
After applying the patch, the user is not able to select/upload more than the allowed number of files, and if the user tries to do this user gets an error message.
After applying patch functionality working as expected and also verified comment#110 that issue also resolved.
We can move this ticket to RTBC.
Comment #123
paulocsHi @manojithape!
Did you test if #108 and #110 were addressed?
Comment #124
chr.fritschI can confirm that #108 is still an issue.
I also tried to reproduce #110, but never came to the point that I was unable to unselect media items. But yes, if you select 1 item -> upload a second image, you have 2 items selected and you are still able to select more items. If you reduce your selected items to one, from there all the other items are disabled for selection.
Comment #125
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedThank you, Adam for the patch fix
Having the same issue in Drupal ~9.0 too. Which was reported by the QA team with a number of scenarios.
Important to Freeze AJAX actions when the limit of media items is out
In case of using Media Library in a custom Block Type. Which used by the Layout Builder
For sure it should be fixed in Drupal core for normal edit/add forms and the 1st, 2nd, and 3rd AJAX level modal in back-end and front-end.
Comment #127
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedTested the #107 patch with Drupal 9.2.x
Working very well. Very much better than before.
Thanks, Paulo for patching this issue for Drupal 9.2.x
Having a message and a blocker of insert action is good
Hoping to have a quick release for this issue in Drupal 9.3.x and 9.2.x or it will be only committed to 9.3.x
Comment #128
yogeshmpawarComment #129
NikolaAt CreditAttribution: NikolaAt commentedHello, recently upgrade to 9.2.6 and find out that changes committed to the core are not full. There is one more existing case from the first variations of the patch, where it handles if you switch pages or apply some filters for media library.
Comment #130
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedSwitched to test with the 760.patch to fix drupal-9.2.x
After
It's working well
After the fix:
1 was selected then on the upload of new image media file
Very good as not able to insert selected if they are out of the limit
After unchanging the extra old selected media item
Then we are able to insert
Comment #131
NikolaAt CreditAttribution: NikolaAt commentedIgnore #129 -> Proper patch is this.
This is fixing my issue with switching the pages in media browser.
Comment #133
scotwith1t#131 did not work for us. Using the pager to go to other pages of results, we can then add more than 1 item to the field which only allows a single image on D9.2.6.
Comment #134
danflanagan8Setting back to NW because there's a cspell error in #131 and the latest comment suggests that patch does not quite solve the bug.
Comment #135
mErilainen CreditAttribution: mErilainen at Wunder commentedThe same issue as with the pager applies when filtering with a keyword. "2 of 1 item selected".
Comment #136
NievesWunder CreditAttribution: NievesWunder commentedI fixed the issue of the images being still enabled even though the limit was achieved when filtering and using the pager
Comment #137
dariemlazaroI need help. The patch #136 not work in Drupal 9.3.7.
Comment #139
webcultist CreditAttribution: webcultist commentedTried 136. I could check just one picture, but when I already checked one image and filled another into the upload field above and clicked embed, both images get posted and you can't delete one in interface. Nothing happens when I clicked the "X".
Comment #140
bbu23 CreditAttribution: bbu23 at Ninja Coders commentedTried patch #136 against Drupal core 9.3.13, but unfortunately this doesn't work for my case.
This is reproducible by adding medias with pagination. For example:
1. Add media (let's assume image, but it doesn't matter)
2. From the media library listing, select one item from the first page. Now all the other entities should be disabled for selection (for limit 1)
3. Navigate to the next page and see that the items are available for selection
4. Select another image from the second page and insert them
5. Now you have 2 images, with "-1 media items remaining."
Comment #141
Chris Matthews CreditAttribution: Chris Matthews commentedSince 10.0.0-beta1 and 9.5.0-beta1 are planned to be released the week of September 12, should this issue now be targeted against 10.1.x-dev?
Comment #142
phenaproximaComment #143
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #136 on Drupal 10.1.x.
Comment #144
aludescher CreditAttribution: aludescher at drunomics for Porsche Informatik Gesellschaft m.b.H. commentedRe-rolled patch #143 for 9.4.x.
Comment #145
bnjmnm#129 / #131 are of an approach that was proposed very early in the process of this issue and the decision was to go in another directions. Now it appears like the comments since are reviews and rerolls of an outdated approach. Some really good changes happened since then and we should be working with those!
The work had moved to an MR. I'm providing a 10.1.x version of the work in that MR because
There's one thread in the MR that wasn't addressed
core/modules/media_library/src/Form/FileUploadForm.php line 157
@phenaproxima requested "Let's add a @see comment referencing the method where the validation is taking place."
Comment #146
smustgrave CreditAttribution: smustgrave at Mobomo commentedThank you @bnjmnm for the correct patch and catching that!
Seems the thread comment in #145 has been addressed.
Without the patch verified that the issue is present in 10.1.x following the testing steps in the issue summary.
After applying the patch.
I do get the message "There are currently 3 items selected. The maximum number of items for the field is 2. Please remove 1 item from the selection." as a warning
If I click save the warning becomes an error.
Tested uploading a 4th item also and the message correctly reflects that too.
Comment #147
xjmThanks for working on this! I haven't reviewed it completely yet, but I found several things that need to be addressed:
I'm surprised "medias" didn't fail cspell. It's not a word, because "media" is already the plural of "medium".
I'd suggest the method name should instead be:
getSelectedMediaItemCount()
.Also, does it really make sense for this method to be private (vs. protected)?
Finally, it should have a return typehint.
Nit: "IDs" should be capitalized. An id is something else.
This should begin with "De-selects", not "Unselects". "Media Library" should probably also be capitalized.
Nit: "Media Library" should be title-case, since it's a module name. (Here and elsewhere.)
Here and elsewhere, it should say "constrains the number of selected items". And "Media Library".
This is a really strange parameter name. Can we call it
$selected_operation
or something?Maybe:
Comment #148
xjmHiding old patches and closing the MR for clarity.
Comment #153
xjmIt is difficult to assign credit on this issue because:
Due to these problems, I unchecked everyone and started over.
The following people have been credited for substantive helpful code review and work on the code changes:
For helpful usability and accessibility feedback:
For the listed contributions
For work on #3092536: Possible to select multiple images in a media field where only 1 value is accepted
For work on #3160240: User can select multiple media entities but only one is allowed
Users that received credit for some contributions, but who should take note of best practices regarding others in the future
This contributions might receive credit but I need a second opinion from another maintainer
Contributions which will not receive credit
For folks on this list, in the future, please be more careful to read and understand the current state of an issue before working on it, take note of when a merge request has been opened, and don't resurrect patches for old versions.
If you have questions or concerns with these credit assignments, please carefully review the core issue credit guidelines.
Thanks everyone for your efforts!
Comment #154
harshitthakoreUpdated patch as suggested by @xjm in comment #147.
Comment #155
xjmThanks @harshitthakore.
Please provide interdiffs when updating patches using the patch workflow.
Your reroll is also missing the entire test file. When you apply patches locally, use
git apply --index
to stage files, and also pay close attention to making sure your patch includes all the changes from the original. A good way to check this is to consider the diffstat and patch size. @bnjmnm's patch in #145 is 17.94 K, and has 6 files changes, 277 insertions, and 9 deletions. Yours is 11.58 K with 5 files changed, 94 insertions, and 9 deletions. That is missing a lot of the added code.I will leave credit for your contribution because you did successfully update the method name, so there is still less work for subsequent reviewers, but please be more careful in the future to continue receiving credit for your contributions.
It looks like our instructions for applying a patch to a feature branch and creating a patch for an issue did not correctly include use of the
--index
flag, so I've updated them for future reference.If you need assistance with the patch creation workflow, you can also discuss patch creation with other contributors in Drupal Slack.
Comment #156
xjmI've attached #154 with the test from #145. Since it's the same version of that test from the earlier patch, points 4-7 in my review will still need to be addressed. Edit: Point 1 is also not fully addressed; the method has been renamed but there's no change to its typehint or visibility.
Comment #157
xjmThis also still says "medias".
Comment #158
xjmFixing attribution.
Comment #159
smustgrave CreditAttribution: smustgrave at Mobomo commentedFinished addressing the points in #147 + #157
Comment #160
mgiffordWCAG tagging.
Comment #161
joaopauloscho CreditAttribution: joaopauloscho at Zoocha commentedThe patch #159 works for us on 9.5.2 - thanks!
Comment #162
sonam.chaturvedi CreditAttribution: sonam.chaturvedi at Salsa Digital commentedPatch #159 works fine on 10.1.x-dev only when one Media type is enabled.
After applying patch:
1. When only "Image" media type is enabled in the Media Library then warning is displayed on exceeding allowed limit and error is displayed on clicking "Insert Selected".
2. When multiple media type is enabled say "Image" , "Document" then no warning is displayed on exceeding media selection. However, error is displayed on clicking "Insert Selected".
Expected Result: Warning should be displayed when combination of media type is selected and count exceeds allowed limit.
Comment #163
sonam.chaturvedi CreditAttribution: sonam.chaturvedi at Salsa Digital commentedComment #164
smustgrave CreditAttribution: smustgrave at Mobomo commentedWonder if this is any better?
Comment #165
sonam.chaturvedi CreditAttribution: sonam.chaturvedi at Salsa Digital commentedPatch #164 works fine on 10.1.x-dev and issue is resolved.
Test Result:
When multiple media type is enabled say "Image" , "Document" then user is not allowed to do multiple media selection. As user is able to select only one media at a time, there is no warning displayed. IMO this should be fine.
Please refer attached after patch video.
Comment #166
mei2020 CreditAttribution: mei2020 commentedcore/modules/media_library/js/media_library.ui.js on 9.5.x is different from 10.1-x-dev, rerolled this patch for 9.5.x, hope it helps.
Comment #167
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedTried to fix CCF for #166.
Comment #168
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #169
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedRe rolled the patch #167, the patch doesn't apply anymore
Comment #170
smustgrave CreditAttribution: smustgrave at Mobomo commented#164 still does.
Comment #172
mgifford@smustgrave in #166 it was reported that core/modules/media_library/js/media_library.ui.js on 9.5.x is different from 10.1-x-dev
I don't know, but just not sure if you're suggesting that #164 is the patch we should be using or what...
Comment #173
smustgrave CreditAttribution: smustgrave at Mobomo commented#164 should be used for testing on 11.x or 10.x
Can determine if we will backport to D9 after I suppose.
Comment #174
irene_dobbs CreditAttribution: irene_dobbs at Bounteous commentedTested patch 3059955-164.patch on DrupalPod, I used Drupal 10.1.x
This patch is working as proposed solution suggests, if selected number of items exceeds maximum number of allowed items, a warning message is displayed. If user tries to click 'Insert Selected' an error message is displayed and the user is not allowed to insert any items.
Comment #175
irene_dobbs CreditAttribution: irene_dobbs at Bounteous commentedComment #176
lauriiiRan
yarn prettier
to try to make the custom commands pass.Comment #178
lauriiiMade a minor fix on commit, attached interdiff.
Committed ae4b724 and pushed to 11.x. Thanks!
Leaving open for potential backport to 10.1.x.
Comment #180
lauriiiGot a +1 from @catch for the backport to 10.1.x during RC.
Comment #181
seanBWow, thank you all for getting this done! ❤️👏
Comment #182
Swayanka CreditAttribution: Swayanka commentedHi , I am adding a patch for the issue where i was not able to add any documents from "Add Media" .This has worked for me.
Comment #183
lauriii@Swayanka I think we should file a new issue for the bug you are reporting 😊