Problem/Motivation
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Problem/Motivation
This issue is a subtask to implement the new design of the media library #3019150: Update/improve mockups and designs for the media library.
Media types are related to media source plugins, and each media source contains a source field of a specific type. The source field input is used to uniquely identify the media item. The types of source input could be different for media sources. In core we currently have file based sources to allow adding new media via file uploads, but also the oembed source where users can add media via a URL.
There are a couple of issues we need to solve when users are trying to add new media:
- Since sources are plugins, contrib modules could get creative and add all kinds of source input field types (for example using an address field). How do we show all the source input fields?
- How do we help the user pick the right source field to add new media?
- Once the right input is given, it could resolve to multiple media types. How does the user pick the right media type?
Proposed resolution
While thinking about this problem, we assumed users know in advance what they are trying to add. They want to add a specific Youtube video, or upload some images from an event. If users pick a media type first, we automatically know which source field matches that media type.
- The first step to help users add new media in an easy way is creating separate vertical tabs for each media type so they can easily pick the type of media they want to interact with.
- If there is only 1 media type configured for a field, we hide the verticle tabs.
- We allow the media types to be sorted per field (widget). For each field the user can specify in which order the tabs should be shown. The first tab opens automatically so users can directly add media of that specific type.
- While selecting media in different tabs, the selection on other tabs should be remembered so the users can insert media from different tabs directly.
Current progress
There is a patch for the following changes:
- Menu links as vertical tabs in the media library modal. The menu links are not shown when there is only 1 type configured for the field.
- Type argument for the media library view so each vertical tab only shows media of the selected type.
- Only show the 'Select media' button when there is actually something selected.
- Show the number of selected items in the button pane.
- Make sure the selection persists between tabs.
- Make sure the selection is cleared when items are selected or the modal is closed.
- Make sure media items are disabled for each tab when the maximum number of items for the field is selected.
- Allow the media types to be sorted per widget, so the most important media type for the field is automatically selected when opening the media library.
Screenshots of the changes:
Before: Modal without anything selected
After: Modal without anything selected
Before: Modal with items selected
After: Modal with items selected
Before: Modal with max. item selected
After: Modal with max. item selected
After: Selection persists in other tabs (new feature)
After: Widget settings to sort vertical tabs style menu in modal (new feature)
Comment | File | Size | Author |
---|---|---|---|
#70 | add-rtl.png | 540.56 KB | seanB |
#70 | add-ltr.png | 550.79 KB | seanB |
#70 | library-rtl.png | 846.09 KB | seanB |
#70 | library-ltr.png | 844.88 KB | seanB |
#69 | 3020716-68.patch | 147.68 KB | seanB |
Comments
Comment #2
seanBHere is a first patch to add the following (incl tests):
Comment #3
phenaproximaThis is a great start! Will post a detailed review later, but for now I think this could benefit from "before" and "after" screenshots in the IS, to illustrate the difference.
Comment #4
phenaproximaWill we need an update function to make these modifications to the view? I think we do, if we're beta experimental. (Need to ask a release manager.)
I don't think that this is a mapping; it's an ordered sequence of media type IDs, like [remote_video, audio_file, image]. The implicit numeric keys are the weights.
Can we reuse or build on any vertical tabs styling? It seems like we might be repeating a lot of things here?
This seems like something we might want to make a little more formal than just a random object somewhere. Perhaps we should define an entire JavaScript class to encapsulate the selection, which could have additional useful methods on it. For now, maybe we can just start by defining it as a class (Drupal.MediaLibrary).
What I'm envisioning here would allow us to keep things relatively abstracted. For example, rather than have code which is constantly setting and unsetting CSS classes and doing other finicky DOM operations, we could encapsulate that like:
Drupal.MediaLibrary.selectItem(3);
This looks a bit complicated. It could use a comment.
It'd be neat if this were a generic thing we could do with any view. Maybe we should write this with an eye towards generic-izing it.
IMHO, this kind of thing is another reason to encapsulate the client-side media library logic in a JavaScript class.
This is hard to read. Why is an object being passed to .on()?
Why is this in a service? Seems kind of unnecessary...
Not sure about the name of this class. It's not really configuration; it's state. How about we call this Drupal\media_library\State instead?
I don't think this should be here. The widget ID is only relevant for field widgets, but we plan to integrate the media library with CKEditor as well. This is why I extended ParameterBag -- so that we could have accessors for all the bits of state that are universal for every context in which we might use the media library, but also allow for arbitrary, context-specific parameters like widget_id.
Maybe this should be $media_types ?: NULL, because if media_library_allowed_types is an empty array, no types will be loaded. Not sure that's desirable, but thought I'd call it out here.
I'm not entirely sure this belongs here. Dialog options are not part of the media library state. However, if the state of the media library should influence the dialog options (not sure why it would), then this should not be static.
This isn't a controller :) Also, it should be marked @internal and maybe even final. We don't want people extending it, at least not now.
This is really clean and elegant. I like it a lot!
Why would it be less than 1? Let's change that to ===.
No need for this. The state object extends ParameterBag, so this can just be $query = $configuration->all();
Maybe we should throw an AccessDeniedHttpException or NotFoundHttpException if the view doesn't exist. Better yet, we would ideally just have the controller deny access with a log message.
This, and the view ID, could and should be semi-configurable. How about a default route parameter?
target_bundles can be NULL, signifying that all bundles are allowed. We need to handle this case, and test for it.
This is confusing. Can there be more detailed comments explaining the intention here?
I don't know about "media type order"; it doesn't explain the connection that this has to how the UI will be rendered. How about "Enabled tabs", or "Tab order" or something? Also, we shouldn't display this if only one media type is enabled for the field.
We should keep this stuff encapsulated to the state object.
Comment #5
Wim LeersExciting! This definitely needs screenshots though. That was the first thing I was looking for :)
Comment #6
seanB*edit* Sorry, uploaded a wrong patch. Tried again in #8.
Comment #7
seanBComment #8
seanBAdded screenshots to the IS and also looked at #4:
Still todo:
Comment #11
seanBAdded the following:
Comment #13
phenaproximaVery cursory partial review...
I think the sequence is actually a string sequence, since each value in the sequence is a media type ID.
Let's add a follow-up (and put a TODO here referencing it) to look into making vertical tab styling semi-generic enough for us to reuse it here, rather than repeat all the CSS.
This is an improvement from a code organization perspective, but I'm also starting to think that the logic surrounding the media library is complex enough that we should probably create a Backbone view which encapsulates it and allows us to partially de-shackle the selection logic from the DOM. No need to do anything about this just yet, but it's worth considering further.
Nit: These methods should all be preceded by a blank line.
I'm not sure how I feel about having these methods. The presence of these methods assumes that the media library will always be called up in the context of a field, which is not necessarily the case. I think that we should remove these methods; if calling code needs a field ID or field type, they should call
$state->get('media_library_field_id')
and$state->get('media_library_field_type')
, respectively.Comment #14
seanB1. Fixed
2. Fixed (#3023767: Reuse or build on vertical tabs styling in media library)
3. Not really sure how backbone is going to make it better, but I don't have a lot of backbone experience yet. If you could elaborate a bit more on what/how you hope to improve I guess this seems like a great opportunity to learn something.
4. Fixed
5. Fixed, replaced
getFieldId()
andgetFieldType()
bygetOpenerId()
. We can use this generic ID for fields as well as the WYSIWYG integration at a later stage (or any ID really).Comment #15
phenaproximaCan this a bit more descriptive -- "Allowed media types, in display order", perhaps?
The label should probably be "Media type ID" or something like that.
I think we should probably rename both the service and the class it wraps. How about something like MediaLibraryUiBuilder (media_library.ui_builder)?
This logic should be in MediaLibraryState itself. Its constructor should probably require the $opener_id as a parameter, and throw \InvalidArgumentException if it's anything except a non-empty string.
This will match an invalid string like 'field:'. We should probably check, using a regex, that there are additional characters after the 'field:' prefix. Additionally, we could add a helper method to validate this (something like
getFieldIdFromState($state)
).I'm not sure how I feel about having the state object load the actual media types; maybe it should just return the IDs, and the calling code should do the loading.
This should probably be an implementation of AccessibleInterface::access(). Since we are returning a reason why access is forbidden, we can probably remove the logger calls.
Let's move this downwards so that we're only calling it when we actually need it.
We need to document this class more. We should explain what it's used for, why it extends ParameterBag (so that opener-specific parameters can be included, in addition to the parameters that apply to any usage of the media library), and what an "opener" is.
What will this return if the cardinality is unlimited? We need to at least document that in the method comment.
I don't think this belongs here; I think it violates the single-responsibility principle. Let's move this, and createUploadUrl(), to the widget class.
'target_bundles' can be null, signifying that all bundles are accepted. We need to handle this case, or array_keys() may generate errors here.
Comment #16
seanB1. Fixed.
2. Fixed. It stored the weight before but changed it via a value callback. Apparently, it is pretty hard to fix the way the tabledrag element stores stuff.
3. Fixed.
4. Fixed.
5. Fixed. Not sure if we should add helper methods for opener specific needs. I don't think the state should care about that.
6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.
10. Fixed.
11. Fixed. Added a create method for the state so the variable names are still encapsulated in the state.
12. Fixed.
Besides the above I also removed the unnessecary 8.6.0 fixture for the media module and renamed the update test. While working on the update test for #2988433: Automatically create and configure Media Library view and form displays it became clear to me that we will probably need more tests in the future and making them a bit more specific will help.
Comment #17
phenaproximaAfter some discussion with @seanB, I have decided that we don't need this; it can just be a simple array for now. I'd *like* to refactor the entire JavaScript side of this module as a Backbone view, but that's not a stable blocker or UX gate, and is very much a nice-to-have, whereas this issue is listed as a _must-have_ in the roadmap, and code freeze is two months away.
So let's just go back to a simple array called Drupal.MediaLibrary.currentSelection. Sorry for having wasted time on this.
Nit: For readability, can we put count after $toggleElements?
Also, a thought -- rather than manipulate individual elements in the button pane, what if we toggled the _entire_ $buttonPane based on the selection state? Would that make things any simpler?
This could probably be $toggleElements.filter('.media-library-selected-count').
Can we call Drupal.theme() once, at the top of this else block?
Should this be .closest()?
Not sure we need to use jQuery here -- won't
e.currentTarget.checked
work for this?I think we need to join the items with a delimiter, no? Alternately, we could use JSON.stringify to nicely and automatically encode it as
[34, 36, 43...]
Should be "Once the selection is updated..."
Can we attach this to the hidden input field's change event, so that when we change it (or trigger its change event ourselves), the button pane is automatically updated?
Perhaps this logic should also probably be part of the hidden input field's change event handler.
Can we change the word "apply" to "restore", and explicitly mention that we're loading the selected items from the hidden input field?
This can be collapsed into one line:
$query = MediaLibraryState::fromRequest()->all();
We should remove this message entirely, as it is no longer the case. In fact, what with this change, I'm not sure we're using BadRequestHttpException in this file at all.
Rather than create MediaLibraryState::fromRequest() several times in the same class, let's just add a new class property for it in the constructor. $this->state = MediaLibraryState::fromRequest($this->getRequest())
Great freaking documentation! This is exactly what I wanted :)
Can we actually list the machine names of the parameters here? For example: "media_library_opener_id: The opener ID is used to describe..."
Let's also explicitly mention that the allowed types should be an array of media type machine names, and that the selected type should be the machine name of a media type.
Let's make it impossible for outside code to construct this class directly. We can do that with a protected constructor:
s/modal/media library. We shouldn't assume that the media library has been opened in a modal.
We should validate our inputs here and throw exceptions if any of it is invalid (we'll also want a unit test to confirm that the validation works correctly).
media_library_opener_id should always be a non-empty string, as should media_library_selected_type. media_library_allowed_types should be an array of non-empty strings. And media_library_remaining must be numeric and non-null.
To ensure that the aforementioned validation runs, let's instead do something like this:
Then, we can call $this->set() to restore any additional parameters that were passed in.
This class should be marked explicitly internal. Personally, I think it should be final, and all non-public members should be private; this is not part of Drupal's API, and we should not give people the impression that is. (@internal is about as effective as wet tissue paper at preventing people from extending stuff like this). But let's hold off on that until a framework or release manager says we can.
I could be wrong, but doesn't $return_as_object default to FALSE in AccessibleInterface?
Uh...we can't add a cacheable dependency on something which is falsy. This should have died with a fatal error. The fact that the tests didn't catch this means it needs tests. :)
Let's also be sure this code path has tests.
I wish we could delegate access checking here to the view itself. But, that is a nice-to-have and could be done in a follow-up.
Let's move this below the count check, so that we're not loading entities we might not need.
If $state->getAllowedTypeIds() returns an empty array, this will load no types. Do we maybe want to change this to
$state->getAllowedTypeIds() ?: NULL
, and move it below the count check?What if no type is selected? We should probably mark the first link in the menu as the active one.
The ?? operator is PHP 7 only, so I'm not sure we can use it unless a framework or release manager says so.
I think we should change "enabled" to "allowed".
We still need to account for the possibility that target_bundles will be NULL, which will cause this code to error.
Pro-tip: if we're just getting IDs, it's nicer to do getStorage('media_type')->getQuery()->execute().
The comments here are useful, thank you!
We should probably change this to > 1, since we don't want to show the table if (for some reason), zero media types are enabled.
On second thought, we probably don't even need this check. If there's one media type in the table, what's the harm? Tabledrag will still work, the user just won't be able to drag things anywhere.
Should be "...we don't want to store this extra weight field..."
&$element should be type hinted as an array.
However...do we really need to do this at the form API level? What if we override the setSetting() function to handle this stuff instead? It might be much cleaner and easier to work with than form stuff.
Nit: JavaScript
Same.
Comment #18
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedPlease remember to add the accessibility tag whenever we're proposing a brand-new UI behaviour in JS, because there are certain to be some accessibility aspects to address. If accessibility isn't considered when writing a new JS behaviour, in all likelihood it won't be accessible. I found this issue because it was in the main media library roadmap as a must-have.
<div aria-live="polite">
should suffice.strong: edited my answer to have numbered points, at seanb's request, so they are easier to reply about. seanb also directed me to the related issue where the design update is described.
Comment #19
seanBThanks @phenaproxima and @andrewmacpherson. Rerolled the patch since #3020707: Streamline buttons in the media library widget landed (Yay!). Also fixed #17 in the attached patch. Going to take a look at the accessibility review next.
1. Fixed.
2. Fixed the nit. About the button pane toggle. I did that in a previous version of the patch, but that looked a bit awkward. The button pane has a background color which acts as some sort of bottom border for the modal. If we remove that entire element it just looks a bit weird. The height of the entire modal also changes a bit more, which causes things to move. The code might be a bit uglier, but the result looks quite a lot better.
3. Fixed.
4. Fixed.
5. Fixed.
6. Fixed.
7. The default separator is a comma. The hidden selection field already used a comma separated list of IDs to update the widget from the modal. Not sure if we need to change this to JSON.
8. Fixed.
9. Fixed. Good idea!
10. Fixed.
11. I think restore could be a bit misleading since the current selection is also applied to other tabs/views. I personally prefer 'apply'. We are also not loading the items from the hidden input field, we are using the
Drupal.mediaLibrary
array. While applying the selection the change event on the media items takes care of updating the selection in the hidden field. Which makes me realise that applying the selection is in a different behavior as the change event for the checkboxes. Since we need the change event handler to be attached to apply the selection correctly, we have to do everything in 1 behavior. Added some documentation.12. Fixed.
13. Fixed.
14. I don't think we can do this. The upload form is constructed in several places where the opener ID argument is not in the URL and the state is not relevant at all. For example when just calling the access check on the form.
15. Yay!
16. Fixed. Instead of machine name I used media type ID to be consistent.
17. Fixed.
18. Fixed. Added a kernel test.
19. Fixed.
20. Fixed. Marked internal, but I do see lot's of ways developers can extend the library in the future. Swapping out the media type vertical tabs for a list of taxonomy terms for example. Let's not make it part of the API for now, but I do think brave souls should be able to work with this (even though we don't encourage or support it).
21. Fixed. The custom access check for the route doesn't receive this parameter. So I have to change the default (which is confusing), or don't implement
AccessibleInterface
. I chose the latter and renamed the method tocheckAccess
since that method name showed up the most while looking for custom access checks on controllers (also to avoid confusion).22. Fixed. D'oh! Added a kernel test.
23. Fixed. Added a kernel test.
24. We have #2983179: [META] Implement stricter access checking for the media library to fix this. I think we need some advanced logic here to also check stuff related to the opener of the library (for example field access).
25. Fixed.
26. Changed the code a bit. But since we now have validation on the state creation I think we are good here.
27. We now have validation so this can no longer happen. Also added validation to make sure the allowed media types actually exist and that the selected media type is present in the list of allowed types.
28. According to https://www.drupal.org/node/2938726 8.7 will only support PHP 7. So this should not be an issue.
29. Fixed.
30. Fixed.
31. Fixed, thanks!
32. Yay!
33. Fixed.
34. Fixed.
35. That's what I thought, but apparently
setSetting()
andsetSettings()
are not used on the widget plugin. The form saves the values on theEntityFormDisplay
config entity. We could implement a hook to fix it, but not sure how we are going to find the settings for all media fields in the entity form display object. I've seen the paragraphs module do it through a validate method, but not sure if either of those options are better/easier. The form API does not seem to have a way to send input for a field but not process it.36. Fixed.
37. Fixed.
Comment #20
seanBJust looked at #18
1. We looked at vertical tabs, but they can only be used inside forms at the moment (they are form elements). The tabs are basically navigation between the different views. Not sure what the best way would be to fix this.
2. We should probably not use the standard AJAX links (which is why the whole modal is replaced) and add our own JS to only replace the library content and keep the menu.
3. Fixed. Let's always display the buttons.
4.1/4.2 Fixed. Added the aria-live attribute.
4.3 Let's discuss this together with point 1 and 2 to think of a complete solution surrounding the modal navigation.
5.1 This is nessecary to make sure the selection of the user respects the field cardinality. This is important because the field validation will fail otherwise. That means you are immediately presented with an error after selecting and we want to prevent that I think.
5.2 That is correct, the disabled attribute is added.
5.3 Fixed. The selection count now shows: '0 of 5 items selected' to help the user understand. For unlimited field we just show the count '1 item selected'.
5.4 We should probably prevent the user from submitting too many selected items. Not sure what the best way would be to do that? Could you explain a bit more what the problem is with the disabled attribute? Just curious :)
6.1 We found that different fields could have multiple media types. You should be able to sort them how you want (depending on the type of site you are building).
6.2 I think in the mean time you found the screenshot of where site builders can configure this :)
6.3 By default to first (top) tab is selected.
6.4 We can't force consistent navigation since different fields could have different media types enabled. That being said, we could add a description to the widget settings tabledrag to advise site-builders to order the media types the same way for all media library fields?
6.5 It is a way to sort the order of the media type navigation in the modal. By default the first tab is opened. So it is mostly to allow site builders to configure the default media type to open, but prioritising the rest of the navigation seems useful as well.
If I'm not mistaken (and implemented to correct solution for the other points), the things we still need to address are:
1 / 2 / 4.3 / 5.4
@andrewmacpherson could you review the changes in the patch and confirm everything besides the mentioned points is addressed?
Comment #21
seanBI think the attached patch should solve #18.1 & #18.2.
For #18.4.3 whe have #3023809: Add a selection overview to the media library widget modal to improve the selection management for users. Maybe that issue should get a higher priority. The media library view has a pager and filters so we also can't guarantee the selected items are directly visible on the tabs.
So I guess the only thing left to discuss is whether disabling items is a good idea or not. Maybe with the aria-live message now telling users '5 of 5 items selected' this is not as big of a problem as before?
Comment #22
phenaproximaNice docs!
Can this be Drupal.MediaLibrary (to keep it in line with similar client-side systems like Drupal.Ajax)?
We should probably explain why this is necessary.
Nit: Is it possible for us to pass the success callback to Drupal.ajax(), rather than setting it after the fact?
We don't use array() syntax any more :)
We should probably do getQuery()->condition('id', $allowed_media_type_ids)->execute() here, rather than load all media types.
Can we move all the validation logic to a protected static function? I ask because, if we want to add other static create methods in the future, it'll be nice to be able to validate without necessarily needing to instantiate.
I'm not sure about the name of this method. How about buildWrapper() or buildMainInterface() or buildUi()?
Same sort of thing here. How about something like buildLibraryContent() or buildLibraryViewTab() or similar?
Let's explicitly set the maximum cache age to 0. I can't imagine we would ever want to cache this result.
I think there is some constant for _wrapper_format, and we should use it here (I believe it's on MainContentViewSubscriber). Also, if remove() is chainable, we can collapse this to one line: $query = $state->remove('_wrapper_format')->all();
This is the only place we're using the entity type manager, so why don't we just inject the entity storage handler itself? If we have to unit test this class, that will make it a bit easier to mock things.
We can probably merge this method directly into buildContent() (or whatever we end up renaming it). With the current refactoring, there's no real benefit to having it in its own method.
Comment #23
seanBJust worked at fixing #22
1. Yay!.
2. Fixed.
3. Fixed.
4. Doesn't seem to work I'm afraid. We could extend Drupal.Ajax, but I think what we currently have is better. There are other places in core doing the same thing.
5. D'oh! Fixed.
6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.
10. Fixed.
11. Fixed, we can't chain those though.
12. I don't think we can do that in a service?
13. Even though it might be a little overkill at the moment, but when we are going to add the form in #3023802: Show media add form directly on media type tab in media library, it might be better to have this as a separate method.
Comment #24
phenaproxima@seanB and I demoed this to the UX call yesterday. It was an unusually full call, and the feedback on the current patch was positive! I think we have the all-clear to land this as soon as possible. A couple of additional takeaways:
So, I think we're pretty much full speed ahead on this one. Let's land it.
Comment #25
phenaproximaPartial review:
Drupal.MediaLibrary is essentially going to be the wrapper for the application library. So let's just say "Wrapper object for the current state of the media library".
This comment should be moved to directly above currentSelection, I think, so that it's obvious that we're using that specific array to store the selection.
Can we remove "views pages"?
Nice. I like that we're explicitly stating that this is an accessibility improvement. Can we also open a follow-up issue to explore moving this code into the core dialog system for better accessibility in general? (We'll also need to reference that follow-up in this comment.)
I wonder if we should move the content of this event handler into Drupal.MediaLibrary, since that's the object which should contain the overall "application" logic. Maybe something like
Drupal.MediaLibrary.loadTabContent(link_element)
? Within such a function, we could easily get ahold of the complete menu with$(link_element).closest('.media-library-menu')
.Nit: AJAX should always be all-caps.
Same here.
I think this function should also be moved into Drupal.MediaLibrary as Drupal.MediaLibrary.updateSelectionInfo() or something.
Idea: Let's pass the entire currentSelection into Drupal.theme('mediaLibrarySelectionCount'), for greater theming flexibility.
We don't need this. If there are no items, the subsequent stuff will basically just be a no-op.
JavaScript's Array object has a .includes() method which does this this, but it's not supported in IE :( Sadness!
We should move this above the if check, since we use the value in both logic branches.
Rather than keep repeating Drupal.MediaLibrary.currentSelection in this function body, can we just say
const currentSelection = Drupal.MediaLibrary.currentSelection
at the top of the function and use that?I think these should also be moved to Drupal.MediaLibrary. Or, better yet, we could turn them into a single toggleItems() method by taking advantage of
$.toggleClass('media-library-item-disabled', is_disabled)
. See http://api.jquery.com/toggleClass/#toggleClass-className-state.Can we remove "in the view"? It's a bit confusing.
Do we need the TRUE here? Why not let the config system validate this stuff?
I feel like this could be combined into a single route and public build() method, which delegates to protected methods buildUi() and buildLibraryContent(). We should seek to reduce the amount of public things we expose, since
@internal
is about as effective a barrier as wet Kleenex in practice.The regex should probably begin with ^.
Comment #26
phenaproximaAnother partial review, picking up from where #25 left off:
So this is a change in logic; previously, loadMultiple() could be passed a NULL, but now it will always be given an array. Do we have test coverage for this?
Supernit: This line is 81 characters long. :)
I think it would be clearer if we change "containing" to "of".
"of which the" should be "whose".
want --> wants
We should document that passing a negative number here will allow unlimited items to be selected.
What are we using StringTranslationTrait for?
To be absolutely sure this visibility reduction works consistently (if it doesn't, it could cause this patch to be reverted), let's be sure to run tests for every PHP 7.x version testbot currently supports.
"Creates"
I don't think the @throws is supposed to go here, since this method never actually throws anything.
There is no need for this method to return anything. If it doesn't throw an exception, validation succeeded.
This will not catch an opener ID like " ". We should probably change this to a regex.
Same here.
And here.
This should be using $this->t().
Nit: I think this probably needs to be outdented a bit.
Does this return media types, or media type IDs? The doc comment does not match the method name.
Let's explicitly mention that this list has been pre-sorted by the user via the settings form.
Can we rephrase this first sentence? How about "Some of the media types may no longer exist, and new media types may have been added that we don't yet know about."
Should be "...that no longer exist."
Rather than put 99% of the function body in an if statement, let's just early-return $form if count($media_type_ids) is less than or equal to 1.
Can we name this variable $weight? That would more clearly reflect how we're using it.
Can't we just use reset($allowed_media_type_id) to get the selected type?
This regex should also probably start with a ^. We use it elsewhere too, so it might be worthwhile to make it a constant somewhere?
Comment #27
seanBNew patch with fixes for #25 and #26:
#25.1 Fixed.
#25.2 Fixed.
#25.3 Fixed.
#25.4 Created #3026636: Allow AJAX links to replace a specific selector
#25.5 At the moment Drupal.MediaLibrary doesn't contain any logic and I think we should try to keep it that way. I did create a
media_library.ui.es6.js
file to remove to non-widget specific media library JS frommedia_library.widget.es6.js
. Having 2 separate files might make it easier to understand what is going on.#25.6 Fixed.
#25.7 Fixed.
#25.8 Same here, I think we should keep the logic out of Drupal.MediaLibrary. I did rename the method and update the docs.
#25.9 Fixed.
#25.10 Fixed.
#25.11 Sadness indeed.
#25.12 Fixed.
#25.13 Fixed.
#25.14 Toggle is something different. We want to enable the disabled items in some cases, and disable the enabled in other cases. It's not just a question of reversing the current state.
#25.15 Fixed.
#25.16 Fixed.
#25.17 We have 2 separate actions: open the library / open a tab. The fact that a default tab is included in the "open library" action makes it a little weird, but I personally think these are different actions and they need a different route. Maybe if we had #3026636: Allow AJAX links to replace a specific selector we could replace the dialog content and set the focus via a simple link. That would remove the need for a separate route.
#25.18 Fixed.
#26.1 Fixed. Removed the
?: []
fromgetAllowedTypeIds
since the allowed types are now a required parameter on the library. And we definitely have test for that part.#26.2 Fixed.
#26.3 Fixed.
#26.4 Fixed.
#26.5 Fixed.
#26.6 Fixed.
#26.7 Fixed.
#26.8 OK!
#26.9 Fixed.
#26.10 Fixed.
#26.11 Fixed.
#26.12 Fixed.
#26.13 Fixed.
#26.14 Fixed.
#26.15 Fixed.
#26.16 Fixed.
#26.17 Fixed. Just the IDs.
#26.18 Fixed.
#26.19 Fixed.
#26.20 I don't think that is true. The media type could still exist but could be removed from the type configured for the field. I changed 'available' to 'configured' to make this more explicit.
#26.21 Fixed.
#26.22 Fixed.
#26.23 Fixed.
#26.24 Fixed.
Comment #29
seanBD'oh! Forgot to add the new media_library.ui.js file.
Comment #31
phenaproxima"trigger" --> "triggers"
On second thought, let's not make this a constant. Let's just add a public static method to check if an opener ID belongs to MediaLibraryWidget, and keep the regexes and prefixes internal. Let's call it MediaLibraryWidget::isOpener($opener_id).
If I'm not mistaken, FormBase has a getRequest() method we can use to pass the request to MediaLibraryState::fromRequest()...
Below the @internal, let's add a disclaimer: "This class is an internal part of the media library field widget and should not be instantiated or used by external code."
In all these checks, let's move the is_string() call before empty(trim()), because otherwise we risk passing a non-string to trim().
Let's add the same @internal disclaimer here as we did for MediaLibraryState.
I'm not certain this will translate correctly for all languages. Shouldn't we do something like '@title (active tab)' to increase translatability?
These variable names are really confusing and make the function harder to understand. I think we should rename $media_type_ids to $tab_order, and $configured_media_type_ids to $allowed_media_type_ids.
I think this is meant to be count($media_types) !== 1
The id attribute should only be a string, never an array. :)
Can we rename this variable to $url_options, so as to be clearer what it really contains?
I wonder if we could instead do something like:
What does this mean? Can the comment be expanded?
This looks like a good place for array_map(), to reduce the number of variables.
We don't need $expected_link_titles to be its own variable; let's just pass it as the second argument to assertSame().
Should this maybe use $assert_session->buttonExists()->press()?
I think $assert_session->elementExists('named', ['link', 'text of the link'])->click() might be more readable here.
Can we replace this with $assert_session->fieldExists()->setValue() and $assert_session->buttonExists('Save')->press() instead?
We don't need $twin_button to be its own variable.
Same here.
It's more readable to find links by their text. $assert_session->elementExists('named', ['link', 'Type three'])->click()
We don't need to assert that the element exists; elementTextContains() will do it implicitly.
This should be @inheritdoc, and protected.
Why does this need to be cloned?
Same here.
We can restructure this method a bit to make it less verbose:
Comment #32
seanBNew patch to fix the comment in #31:
1. Fixed.
2. Fixed. Since we need to extract the field ID as well I created a method getOpenerFieldId() that could return NULL. Added a protected static variable for the field prefix.
3. Fixed.
4. Fixed.
5. Fixed.
6. Fixed.
7. Fixed (in the JS as well).
8. Fixed. Not sure about
$tab_order
though? Chose to use$sorted_media_type_ids
instead.9. Fixed.
10. Fixed.
11. Fixed.
12. Fixed.
13. Fixed.
14. Fixed.
15. Fixed.
16. Fixed.
17. Fixed.
18. Fixed.
19. Fixed.
20. Fixed.
21. Fixed.
22. Fixed.
23. Fixed.
24. We want to restore the original view later, so I think we need a copy of the original object before we start changing it.
25. Fixed.
26. Fixed.
Als reverted #17.7. That seemed to fail in some PHP 7 versions.
Let's make it impossible for outside code to construct this class directly. We can do that with a protected constructor:
Comment #33
phenaproximaRemoving this is fine and good, but it means we need to validate the parameters via the constructor. :)
Let's add a @see referencing MediaLibraryState, so that people can look up what an "opener ID" is if they need to.
Comment #34
seanBOk, new patch attached.
#33.1 Fixed.
#33.2 Fixed.
Comment #35
alexpott@phenaproxima asked me to provided feedback on whether we should
I think we should have a single route with a query parameter to determine if we return the or not. I think this is the better way to go because the only reason we seem to be discussing this is to work around a limitation discovered in the ajax api and this is closer to the ideal solution.
So for example:
/media-library/modal/media_type?chrome
would return the media list with the GUI and/media-library/modal/media_type
would return just the media list.Comment #36
phenaproximaThanks for settling the question, @alexpott. Kicking back to Needs work for that.
Comment #37
seanBNew patch to remove the extra route and add a parameter (specific for just the
MediaLibraryUiBuilder
).Also added some minor changes to the
MediaLibraryState
to make the validation run before the parent constructor.Comment #38
phenaproximaI think this patch does what we need it to do for now. We don't need it to be perfect; we need it to be good enough to get this module to stable. Therefore, I'm comfortable saying that, by my own standards, it's RTBC.
So, what we still need here is:
Comment #39
lauriiiI'm not 100% done with my review yet but here's what I could find so far.
We shouldn't use ids in selectors for theming.
This doesn't work with default Drupal dialog styles.
I don't see any mention about supporting the default Drupal Core dialog styles. Seven has it's own dialog design. Should we have separate designs for Drupal Core and Seven?
Would it be possible to add classes to these elements? That would allow us to use a single class for targeting these elements.
What is this property used for here?
When uploading next patch for this issue, please run
yarn prettier
command which will format this file according to our coding standards.Comment #40
seanBThanks @lauriii
#39.1. Fixed.
#39.2. Moved it to the seven theme specific CSS file.
#39.3. Not really sure, there is some minimal styling on the module to make sure the layout is as it should be. The colors, margins, paddings etc are currently created for the seven theme only.
#39.4. Fixed for the links. The 'links' render element doesn't seem to support setting a class on the li though.
#39.5. The z-index 1 on the active links is needed to make the box shadow overlay on tab of the other links. The z-index 2 on focus has been removed and did not seem to do anything.
#39.6. Fixed. Also ran CSSComb which seems to have moved existing CSS around as well.
Comment #41
lauriiiComment #43
seanBRerolled #40.
Comment #44
benjifisherWe discussed this at the weekly Drupal usability meeting - 2019-01-22. Thanks for presenting this issue there! The consensus was very positive: generally, we like the vertical tabs and the text indicating how many items were selected.
Speaking for myself, not the group, I have a few suggestions.
In general, I like the vertical tabs. I think they convey the idea that we can switch from one to another without losing our selection. I think that adding the selected count to each tab would reinforce that.
Comment #45
seanBThanks @benjifisher! I updated the screenshots in the IS since there were some minor changes since the last ones were uploaded.
Regarding your feedback:
1. We thought about that. The problem is that clicking the tab would not necessarily show the selected items. For example when using the pager or filters. In those cases showing the number of selected items below a tab title could be confusing. I strongly believe #3023809: Add a selection overview to the media library widget modal would be the best solution to manage your selection (as a tab, or at least as a link on the 1 of 5 items selected text).
2. That has been fixed in #3020707: Streamline buttons in the media library widget. Removed the screenshots of the empty/filled widget since we are not changing that in this issue.
3. The text in the latest patch is X of Y items selected. Hopefully that would solve it in a generic and accessible way.
4. When the media library closes, the selection is lost. Entity references allow users to add the same entity multiple times. I am not sure we should change that. The number of open slots is updated though, so the field cardinality is respected.
5. We have #3023809: Add a selection overview to the media library widget modal. We can discuss whether this should be an actual tab or simply a link on the X of Y items selected text. I think we at least need to update the priority of this issue to 'should have' in our roadmap, since this came up multiple times in the a11y and ux reviews.
Comment #46
lauriiiWe should add @todo here to fix this either after #3029227: Allow setting attributes to li element from render array for links.html.twig or when this is being moved to Seven.
This should be named using BEM pattern. This class could be
media-library-menu__link
for example.This documentation should be adjusted to comply with our JavaScript documentation standards.
We probably should make this overridable by creating a theme function.
Comment #47
seanB#46
1. Fixed.
2. Fixed.
3. Fixed.
4. Not really sure about this. The (active tab) span is added for a11y purposes, not for theming, and wrapped in a t() function (since the text order might be different in some languages). This is now the same text as what is added by default in
MediaLibraryUiBuilder::buildMediaTypeMenu()
, and we probably don't want that to be different.Comment #48
phenaproximaIn retrospect, I don't know why I added the "needs release manager review" tag. Probably for completeness' sake. I'm removing it.
Comment #49
seanBI think the JS has also been reviewed by phenaproxima and lauriii. Since lauriii removed the 'Needs frontend framework manager review' tag, I think we can also remove the 'needs JavaScript review' tag.
Comment #50
lauriiiWe should document behaviors according to our JavaScript coding standards.
We should attach this using
js-
prefixed class.Nit: double space
We shouldn't include the element type in the selector.
Comment #51
seanB#50:
1. Fixed.
2. Fixed.
3. Fixed.
4. Fixed.
Comment #52
phenaproximaThis patch has been extensively reviewed by several people, including myself, and demonstrated more than once to the UX team, which received it quite positively. It has cleared review by the front-end framework manager and product managers, and does not introduce any API changes (or, indeed, any API at all), so does not need review from framework or release managers. Follow-ups are filed, and it looks like all feedback is addressed so far.
The one sticking point is accessibility review, which has not been officially completed. However, we have been unable to get secure enough time from the community's accessibility experts to complete that part, and seeing as how this issue is an 8.7 target which is blocking several other 8.7 targets (see #2834729: [META] Roadmap to stabilize Media Library), I'm removing the accessibility review tag for now and marking this issue RTBC. If there are accessibility issues which surface after this patch has been committed, we are entirely open to fixing those in follow-ups, and we are also fine with such issues being stable blockers if need be.
We gaan.
Comment #53
Gábor HojtsyComment #54
Gábor HojtsyRe: the accessibility review, looking at #20 and #21 as far as I see all but one of the issues raised were fixed. So we should be in a pretty good shape AFAIS.
Comment #55
lauriii@Gábor Hojtsy those are computer generated JS files. All of our pre-existing computer generated JS files have the same problem.
Comment #56
Gábor HojtsyOk. Went to commit this but does not apply to core/modules/media_library/media_library.install, core/modules/media_library/media_library.module and core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php anymore unfortunately.
Comment #57
seanBRerolled #51.
Comment #58
seanBEverything is still green, so I guess we can go back to RTBC.
Comment #59
Gábor HojtsyUpon attempting to commit there were a few yarn and one phpcs error. These seemed too numerous to fix on commit.
Comment #60
seanBSorry about that. I ran
yarn prettier
and fixed the PHPCS error.Comment #61
seanBRemoved a PHP 7 only
??
operator.Comment #62
phenaproximaOK, here we go again.
Comment #63
phenaproximaUh-oh. Kicking back for PHP 5.x failures...
Comment #64
seanBStrange enough, the reason for the test fail did not seem to be introduced by this patch, but luckily changing the tests managed to show this issue. The weights for the added media items were not properly set when adding new items to the widget, which could lead to a "random" order of added media items.
Attached patch should fix this.
Comment #65
phenaproximaThat's a big wall of green. I'm satisfied.
Comment #66
xjmThis is not a normal issue. :)
Comment #67
larowlanCopying from #3023802: Show media add form directly on media type tab in media library
There is a lot of work gone into this, its really appreciated.
do we need RTL equivalents here?
should we use getElementById here instead of jQuery?
note to self, do we check access on this
we should pass along the request here
the only place I see this used is in the routing and in tests.
we deliberately decided not to make controllers services in the D8 design process because we didn't want to bloat the service container.
I don't see a reason to do it here either.
Am I missing something?
We have ContainerInjectionInterface if you need to inject stuff into the controller and we have the ControllerResolver if you need to instantiate the class in a test.
Why do we hide them with CSS and not use form modes?
we're calling this in the array_map which means we're doing the getStorage and the getSourceFieldame each time, but really we could do it once and pass it along
I think we need an anti-tamper token here too
should we inject this?
i feel we should be passing the request object here, this is a non-static method, we have container injection, we should fetch it and pass it along
Once we do that, we make the parameter required and it no longer needs the singleton
should we inject here instead of using the singleton?
yeah, defs should be adding an anti-tamper token to prevent people meddling with the allowed types
the last one is always selected?
are we sure a global static is the right choice? - from the uses I see, could be a constant
what controls are in place to prevent someone from editing the dom on this and adding in references to media entities they cannot access?
we should pass on the request here, obtained from DI on the plugin
Comment #68
dwwGreat review! Agreed that a ton of fantastic work has already gone into this (and related) patches. Many thanks for that. But alas, no longer RTBC given #67. ;)
Comment #69
seanB1. Fixed.
2. Fixed.
3. Not sure what is meant by this?
4. Fixed.
5. In #3023797: Let users choose what to do after selecting and/or adding items in the media library and #3023801: Allow newly uploaded files to be deleted from the media library without saving them we also need to build the media library UI from the media add forms. Thats why we decided to make this a service.
6. This is relevant for #3023802: Show media add form directly on media type tab in media library, but since the alt text, title and upload field are provided by a single widget, we need to hide the relevant part via CSS. We also have #2987921: Media Library add form should suppress extraneous components of image fields using a form alter, not CSS to improve this.
7. Wil fix in #3023802: Show media add form directly on media type tab in media library
8. Since this is “just” a widget for an entity reference field, the validation of that field ensures users can not select anything they shouldn’t. Same as with the autocomplete widget. Users can type whatever they want in there as well. I think the media library shouldn't really care about that.
9. Fixed. Since it is a value object I don't think we should add dependencies. That being said, we should probably remove the dependency and the validation if a type exists.
10. Fixed.
11. Fixed.
12. See 8.
13. The query is used within the foreach loop to generate the link. So each link has a different 'selected type' in it's URL parameters.
14. We did this to keep it protected for this class only. I don't have a strong opinion on this though.
15. I believe ValidReferenceConstraintValidator should do this.
16. Fixed.
Already had a quick chat with larowlan about the tamper tokens:
Comment #70
seanBScreenshots of the LTR vs RTL display.
Media library selection:
Add media form:
Comment #71
phenaproximaThis looks pretty good to me. I think @larowlan's feedback has been adequately addressed. Let's get it in.
Comment #72
dwwVery minor nit: do we have to use
\Drupal::request()
here? This is a plugin, so we should be able to inject this dependency if we wanted to. Definitely not a blocker, just curious on the rationale. I guess the existing code does this, so fixing it is out of scope for this patch. Follow-up? Won't fix? ;)Otherwise, definitely +1 to RTBC. This looks great!
Thanks,
-Derek
Comment #73
seanBThat code is in
updateWidget()
which is a static method.Comment #74
knyshuk.vova CreditAttribution: knyshuk.vova at Internetdevels, Drupal Ukraine Community commentedThe patch looks good and applies successfully. +1 for RTBC.
Comment #76
Gábor HojtsySuperb thanks all for the thorough reviews and diligent implementation.
Comment #77
seanBYAY! Thanks everyone.
Comment #78
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedRe #52:
Please don't do this. Removing the "needs accessibility review" tag effectively removed it from my attention span, which means it won't get a review! Note that the 8.7 target is an arbitrary roadmap goal, which does not trump the accessibility gate. The fact that the accessibility topic maintainers are volunteers with limited time has no bearing on the gate.
But if you commit an issue with accessibility problems, to unblock others which get committed quickly, will it still possible to revert this issue? I accept that may not be desirable to revert a big change like this issue, and unblocking other issues has a lot of merit. But an accessibility issue will potentially become harder to fix once other issues have built on top of it. Moving accessibility issues to follow-ups shouldn't be done lightly just to keep things moving.
That said, it depends on what the accessibility issue is....
Re. #54, Gábor said:
You mean from #18? I can't tell from the issue comments which one remains. Is there a follow-up issue for it?
Comment #79
Gábor HojtsySo #18 is referred in #20 and #21.
That said, that remaining point may not need a followup.
Comment #80
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks Gábor.
I found #3023809: Add a selection overview to the media library widget modal already, added some implementation suggestions there. It wasn't tagged accessibility.
The disabled checkboxes issue is an odd one. Semantically, an
<input disabled>
is OK. The part that concerned me was dynamically disabling (or re-enabling) a large number of inputs at once. Some assistive tech will filter disabled inputs out of the "list of form elements" feature, and they will also be excluded from the tabbing order. That said, a user may still find the disabled inputs if using a screen reader in "browse mode" as distinct from "form mode". It rather depends on how much confidence or knowledge a user has with their assistive tech. They're no more likely to be power users than anyone else. On the whole I think it's safer to let the user make too many selections, than pull available options out from under them. The number doesn't actually matter until you submit the entire thing.Update: I should add, on reflection, I don't exactly know what to do about this disabling checkboxes issue. I don't think there's a right or wrong answer for WCAG, or usability in general. So this one doesn't have to be addressed as a stable blocker for media library. Instead it's a wait and see once this reaches a bigger audience, or usability study.
Comment #81
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedAdded a follow-up, the ARIA live region for the selection count isn't managed correctly. I wasn't explicit enough in #18.4...
I meant the text node should change, but the same aria-live DIV wrapper is used throughout the life of the dialog. Aria is fiddly that way.
Comment #83
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedfixing accessibility tag