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.

  1. 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.
  2. If there is only 1 media type configured for a field, we hide the verticle tabs.
  3. 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.
  4. 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:

  1. 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.
  2. Type argument for the media library view so each vertical tab only shows media of the selected type.
  3. Only show the 'Select media' button when there is actually something selected.
  4. Show the number of selected items in the button pane.
  5. Make sure the selection persists between tabs.
  6. Make sure the selection is cleared when items are selected or the modal is closed.
  7. Make sure media items are disabled for each tab when the maximum number of items for the field is selected.
  8. 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
 Modal without anything selected

After: Modal without anything selected
 Modal without anything selected

Before: Modal with items selected
 Modal with items selected

After: Modal with items selected
 Modal with items selected

Before: Modal with max. item selected
 Modal with max. item selected

After: Modal with max. item selected
 Modal with max. item selected

After: Selection persists in other tabs (new feature)
 Selection persists in other tabs

After: Widget settings to sort vertical tabs style menu in modal (new feature)
 Widget settings to sort vertical tabs style menu in modal

CommentFileSizeAuthor
#70 add-rtl.png540.56 KBseanB
#70 add-ltr.png550.79 KBseanB
#70 library-rtl.png846.09 KBseanB
#70 library-ltr.png844.88 KBseanB
#69 3020716-68.patch147.68 KBseanB
#69 interdiff-64-68.txt14.16 KBseanB
#64 3020716-64.patch146.65 KBseanB
#64 interdiff-61-64.txt1.12 KBseanB
#61 3020716-61.patch145.9 KBseanB
#61 interdiff-60-61.txt893 bytesseanB
#60 3020716-60.patch145.81 KBseanB
#60 interdiff-57-60.txt2.22 KBseanB
#57 3020716-51-reroll.patch145.66 KBseanB
#51 3020716-51.patch146.14 KBseanB
#51 interdiff-47-51.txt14.6 KBseanB
#47 3020716-47.patch139.96 KBseanB
#47 interdiff-43-47.txt3.46 KBseanB
#45 after-modal-selection-persists.png102.22 KBseanB
#45 after-modal-max-items-selected.png811.46 KBseanB
#45 after-modal-items-selected.png763.91 KBseanB
#45 after-modal-no-selection.png762.22 KBseanB
#43 3020716-40-reroll.patch139.73 KBseanB
#40 interdiff-37-40.txt9.08 KBseanB
#40 3020716-40.patch139.48 KBseanB
#39 Screen Shot 2019-01-24 at 18.42.08.png33.23 KBlauriii
#37 interdiff-34-37.txt6.05 KBseanB
#37 3020716-37.patch135.04 KBseanB
#34 interdiff-32-34.txt1.94 KBseanB
#34 3020716-34.patch134.7 KBseanB
#32 interdiff-29-32.txt40.28 KBseanB
#32 3020716-32.patch134.42 KBseanB
#29 interdiff-27-29.txt5.79 KBseanB
#29 3020716-29.patch131.98 KBseanB
#27 interdiff-23-27.txt44.35 KBseanB
#27 3020716-27.patch126.19 KBseanB
#23 interdiff-21-23.txt15.19 KBseanB
#23 3020716-23.patch127.57 KBseanB
#21 interdiff-20-21.txt12.98 KBseanB
#21 3020716-21.patch126.21 KBseanB
#20 interdiff-19-20.txt15.13 KBseanB
#20 3020716-20.patch121.75 KBseanB
#19 interdiff-16-19.txt49.06 KBseanB
#19 3020716-19.patch120.58 KBseanB
#16 interdiff-14-16.txt206.87 KBseanB
#16 3020716-16.patch108.29 KBseanB
#14 interdiff-11-13.txt10.08 KBseanB
#14 3020716-13.patch278.8 KBseanB
#11 interdiff-8-11.txt216.29 KBseanB
#11 3020716-11.patch279.01 KBseanB
#8 interdiff-2-8.txt49.61 KBseanB
#8 3020716-8.patch72.13 KBseanB
#7 before-modal-max-items-selected-2.png965.27 KBseanB
#7 before-modal-items-selected-2.png978.16 KBseanB
#6 after-widget-settings.png80.18 KBseanB
#6 after-widget-selected.png774.03 KBseanB
#6 before-widget-selected.png774.03 KBseanB
#6 after-modal-selection-persists.png114.92 KBseanB
#6 after-modal-max-items-selected.png924.33 KBseanB
#6 before-modal-max-items-selected.png973.83 KBseanB
#6 after-modal-items-selected.png942.1 KBseanB
#6 before-modal-items-selected.png982.23 KBseanB
#6 after-modal-no-selection.png922.64 KBseanB
#6 before-modal-no-selection.png967.4 KBseanB
#6 after-widget-empty.png46.71 KBseanB
#6 before-widget-empty.png46.71 KBseanB
#6 interdiff-2-6.txt49.61 KBseanB
#6 3020716-6.patch125.46 KBseanB
#2 3020716-2.patch66.84 KBseanB
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanB created an issue. See original summary.

seanB’s picture

Here is a first patch to add the following (incl tests):

  1. 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.
  2. Type argument for the media library view so each vertical tab only shows media of the selected type.
  3. Only show the 'Select media' button when there is actually something selected.
  4. Show the number of selected items in the button pane.
  5. Make sure the selection persists between tabs.
  6. Make sure the selection is cleared when items are selected or the modal is closed.
  7. Make sure media items are disabled for each tab when the maximum number of items for the field is selected.
  8. 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.
phenaproxima’s picture

Issue tags: +Needs screenshots

This 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.

phenaproxima’s picture

  1. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
    @@ -380,6 +380,48 @@ display:
    +      arguments:
    +        bundle:
    +          id: bundle
    +          table: media_field_data
    +          field: bundle
    

    Will 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.)

  2. +++ b/core/modules/media_library/config/schema/media_library.schema.yml
    @@ -0,0 +1,14 @@
    +      sequence:
    +        type: mapping
    +        label: 'Media type weight'
    +        mapping:
    +          weight:
    +            type: integer
    +            label: 'Weight'
    

    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.

  3. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,63 @@
    +.media-library-menu {
    +  background-color: #e6e5e1;
    +  display: block;
    +  padding: 0;
    +  margin: 0;
    +  width: 600px;
    +  max-width: 20%;
    +  border-bottom: 1px solid #ccc;
    +  line-height: 1;
    +}
    

    Can we reuse or build on any vertical tabs styling? It seems like we might be repeating a lot of things here?

  4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,11 @@
    +  Drupal.media_library = {
    +    selection: [],
    +  };
    

    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);

  5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +85,183 @@
    +          const $form = $(e.currentTarget).parents('.media-library-views-form');
    +          const position = Drupal.media_library.selection.indexOf(e.currentTarget.value);
    +          const checked = $(e.currentTarget).is(':checked');
    +          if (checked && position === -1) {
    +            Drupal.media_library.selection.push(e.currentTarget.value);
    +          } else if (!checked && position !== -1) {
    +            Drupal.media_library.selection.splice(position, 1);
    +          }
    +
    

    This looks a bit complicated. It could use a comment.

  6. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +85,183 @@
    +  Drupal.behaviors.MediaLibraryModalApplySelection = {
    

    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.

  7. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +85,183 @@
    +          if (
    +            settings.media_library &&
    +            settings.media_library.selection_remaining &&
    +            Drupal.media_library.selection.length ===
    +            settings.media_library.selection_remaining
    +          ) {
    

    IMHO, this kind of thing is another reason to encapsulate the client-side media library logic in a JavaScript class.

  8. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +85,183 @@
    +          .on({
    +            'dialog:aftercreate': () => {
    +              const $buttonPane = $(
    +                '.media-library-widget-modal .ui-dialog-buttonpane',
    +              );
    +              if (Drupal.media_library.selection.length === 0) {
    +                $buttonPane
    +                  .find('.media-library-select, .media-library-selected-count')
    +                  .hide();
    +              } else {
    +                const selectItemsText = Drupal.formatPlural(
    +                  Drupal.media_library.selection.length,
    +                  '1 item selected',
    +                  '@count items selected',
    +                );
    +                const $selectedItems = $buttonPane.find(
    +                  '.media-library-selected-count',
    +                );
    +                if ($selectedItems.length) {
    +                  $selectedItems.html(selectItemsText);
    +                }
    +                else {
    +                  $buttonPane.append(
    +                    `<div class="media-library-selected-count">${selectItemsText}</div>`,
    +                  );
    +                }
    +              }
    +            },
    +          });
    

    This is hard to read. Why is an object being passed to .on()?

  9. +++ b/core/modules/media_library/media_library.services.yml
    @@ -0,0 +1,4 @@
    +services:
    +  media_library.modal:
    +    class: Drupal\media_library\MediaLibraryModal
    +    arguments: ['@request_stack']
    

    Why is this in a service? Seems kind of unnecessary...

  10. +++ b/core/modules/media_library/src/MediaLibraryConfiguration.php
    @@ -0,0 +1,99 @@
    +class MediaLibraryConfiguration extends ParameterBag {
    

    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?

  11. +++ b/core/modules/media_library/src/MediaLibraryConfiguration.php
    @@ -0,0 +1,99 @@
    +  /**
    +   * Returns the widget ID of the media field.
    +   *
    +   * @return string
    +   *   The field widget ID.
    +   */
    +  public function getWidgetId() {
    +    return $this->get('media_library_widget_id');
    +  }
    

    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.

  12. +++ b/core/modules/media_library/src/MediaLibraryConfiguration.php
    @@ -0,0 +1,99 @@
    +    return MediaType::loadMultiple($media_types);
    

    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.

  13. +++ b/core/modules/media_library/src/MediaLibraryConfiguration.php
    @@ -0,0 +1,99 @@
    +  /**
    +   * Get media library dialog options.
    +   *
    +   * @return array
    +   *   The media library dialog options.
    +   */
    +  public static function dialogOptions() {
    +    return [
    +      'dialogClass' => 'media-library-widget-modal',
    +      'title' => t('Media library'),
    +      'height' => '75%',
    +      'width' => '75%',
    +    ];
    +  }
    

    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.

  14. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,151 @@
    +/**
    + * Controller which renders the media library modal.
    + */
    +class MediaLibraryModal {
    

    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.

  15. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,151 @@
    +  public function build() {
    +    $configuration = MediaLibraryConfiguration::fromRequest();
    +    return [
    +      'wrapper' => [
    +        '#type' => 'html_tag',
    +        '#tag' => 'div',
    +        '#attributes' => [
    +          'class' => ['media-library-wrapper'],
    +        ],
    +        'menu' => $this->getMediaTypeMenu($configuration),
    +        'content' => [
    +          '#type' => 'html_tag',
    +          '#tag' => 'div',
    +          '#attributes' => [
    +            'class' => ['media-library-content'],
    +          ],
    +          'view' => $this->getMediaLibraryView($configuration->getSelectedType()),
    +        ],
    +      ],
    +    ];
    +  }
    

    This is really clean and elegant. I like it a lot!

  16. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,151 @@
    +    if (count($allowed_types) <= 1) {
    +      return [];
    +    }
    

    Why would it be less than 1? Let's change that to ===.

  17. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,151 @@
    +    $query = [
    +      'media_library_widget_id' => $configuration->getWidgetId(),
    +      'media_library_allowed_types' => array_keys($allowed_types),
    +      'media_library_remaining' => $configuration->getAvailableSlots(),
    +    ];
    

    No need for this. The state object extends ParameterBag, so this can just be $query = $configuration->all();

  18. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,151 @@
    +    $view = Views::getView('media_library');
    

    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.

  19. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,151 @@
    +    $display_id = 'widget';
    

    This, and the view ID, could and should be semi-configurable. How about a default route parameter?

  20. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +99,94 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    $configured_media_type_ids = array_keys($this->getFieldSetting('handler_settings')['target_bundles']);
    

    target_bundles can be NULL, signifying that all bundles are allowed. We need to handle this case, and test for it.

  21. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +99,94 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    uasort($media_types_setting, 'Drupal\Component\Utility\SortArray::sortByWeightElement');
    +    $sorted_media_type_ids = array_keys($media_types_setting);
    +    // Add new media types.
    +    $sorted_media_type_ids = array_merge($sorted_media_type_ids, array_diff($configured_media_type_ids, $sorted_media_type_ids));
    +    // Remove deletes types.
    +    return array_intersect($sorted_media_type_ids, $configured_media_type_ids);
    

    This is confusing. Can there be more detailed comments explaining the intention here?

  22. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +99,94 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    $summary[] = t('Media type order: @order', ['@order' => implode(', ', $media_type_labels)]);
    

    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.

  23. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -232,17 +321,15 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      'media_library_selected_type' => reset($primary_media_type_id),
    +      'media_library_allowed_types' => $media_type_ids,
    

    We should keep this stuff encapsulated to the state object.

Wim Leers’s picture

Exciting! This definitely needs screenshots though. That was the first thing I was looking for :)

seanB’s picture

seanB’s picture

seanB’s picture

FileSize
72.13 KB
49.61 KB

Added screenshots to the IS and also looked at #4:

  1. @TODO, You're right, we at least need the argument.
  2. Fixed. The sequance contains the media types as key, but each media type is another array containing the weight key with the weight. The tabledrag needs the extra wrapper element. I tried to fix that but ran into #784874: Add ability to insert FAPI wrapper element without modifying #parents of children. Added a process method for now to store the value how we want it.
  3. Looked at that. While it visually looks similar, using the vertical tabs render element outside of a form is not possible. Our HTML structure is quite different. It is not as easy as slapping on the same CSS classes I'm afraid.
  4. Fixed. I added some methods and documentation to the object. I couldn't find any examples of JS classes and multiple examples of JS objects (offCanvas, CKEditor), so leaving this an object seems to be more in line with the rest of core.
  5. Fixed
  6. I agree it would be nice, refactored it a bit. The selection stuff is still pretty specific for the media library modal.
  7. Fixed, refactored the JS.
  8. Fixed, also added some comments.
  9. Several reasons. We need the render array in the AJAX command when new content is added as well if we want to reopen the media library after ing new media items. I've seen several requests on how extensible the library is going to be. I believe it should be easy to change the menu to use taxonomy instead of media types or something. All this can also be done making this a regular controller though. Just think that getting the build array from a service is easier than faking a request and using the controller resolver.
  10. Fixed
  11. Fixed
  12. Fixed
  13. Fixed, moved it to the service.
  14. Fixed
  15. Yay!
  16. Fixed
  17. Fixed. I know there was a reason why I did this. I think when submitting the add form, form specific URL parameters are added. When you try to render the media library from the form submit handler again you don't want the other parameters. I'll change it for now since we don't have the add form on the same page yet, but this is something we should probably keep an eye on.
  18. Fixed
  19. We have #2971209: Allow the MediaLibraryUiBuilder service to use an alternative view display, I suggest we fix this in the other issue.
  20. We have #2989503: Add tests to prove that the media library widget works when target_bundles is NULL, I suggest we fix this in the other issue.
  21. Fixed
  22. Fixed
  23. Fixed. Added a new createUrl method to the state. Not sure yet what to do with the widget ID? The widget ID is currently used to determine the way the selection is handled. We could use the more generic parameters 'field_id' and 'field_type' for this? The field type would allow us to also add ckeditor integration later. Implemented this for now.

Still todo:

  • Update hook incl tests
  • Media library access tests

The last submitted patch, 6: 3020716-6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 8: 3020716-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
FileSize
279.01 KB
216.29 KB

Added the following:

  • Update hook for #4.1 (incl test)
  • Access tests to test the changes for #4.18
  • Fix for the test fail in #8

Status: Needs review » Needs work

The last submitted patch, 11: 3020716-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

Very cursory partial review...

  1. +++ b/core/modules/media_library/config/schema/media_library.schema.yml
    @@ -0,0 +1,10 @@
    +      sequence:
    +        type: integer
    +        label: 'Weight'
    

    I think the sequence is actually a string sequence, since each value in the sequence is a media type ID.

  2. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,63 @@
    +.media-library-menu {
    

    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.

  3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,58 @@
    +  Drupal.mediaLibrarySelection = {
    

    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.

  4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,58 @@
    +     */
    +    add(id) {
    

    Nit: These methods should all be preceded by a blank line.

  5. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,159 @@
    +  /**
    +   * Returns the ID of the field that opened the modal.
    +   *
    +   * @return string
    +   *   The field ID.
    +   */
    +  public function getFieldId() {
    +    return $this->get('media_library_field_id');
    +  }
    +
    +  /**
    +   * Returns the field type of the field that opened the modal.
    +   *
    +   * @return string
    +   *   The field type.
    +   */
    +  public function getFieldType() {
    +    return $this->get('media_library_field_type');
    +  }
    

    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.

seanB’s picture

Status: Needs work » Needs review
FileSize
278.8 KB
10.08 KB

1. 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() and getFieldType() by getOpenerId(). We can use this generic ID for fields as well as the WYSIWYG integration at a later stage (or any ID really).

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media_library/config/schema/media_library.schema.yml
    @@ -0,0 +1,10 @@
    +      label: 'Media types tab order'
    

    Can this a bit more descriptive -- "Allowed media types, in display order", perhaps?

  2. +++ b/core/modules/media_library/config/schema/media_library.schema.yml
    @@ -0,0 +1,10 @@
    +      sequence:
    +        type: string
    +        label: 'Weight'
    

    The label should probably be "Media type ID" or something like that.

  3. +++ b/core/modules/media_library/media_library.services.yml
    @@ -0,0 +1,4 @@
    +  media_library.modal:
    +    class: Drupal\media_library\MediaLibraryModal
    

    I think we should probably rename both the service and the class it wraps. How about something like MediaLibraryUiBuilder (media_library.ui_builder)?

  4. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -314,24 +315,31 @@ public function selectType(array &$form, FormStateInterface $form_state) {
    +    if (!$opener_id) {
    +      throw new BadRequestHttpException('The media library opener ID is required and must be a string.');
    

    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.

  5. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -314,24 +315,31 @@ public function selectType(array &$form, FormStateInterface $form_state) {
    +    if (strpos($opener_id, 'field:') === 0) {
    

    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)).

  6. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -480,13 +488,16 @@ public function access(array $allowed_types = NULL) {
    +        $types = $state->getAllowedTypes();
    

    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.

  7. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,182 @@
    +  /**
    +   * Check access to the media library.
    +   *
    +   * @param \Drupal\Core\Session\AccountInterface $account
    +   *   Run access checks for this account.
    +   *
    +   * @return \Drupal\Core\Access\AccessResult
    +   *   The access result.
    +   */
    +  public function access(AccountInterface $account) {
    

    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.

  8. +++ b/core/modules/media_library/src/MediaLibraryModal.php
    @@ -0,0 +1,182 @@
    +    $dialog_options = Json::encode(static::dialogOptions());
    

    Let's move this downwards so that we're only calling it when we actually need it.

  9. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,143 @@
    +/**
    + * A value object for the media library state.
    + *
    + * @internal
    + */
    +class MediaLibraryState extends ParameterBag {
    

    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.

  10. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,143 @@
    +  /**
    +   * Returns the number of additional media items that can be selected.
    +   *
    +   * @return int
    +   *   The number of additional media items that can be selected.
    +   */
    +  public function getAvailableSlots() {
    +    return $this->getInt('media_library_remaining');
    +  }
    

    What will this return if the cardinality is unlimited? We need to at least document that in the method comment.

  11. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,143 @@
    +  public static function createUrl($opener_id, $selected_type_id, array $allowed_media_type_ids, $remaining) {
    +    $query = [
    +      'media_library_opener_id' => $opener_id,
    +      'media_library_selected_type' => $selected_type_id,
    +      'media_library_allowed_types' => $allowed_media_type_ids,
    +      'media_library_remaining' => $remaining,
    +    ];
    +    return Url::fromRoute('media_library.modal', [], [
    +      'query' => $query,
    +    ]);
    +  }
    

    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.

  12. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +101,135 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    $configured_media_type_ids = array_keys($this->getFieldSetting('handler_settings')['target_bundles']);
    

    'target_bundles' can be null, signifying that all bundles are accepted. We need to handle this case, or array_keys() may generate errors here.

seanB’s picture

Status: Needs work » Needs review
FileSize
108.29 KB
206.87 KB

1. 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.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,62 @@
    +  Drupal.mediaLibrarySelection = {
    +    selection: [],
    +
    +    /**
    +     * Add a media item ID to the selection.
    +     *
    +     * @param {number} id
    +     *   The ID of the item we want to add.
    +     */
    +    add(id) {
    +      const position = this.selection.indexOf(id);
    +      if (position === -1) {
    +        this.selection.push(id);
    +      }
    +    },
    +
    +    /**
    +     * Remove a media item ID from the selection.
    +     *
    +     * @param {number} id
    +     *   The ID of the item we want to remove.
    +     */
    +    remove(id) {
    +      const position = this.selection.indexOf(id);
    +      if (position !== -1) {
    +        this.selection.splice(position, 1);
    +      }
    +    },
    +
    +    /**
    +     * Get the selected media items IDs.
    +     *
    +     * @return {Array}
    +     *   An array of selected media item IDs.
    +     */
    +    get() {
    +      return this.selection;
    +    },
    +
    +    /**
    +     * Reset the selected media items IDs.
    +     */
    +    reset() {
    +      this.selection = [];
    +    },
    +  };
    

    After 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.

  2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +    const count = Drupal.mediaLibrarySelection.get().length;
    +    const $toggleElements = $buttonPane.find(
    +      '.media-library-select, .media-library-selected-count',
    +    );
    

    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?

  3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +      const $wrapper = $buttonPane.find('.media-library-selected-count');
    

    This could probably be $toggleElements.filter('.media-library-selected-count').

  4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +        $wrapper.replaceWith(Drupal.theme('mediaLibrarySelectionCount', count));
    

    Can we call Drupal.theme() once, at the top of this else block?

  5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +        const $form = $(e.currentTarget).parents('.media-library-views-form');
    

    Should this be .closest()?

  6. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +        if ($(e.currentTarget).is(':checked')) {
    

    Not sure we need to use jQuery here -- won't e.currentTarget.checked work for this?

  7. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +          .val(Drupal.mediaLibrarySelection.get().join());
    

    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...]

  8. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +        // Once the selection is update, update the button pane.
    

    Should be "Once the selection is updated..."

  9. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +        updateButtonPane();
    

    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?

  10. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +        // Prevent users from selecting more items than allowed in the view.
    +        if (
    +          Drupal.mediaLibrarySelection.get().length ===
    +          settings.media_library.selection_remaining
    +        ) {
    +          disableItems($mediaItems.not(':checked'));
    +          enableItems($mediaItems.filter(':checked'));
    +        } else {
    +          enableItems($mediaItems);
    +        }
    

    Perhaps this logic should also probably be part of the hidden input field's change event handler.

  11. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +136,152 @@
    +  Drupal.behaviors.MediaLibraryModalApplySelection = {
    

    Can we change the word "apply" to "restore", and explicitly mention that we're loading the selected items from the hidden input field?

  12. +++ b/core/modules/media_library/media_library.module
    @@ -81,11 +80,8 @@ function media_library_views_post_render(ViewExecutable $view, &$output, CachePl
    +      $state = MediaLibraryState::fromRequest();
    +      $query = $state->all();
    

    This can be collapsed into one line: $query = MediaLibraryState::fromRequest()->all();

  13. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -314,24 +315,26 @@ public function selectType(array &$form, FormStateInterface $form_state) {
    -   *   If the "media_library_widget_id" query parameter is not present.
    +   *   If the "media_library_state_id" query parameter is not present.
    

    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.

  14. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -314,24 +315,26 @@ public function selectType(array &$form, FormStateInterface $form_state) {
    +    $opener_id = MediaLibraryState::fromRequest()->getOpenerId();
    

    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())

  15. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,136 @@
    + * When the media library is opened it needs several parameters to work
    + * properly. The parameters are retrieved from the MediaLibraryState value
    + * object. Since the parameters are passed via the URL, the value object is
    + * extended from ParameterBag. This also allows an opener to add extra
    + * parameters if needed. The following parameters are needed to open the media
    + * library:
    

    Great freaking documentation! This is exactly what I wanted :)

  16. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,136 @@
    + * - Opener ID: The opener ID is used to describe the "thing" that opened the
    + *   media library. Most of the time this is going to be a form field.
    + * - Allowed types: The media types available in the library can be restricted
    + *   to a list of allowed types.
    + * - Selected type: The media library contains tabs to navigate between the
    + *   different media types. The selected type contains the ID of the tab that
    + *   should be opened.
    + * - Remaining slots: When the opener want to limit the amount of media items
    + *   that can be selected, it can pass the number of remaining slots that can be
    + *   selected.
    

    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.

  17. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,136 @@
    +class MediaLibraryState extends ParameterBag {
    

    Let's make it impossible for outside code to construct this class directly. We can do that with a protected constructor:

    protected function __construct($parameters) {
      parent::__construct($parameters);
    }
    
  18. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,136 @@
    +   * Returns the ID of the opener of the modal.
    

    s/modal/media library. We shouldn't assume that the media library has been opened in a modal.

  19. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,136 @@
    +  public static function create($opener_id, array $allowed_media_type_ids, $selected_type_id, $remaining) {
    +    return new static([
    +      'media_library_opener_id' => $opener_id,
    +      'media_library_selected_type' => $selected_type_id,
    +      'media_library_allowed_types' => $allowed_media_type_ids,
    +      'media_library_remaining' => $remaining,
    +    ]);
    +  }
    

    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.

  20. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,136 @@
    +    $state = new static($request->query->all());
    +
    +    // Make sure the opener ID is set in the request to handle the selection.
    +    $opener_id = $state->getOpenerId();
    +    if (empty($opener_id) || !is_string($opener_id)) {
    +      throw new \InvalidArgumentException('The media library opener ID is required and must be a string.');
    +    }
    +
    +    return $state;
    

    To ensure that the aforementioned validation runs, let's instead do something like this:

    $query = $request->query;
    
    return static::create(
      $query->get('media_library_opener_id'),
      $query->get('media_library_allowed_types'),
      ...
    );
    

    Then, we can call $this->set() to restore any additional parameters that were passed in.

  21. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +/**
    + * Service which renders the media library modal.
    + */
    +class MediaLibraryUiBuilder implements AccessibleInterface {
    

    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.

  22. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +  public function access($operation = 'view', AccountInterface $account = NULL, $return_as_object = TRUE) {
    

    I could be wrong, but doesn't $return_as_object default to FALSE in AccessibleInterface?

  23. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +      $result = AccessResult::forbidden('The media library view does not exist.')
    +        ->addCacheableDependency($view->storage);
    

    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. :)

  24. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +    elseif (!$view->storage->getDisplay('widget')) {
    +      $result = AccessResult::forbidden('The media library widget display does not exist.')
    +        ->addCacheableDependency($view->storage);
    

    Let's also be sure this code path has tests.

  25. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +      $result = AccessResult::allowedIfHasPermission($account, 'view media')
    +        ->addCacheableDependency($view->storage);
    

    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.

  26. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +    $selected_type = $media_type_storage->load($state->getSelectedTypeId());
    

    Let's move this below the count check, so that we're not loading entities we might not need.

  27. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +    $allowed_types = $media_type_storage->loadMultiple($state->getAllowedTypeIds());
    

    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?

  28. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,177 @@
    +      if ($selected_type && $selected_type->id() === $allowed_type_id) {
    +        $menu['#links'][$allowed_type_id]['attributes']['class'][] = 'active';
    +      }
    

    What if no type is selected? We should probably mark the first link in the menu as the active one.

  29. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -77,15 +79,20 @@ public function __construct($plugin_id, $plugin_definition, FieldDefinitionInter
    +    $target_bundles = $settings['target_bundles'] ?? [];
    

    The ?? operator is PHP 7 only, so I'm not sure we can use it unless a framework or release manager says so.

  30. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +105,143 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +  protected function getEnabledMediaTypeIdsSorted() {
    

    I think we should change "enabled" to "allowed".

  31. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +105,143 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    $configured_media_type_ids = array_keys($this->getFieldSetting('handler_settings')['target_bundles']);
    

    We still need to account for the possibility that target_bundles will be NULL, which will cause this code to error.

  32. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +105,143 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +      $configured_media_type_ids = array_keys($this->entityTypeManager->getStorage('media_type')->loadMultiple());
    

    Pro-tip: if we're just getting IDs, it's nicer to do getStorage('media_type')->getQuery()->execute().

  33. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +105,143 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    // There could have been added or removed media types in the field storage.
    +    // We need to make sure new media types are added to the list and remove
    +    // media types that are no longer available for the field.
    +    $new_media_type_ids = array_diff($configured_media_type_ids, $media_type_ids);
    +    // Add new media type IDs to the list.
    +    $media_type_ids = array_merge($media_type_ids, $new_media_type_ids);
    +    // Remove media types that are no longer available.
    +    $media_type_ids = array_intersect($media_type_ids, $configured_media_type_ids);
    

    The comments here are useful, thank you!

  34. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +105,143 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    if (count($media_type_ids) !== 1) {
    

    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.

  35. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +105,143 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +   * The tabledrag functionality needs a specific weight field, but we don't
    +   * this extra weight field in our settings.
    

    Should be "...we don't want to store this extra weight field..."

  36. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -98,6 +105,143 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +  public static function setMediaTypesValue(&$element, $input, FormStateInterface $form_state) {
    

    &$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.

  37. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -64,6 +60,17 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +    // and populated via javascript.
    

    Nit: JavaScript

  38. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -64,6 +60,17 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +        // This is used to identify the hidden field in the form via javascript.
    

    Same.

andrewmacpherson’s picture

Issue summary: View changes
Issue tags: +Accessibility, +Needs accessibility review

Please 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.

  1. This doesn't actually use the existing vertical-tabs.js does it - it just looks similar? So we don't have all the features of the existing API, like the tab summaries.
  2. There are some strange behaviours going with the dialog. AFAICT, the entire dialog is replaced?
    1. When you choose a tab, it replaces the entire dialog, and puts focus on the start of the dialog, and screen readers announce the fact that focus has entered a dialog. "Hmmm, I thought I was already in a dialog... is this the same dialog, or a new dialog on top of the previous dialog? It has the same name as the previous dialog. Is the old dialog still around?"
    2. It's not clear what has changed, or why we have a new dialog. I suppose the content of each tab will be different, the latest patch just doesn't do that yet. But we'll have to convey to assistive tech what has changed on screen. I'm not sure about the best way to do that just now. It could be easier if the dialog content was changed, instead of replacing the entire dialog.
  3. Only show the 'Select media' button when there is actually something selected.

    1. The appearing-disappearing-reappearing button is strange. It runs counter the spirit of WCAG's consistent navigation principles, I think. If we keep this, we'll have to tell users that the available controls have changed.
    2. I'd rather keep the button present all the time though. If a user clicks it while there are no selections, that's harmless. The dialog can be dismissed, and the field content is unchanged.
    3. We could mark the button with the disabled attribute, but that has some problems of it's own (see Disabled buttons suck).
  4. Show the number of selected items in the button pane.

    1. When this count changes, it would be good to inform all users. Replacing the text inside a <div aria-live="polite"> should suffice.
    2. We aren't showing a message when no items are selected, because the message disappears when the button does. It would be valuable to show this all the time, with an explicit "0 items selected" message. This will work in tandem with the aria-live behaviour, so screen reader users are informed when they no longer have any selections. If the message is removed when reaching zero, screen reader users won't be told.
    3. Can we convey which tabs contain selected items, say by adding a summary message to each tab header? I'm thinking about the situation where a user has some selections in more than one tab, and has reached the maximum count for the media field cardinality. If the user needs to adjust their selections, they have to go digging through each tab to find out where the selections are. Indicating which tabs have selected items would be a useful hint to reduce the amount of exploration needed. In particular, it could be a big help for users with working memory impairments.
  5. Make sure media items are disabled for each tab when the maximum number of items for the field is selected.

    1. Why is this necessary - who or how does it help?
    2. What does this entail - does it mean adding the disabled attribute to checkboxes? That will remove them from the reported form elements in some screen readers that have a "forms mode" on Windows. To some users, it may seem like some content has disappeared!
    3. How does a user know that the maximum has been reached? The screenshot called "After: Modal with max. item selected" doesn't convery that the maximum has been reached, it just says 5 selected. There is a message saying "5 items remaining" but that's in the underlying page, and will be obscured/shaded when the dialog is open. It will be confusing if checkboxes suddenly stop working for no apparent reason.
    4. Instead of disabling checkboxes, we could report when users have selected too many. The count message could convey this, and an error message could be returned when users press the submit button with too many selections.
  6. 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.

    1. I don't really follow this - what does this involve for editors, and why does it matter?
    2. What constitutes "most important media type" - is that a site builder's content modelling decision, when configuring a field?
    3. How does sorting lead to automatically selecting a tab?
    4. Is this about site builders choosing the order of tabs on a per-field basis? If so, I think that runs counter to the WCAG idea of consistent navigation. On sites which have several media fields, site builders could create an inconsistent experience between these, and forces users to study the tab order each time, instead of benefiting from a consistent order. It's creating unnecessary cognitive load, which can particulary impact users with dyslexia ("don't make me read"), dyspraxia (consistent spatial arrangements can help), or other miscellaneous learning/memory issues ("which order is it again?").
    5. Update: I've seen the widget settings screenshot now - "After: Widget settings to sort vertical tabs style menu in modal (new feature)". That makes some of this clearer, but I don't understand how it controls the selected tab. Is this always going to be the first tab, and re-ordering the types is really a proxy for controlling which type gets the privileged first-tab position?

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.

seanB’s picture

Status: Needs work » Needs review
FileSize
120.58 KB
49.06 KB

Thanks @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 to checkAccess 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() and setSettings() are not used on the widget plugin. The form saves the values on the EntityFormDisplay 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.

seanB’s picture

Just 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?

seanB’s picture

I 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?

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,19 @@
    +   * When a user interacts with the media library we want the selection to
    +   * persist as long as the media library modal is opened. We temporarily store
    +   * the selected items while the user filters the media library view or
    +   * navigates to different views pages and tabs.
    

    Nice docs!

  2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,19 @@
    +  Drupal.mediaLibrary = {
    

    Can this be Drupal.MediaLibrary (to keep it in line with similar client-side systems like Drupal.Ajax)?

  3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,230 @@
    +   * Load media library content through AJAX.
    +   */
    

    We should probably explain why this is necessary.

  4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,230 @@
    +          const ajaxObject = Drupal.ajax({
    +            wrapper: 'media-library-content',
    +            url: e.currentTarget.href,
    +            dialogType: 'ajax',
    +            progress: {
    +              type: 'fullscreen',
    +              message: Drupal.t('Please wait...'),
    +            },
    +          });
    +
    +          // Override the ajax success callback to shift focus to the media
    +          // library content.
    +          ajaxObject.success = function(response, status) {
    

    Nit: Is it possible for us to pass the success callback to Drupal.ajax(), rather than setting it after the fact?

  5. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,185 @@
    +  protected function __construct(array $parameters = array()) {
    +    parent::__construct($parameters);
    +  }
    

    We don't use array() syntax any more :)

  6. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,185 @@
    +    $media_type_storage = \Drupal::entityTypeManager()->getStorage('media_type');
    +    if (array_diff($allowed_media_type_ids, array_keys($media_type_storage->loadMultiple($allowed_media_type_ids)))) {
    

    We should probably do getQuery()->condition('id', $allowed_media_type_ids)->execute() here, rather than load all media types.

  7. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,185 @@
    +    // Create a MediaLibraryState object through the create method to make sure
    +    // all validation runs.
    

    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.

  8. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,191 @@
    +  /**
    +   * Build the media library.
    +   *
    +   * @return array
    +   *   The render array for the media library.
    +   */
    +  public function buildLibrary() {
    +    $state = MediaLibraryState::fromRequest();
    +    return [
    +      '#type' => 'html_tag',
    +      '#tag' => 'div',
    +      '#attributes' => [
    +        'id' => 'media-library-wrapper',
    +      ],
    +      'menu' => $this->buildMediaTypeMenu($state),
    +      'content' => $this->buildContent(),
    +    ];
    +  }
    

    I'm not sure about the name of this method. How about buildWrapper() or buildMainInterface() or buildUi()?

  9. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,191 @@
    +  /**
    +   * Build the media library content area.
    +   *
    +   * @return array
    +   *   The render array for the media library.
    +   */
    +  public function buildContent() {
    +    $state = MediaLibraryState::fromRequest();
    +    return [
    +      '#type' => 'html_tag',
    +      '#tag' => 'div',
    +      '#attributes' => [
    +        'id' => 'media-library-content',
    +        'tabindex' => -1,
    +      ],
    +      'view' => $this->buildMediaLibraryView($state),
    +    ];
    +  }
    

    Same sort of thing here. How about something like buildLibraryContent() or buildLibraryViewTab() or similar?

  10. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,191 @@
    +      return AccessResult::forbidden('The media library view does not exist.');
    

    Let's explicitly set the maximum cache age to 0. I can't imagine we would ever want to cache this result.

  11. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,191 @@
    +    // Get the state parameters but remove the ajax wrapper format.
    +    $state->remove('_wrapper_format');
    

    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();

  12. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,191 @@
    +    $allowed_types = $this->entityTypeManager->getStorage('media_type')->loadMultiple($allowed_type_ids);
    

    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.

  13. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,191 @@
    +  protected function buildMediaLibraryView(MediaLibraryState $state) {
    

    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.

seanB’s picture

Status: Needs work » Needs review
FileSize
127.57 KB
15.19 KB

Just 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.

phenaproxima’s picture

@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:

  • The "X of Y selected" text at the bottom is potentially confusing and may need to be re-worded. It's very easy to change that, though, so this does not to be a blocker for this issue. In other words, if it needs to spend a significant amount of time in the ol' bikeshed, we can and should do it in a follow-up.
  • There were requests to add something that will allow users to see which tabs have selected items, and which selections they currently have. Due to the complexity of integrating with Views, we can't just add simple counter badges to each of the vertical tabs; instead, we have been leaning towards implementing a link that will display the current selection. We already have a separate issue for this, though, and will handle this problem there in order to keep this patch as simple as it can be. That issue is #3023809: Add a selection overview to the media library widget modal.

So, I think we're pretty much full speed ahead on this one. Let's land it.

phenaproxima’s picture

Status: Needs review » Needs work

Partial review:

  1. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,19 @@
    +   * Store the media library selection.
    

    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".

  2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,19 @@
    +   * When a user interacts with the media library we want the selection to
    

    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.

  3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -1,7 +1,19 @@
    +   * navigates to different views pages and tabs.
    

    Can we remove "views pages"?

  4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +   * Standard AJAX links (using the 'use-ajax' class) replace the entire library
    +   * dialog. When navigating to a media type in the modal through the vertical
    +   * tabs, we only want to load the changed library content. This is not only
    +   * more efficient, but also provides a more accessible user experience for
    +   * screen readers.
    

    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.)

  5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +        .on('click', e => {
    

    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').

  6. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +          // Override the ajax success callback to shift focus to the media
    

    Nit: AJAX should always be all-caps.

  7. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +            // Execute the ajax commands.
    

    Same here.

  8. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +  function updateButtonPane(remaining) {
    

    I think this function should also be moved into Drupal.MediaLibrary as Drupal.MediaLibrary.updateSelectionInfo() or something.

  9. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +    const selected = Drupal.MediaLibrary.currentSelection.length;
    +    const latestCount = Drupal.theme('mediaLibrarySelectionCount', selected, remaining);
    

    Idea: Let's pass the entire currentSelection into Drupal.theme('mediaLibrarySelectionCount'), for greater theming flexibility.

  10. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +      if (!$mediaItems.length) {
    +        return;
    +      }
    

    We don't need this. If there are no items, the subsequent stuff will basically just be a no-op.

  11. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +          if (Drupal.MediaLibrary.currentSelection.indexOf(id) === -1) {
    +            Drupal.MediaLibrary.currentSelection.push(id);
    +          }
    

    JavaScript's Array object has a .includes() method which does this this, but it's not supported in IE :( Sadness!

  12. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +          const position = Drupal.MediaLibrary.currentSelection.indexOf(id);
    

    We should move this above the if check, since we use the value in both logic branches.

  13. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +          .val(Drupal.MediaLibrary.currentSelection.join())
    

    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?

  14. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +      function disableItems($items) {
    +        $items
    +          .prop('disabled', true)
    +          .closest('.js-media-library-item')
    +          .addClass('media-library-item--disabled');
    +      }
    +
    +      function enableItems($items) {
    +        $items
    +          .prop('disabled', false)
    +          .closest('.js-media-library-item')
    +          .removeClass('media-library-item--disabled');
    +      }
    

    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.

  15. +++ b/core/modules/media_library/js/media_library.widget.es6.js
    @@ -81,40 +93,236 @@
    +          // Prevent users from selecting more items than allowed in the view.
    

    Can we remove "in the view"? It's a bit confusing.

  16. +++ b/core/modules/media_library/media_library.install
    @@ -47,3 +47,159 @@ function media_library_uninstall() {
    +    $view->save(TRUE);
    

    Do we need the TRUE here? Why not let the config system validate this stuff?

  17. +++ b/core/modules/media_library/media_library.routing.yml
    @@ -4,3 +4,15 @@ media_library.upload:
    +media_library.modal:
    +  path: '/media-library'
    +  defaults:
    +    _controller: 'media_library.ui_builder:buildUi'
    +  requirements:
    +    _custom_access: 'media_library.ui_builder:checkAccess'
    +media_library.modal_content:
    +  path: '/media-library/content'
    +  defaults:
    +    _controller: 'media_library.ui_builder:buildLibraryContent'
    +  requirements:
    +    _custom_access: 'media_library.ui_builder:checkAccess'
    

    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.

  18. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -312,26 +313,25 @@ public function selectType(array &$form, FormStateInterface $form_state) {
    +    if (preg_match('/field:([a-z0-9_-]+)$/', $opener_id, $matches)) {
    

    The regex should probably begin with ^.

phenaproxima’s picture

Another partial review, picking up from where #25 left off:

  1. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -484,9 +484,11 @@ protected function getTypes(array $allowed_types = NULL) {
    +        $types = $media_type_storage->loadMultiple(MediaLibraryState::fromRequest()->getAllowedTypeIds());
    

    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?

  2. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    + *   opened the media library. Most of the time this is going to be a form field.
    

    Supernit: This line is 81 characters long. :)

  3. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    + *   containing media type IDs.
    

    I think it would be clearer if we change "containing" to "of".

  4. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    + *   media type of which the tab that should be opened.
    

    "of which the" should be "whose".

  5. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    + * - media_library_remaining: When the opener want to limit the amount of media
    

    want --> wants

  6. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    + *   items that can be selected, it can pass the number of remaining slots.
    

    We should document that passing a negative number here will allow unlimited items to be selected.

  7. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +  use StringTranslationTrait;
    

    What are we using StringTranslationTrait for?

  8. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +  protected function __construct(array $parameters = []) {
    +    parent::__construct($parameters);
    +  }
    

    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.

  9. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +   * Create a new MediaLibraryState object.
    

    "Creates"

  10. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +   * @throws \InvalidArgumentException
    +   *   If one of the passed arguments is missing or does not pass the
    +   *   validation.
    +   */
    +  public static function create($opener_id, array $allowed_media_type_ids, $selected_type_id, $remaining_slots) {
    

    I don't think the @throws is supposed to go here, since this method never actually throws anything.

  11. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +   * @return bool
    +   *   Whether the required parameters are valid or not.
    +   *
    +   * @throws \InvalidArgumentException
    +   *   If one of the passed arguments is missing or does not pass the
    +   *   validation.
    +   */
    +  protected static function validateParameters($opener_id, array $allowed_media_type_ids, $selected_type_id, $remaining_slots) {
    

    There is no need for this method to return anything. If it doesn't throw an exception, validation succeeded.

  12. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +    // The opener ID must be a non-empty string.
    +    if (empty($opener_id) || !is_string($opener_id)) {
    +      throw new \InvalidArgumentException('The opener ID parameter is required and must be a string.');
    +    }
    

    This will not catch an opener ID like " ". We should probably change this to a regex.

  13. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +      if (empty($allowed_media_type_id) || !is_string($allowed_media_type_id)) {
    +        throw new \InvalidArgumentException('The allowed types parameter is required and must be an array of strings.');
    +      }
    

    Same here.

  14. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,211 @@
    +    // The selected type ID must be a non-empty string.
    +    if (empty($selected_type_id) || !is_string($selected_type_id)) {
    +      throw new \InvalidArgumentException('The selected type parameter is required and must be a string.');
    +    }
    

    And here.

  15. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,193 @@
    +          '#markup' => $title . ' <span class="active-tab visually-hidden">' . t('(active tab)') . '</span>',
    

    This should be using $this->t().

  16. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +85,147 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +      ] + parent::defaultSettings();
    

    Nit: I think this probably needs to be outdented a bit.

  17. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +85,147 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +   * @return \Drupal\media\MediaTypeInterface[]
    +   *   The media types sorted by weight.
    +   */
    +  protected function getAllowedMediaTypeIdsSorted() {
    

    Does this return media types, or media type IDs? The doc comment does not match the method name.

  18. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +85,147 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    // Get the sorted list from the widget settings.
    +    $media_type_ids = $this->getSetting('media_types');
    

    Let's explicitly mention that this list has been pre-sorted by the user via the settings form.

  19. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +85,147 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    // There could have been added or removed media types in the field. We need
    

    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."

  20. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +85,147 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    // that are no longer available for the field.
    

    Should be "...that no longer exist."

  21. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +85,147 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    if (count($media_type_ids) > 1) {
    

    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.

  22. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +85,147 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +      $delta = 0;
    

    Can we name this variable $weight? That would more clearly reflect how we're using it.

  23. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -217,26 +360,25 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +    $primary_media_type_id = array_slice($allowed_media_type_ids, 0, 1, TRUE);
    +    $selected_type_id = reset($primary_media_type_id);
    

    Can't we just use reset($allowed_media_type_id) to get the selected type?

  24. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -95,17 +105,23 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +    if (preg_match('/field:([a-z0-9_-]+)$/', $opener_id, $matches)) {
    

    This regex should also probably start with a ^. We use it elsewhere too, so it might be worthwhile to make it a constant somewhere?

seanB’s picture

New 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 from media_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 ?: [] from getAllowedTypeIds 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.

Status: Needs review » Needs work

The last submitted patch, 27: 3020716-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
FileSize
131.98 KB
5.79 KB

D'oh! Forgot to add the new media_library.ui.js file.

Status: Needs review » Needs work

The last submitted patch, 29: 3020716-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

phenaproxima’s picture

  1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,267 @@
    +      // checkbox values trigger the change event for the media items. The
    

    "trigger" --> "triggers"

  2. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -312,26 +314,25 @@ public function selectType(array &$form, FormStateInterface $form_state) {
    +    if (preg_match(MediaLibraryWidget::OPENER_ID_REGEX, $opener_id, $matches)) {
    

    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).

  3. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -484,9 +485,11 @@ protected function getTypes(array $allowed_types = NULL) {
    +        $types = $media_type_storage->loadMultiple(MediaLibraryState::fromRequest()->getAllowedTypeIds());
    

    If I'm not mistaken, FormBase has a getRequest() method we can use to pass the request to MediaLibraryState::fromRequest()...

  4. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,202 @@
    + * @internal
    + */
    +class MediaLibraryState extends ParameterBag {
    

    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."

  5. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,202 @@
    +    if (empty(trim($opener_id)) || !is_string($opener_id)) {
    

    In all these checks, let's move the is_string() call before empty(trim()), because otherwise we risk passing a non-string to trim().

  6. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,198 @@
    + * @internal
    + */
    +class MediaLibraryUiBuilder {
    

    Let's add the same @internal disclaimer here as we did for MediaLibraryState.

  7. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,198 @@
    +          '#markup' => $title . ' <span class="active-tab visually-hidden">' . $this->t('(active tab)') . '</span>',
    

    I'm not certain this will translate correctly for all languages. Shouldn't we do something like '@title (active tab)' to increase translatability?

  8. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +95,154 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    // Get the media type IDs sorted by the user in the settings form.
    +    $media_type_ids = $this->getSetting('media_types');
    +
    +    // Get the configured media types from the field storage.
    +    $configured_media_type_ids = $this->getFieldSetting('handler_settings')['target_bundles'] ?? [];
    

    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.

  9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -83,6 +95,154 @@ public static function isApplicable(FieldDefinitionInterface $field_definition)
    +    $media_types = $this->entityTypeManager->getStorage('media_type')->loadMultiple($this->getAllowedMediaTypeIdsSorted());
    +    if ($media_types !== 1) {
    

    I think this is meant to be count($media_types) !== 1

  10. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -64,6 +61,17 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +      '#attributes' => [
    +        // This is used to identify the hidden field in the form via JavaScript.
    +        'id' => ['media-library-modal-selection'],
    +      ],
    

    The id attribute should only be a string, never an array. :)

  11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -135,6 +138,40 @@ public function testAdministrationPage() {
    +    $library_query = ['query' => $state->all()];
    

    Can we rename this variable to $url_options, so as to be clearer what it really contains?

  12. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $assert_session->elementExists('css', '.media-library-menu');
    +    $assert_session->elementTextContains('css', '.media-library-menu', 'Type One');
    +    $assert_session->elementTextNotContains('css', '.media-library-menu', 'Type Two');
    +    $assert_session->elementTextContains('css', '.media-library-menu', 'Type Three');
    +    $assert_session->elementTextNotContains('css', '.media-library-menu', 'Type Four');
    

    I wonder if we could instead do something like:

    $menu = $assert_session->elementExists('css', '.media-library-menu');
    $assert_session->elementExists('named', ['link', 'Type One'], $menu);
    
  13. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    // Assert menu link reordering.
    

    What does this mean? Can the comment be expanded?

  14. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $links = $page->findAll('css', '.media-library-menu a');
    +    $link_titles = [];
    +    foreach ($links as $link) {
    +      $link_titles[] = $link->getText();
    +    }
    

    This looks like a good place for array_map(), to reduce the number of variables.

  15. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $expected_link_titles = ['Type One (active tab)', 'Type Two', 'Type Three', 'Type Four'];
    +    $this->assertSame($link_titles, $expected_link_titles);
    

    We don't need $expected_link_titles to be its own variable; let's just pass it as the second argument to assertSame().

  16. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $page->find('css', 'input[name="field_twin_media_settings_edit"]')->click();
    

    Should this maybe use $assert_session->buttonExists()->press()?

  17. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $page->find('css', '.tabledrag-toggle-weight')->click();
    

    I think $assert_session->elementExists('named', ['link', 'text of the link'])->click() might be more readable here.

  18. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $edit = [
    +      'fields[field_twin_media][settings_edit_form][settings][media_types][type_one][weight]' => 0,
    +      'fields[field_twin_media][settings_edit_form][settings][media_types][type_three][weight]' => 1,
    +      'fields[field_twin_media][settings_edit_form][settings][media_types][type_four][weight]' => 2,
    +      'fields[field_twin_media][settings_edit_form][settings][media_types][type_two][weight]' => 3,
    +    ];
    +    $this->drupalPostForm(NULL, $edit, t('Save'));
    

    Can we replace this with $assert_session->fieldExists()->setValue() and $assert_session->buttonExists('Save')->press() instead?

  19. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $twin_button = $assert_session->elementExists('css', '.media-library-open-button[href*="field_twin_media"]');
    +    $twin_button->click();
    

    We don't need $twin_button to be its own variable.

  20. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $unlimited_button = $assert_session->elementExists('css', '.media-library-open-button[href*="field_unlimited_media"]');
    +    $unlimited_button->click();
    

    Same here.

  21. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $page->find('css', '.media-library-menu .type-three > a')->click();
    

    It's more readable to find links by their text. $assert_session->elementExists('named', ['link', 'Type three'])->click()

  22. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -145,120 +182,307 @@ public function testWidget() {
    +    $assert_session->elementExists('css', '.media-library-selected-count');
    +    $assert_session->elementTextContains('css', '.media-library-selected-count', '0 of 2 items selected');
    

    We don't need to assert that the element exists; elementTextContains() will do it implicitly.

  23. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -0,0 +1,110 @@
    +  /**
    +   * Modules to install.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    

    This should be @inheritdoc, and protected.

  24. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
    @@ -0,0 +1,110 @@
    +    $view_original = clone Views::getView('media_library');
    

    Why does this need to be cloned?

  25. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryStateTest.php
    @@ -0,0 +1,292 @@
    +  /**
    +   * Modules to install.
    +   *
    +   * @var array
    +   */
    +  public static $modules = [
    

    Same here.

  26. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryStateTest.php
    @@ -0,0 +1,292 @@
    +  public function testCreate($opener_id, array $allowed_media_type_ids, $selected_type_id, $remaining_slots, $exception_class = NULL, $exception_message = '') {
    +    if ($exception_class) {
    +      $this->setExpectedException($exception_class, $exception_message);
    +      MediaLibraryState::create($opener_id, $allowed_media_type_ids, $selected_type_id, $remaining_slots);
    +    }
    +    else {
    +      $state = MediaLibraryState::create($opener_id, $allowed_media_type_ids, $selected_type_id, $remaining_slots);
    +      $this->assertInstanceOf(MediaLibraryState::class, $state);
    +    }
    +  }
    

    We can restructure this method a bit to make it less verbose:

    if ($exception_message) {
      $this->setExpectedException('InvalidArgumentExeception', $exception_message);
    }
    $state = MediaLibraryState::create();
    $this->assertInstanceOf(MediaLibraryState::class, $state);
    
seanB’s picture

Status: Needs work » Needs review
FileSize
134.42 KB
40.28 KB

New 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:

phenaproxima’s picture

  1. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -29,16 +29,11 @@
    -  /**
    -   * {@inheritdoc}
    -   */
    -  protected function __construct(array $parameters = []) {
    -    parent::__construct($parameters);
    -  }
    -
    

    Removing this is fine and good, but it means we need to validate the parameters via the constructor. :)

  2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -662,4 +659,21 @@ protected static function setFieldState(array $element, FormStateInterface $form
    +  /**
    +   * Get the field ID of the widget from an opener ID.
    +   *
    +   * @param string $opener_id
    +   *   The opener ID of the media library.
    +   *
    +   * @return string|null
    +   *   The field ID or NULL if the opener ID is not valid for the widget.
    +   */
    +  public static function getOpenerFieldId($opener_id) {
    

    Let's add a @see referencing MediaLibraryState, so that people can look up what an "opener ID" is if they need to.

seanB’s picture

Ok, new patch attached.
#33.1 Fixed.
#33.2 Fixed.

alexpott’s picture

@phenaproxima asked me to provided feedback on whether we should

  • Have two routes - one to return the full UI and one to return just the content
  • Or one route with some form of switch

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.

phenaproxima’s picture

Status: Needs review » Needs work

Thanks for settling the question, @alexpott. Kicking back to Needs work for that.

seanB’s picture

Status: Needs work » Needs review
FileSize
135.04 KB
6.05 KB

New 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.

phenaproxima’s picture

I 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:

  • Sign-off from a front-end framework manager, concerning our CSS changes;
  • Accessibility sign-off, although anything that's not a major problem should be deferred to a follow-up, because there are still a lot of other issues we need to tackle before alpha;
  • Possibly JavaScript review, although I'm aware that core is lacking JavaScript people-power at the moment;
  • Possibly release manager review, to be sure the changes are, y'know, release-able
lauriii’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
33.23 KB

I'm not 100% done with my review yet but here's what I could find so far.

  1. +++ b/core/modules/media_library/css/media_library.module.css
    @@ -2,6 +2,11 @@
    +#media-library-wrapper {
    
    +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,68 @@
    +#media-library-content {
    

    We shouldn't use ids in selectors for theming.

  2. +++ b/core/modules/media_library/css/media_library.module.css
    @@ -2,6 +2,11 @@
    +  margin: -1em;
    

    This doesn't work with default Drupal dialog styles.

  3. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,68 @@
    +  background-color: #e6e5e1;
    

    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?

  4. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,68 @@
    +.media-library-menu li {
    ...
    +.media-library-menu a {
    ...
    +.media-library-menu a:active,
    ...
    +.media-library-menu a:focus,
    ...
    +.media-library-menu a.active {
    

    Would it be possible to add classes to these elements? That would allow us to use a single class for targeting these elements.

  5. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,68 @@
    +  z-index: 2;
    ...
    +  z-index: 1;
    

    What is this property used for here?

  6. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,267 @@
    +/**
    

    When uploading next patch for this issue, please run yarn prettier command which will format this file according to our coding standards.

seanB’s picture

Status: Needs work » Needs review
FileSize
139.48 KB
9.08 KB

Thanks @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.

lauriii’s picture

Issue tags: +Needs reroll

Status: Needs review » Needs work

The last submitted patch, 40: 3020716-40.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
139.73 KB

Rerolled #40.

benjifisher’s picture

We 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.

  1. Add an indication to the vertical tab of how many items are selected in that tab. Looking at the "after" screenshots with items selected, I would add "(2)" or "(5)" (depending on which screenshot) to the "Images" tab. I am not sure what to add to the tabs with nothing selected: maybe nothing, maybe "()", maybe "(0)". I thnk I prefer "(0)".
  2. The before and after screenshots of the empty widget look the same. Aren't we removing the "Add media" button there?
  3. We do not want to clutter the interface, but maybe add a title attribute (hover text) to the notification "2 items selected". Something along the lines of "This field allows up to 5 items, and 2 are selected.
  4. Question: If the field already has 2 Media items attached, and I click the "Browse media" button to bring up the modal, are the two items selected? Is there any difference between that and starting from 0, then selecting the 2 items?
  5. During the Usability meeting, you mentioned that there is another issue proposing that the "X items selected" text should be a link to a list of the selected items. Where is that issue? I think it would be natural to have another vertical tab, "Selected", listing these items. If we do that, and adopt my earlier suggestion, then "Selected (5)" makes the notification "5 items selected" redundant.

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.

seanB’s picture

Thanks @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.

lauriii’s picture

  1. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,71 @@
    +.media-library-menu li {
    

    We 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.

  2. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,71 @@
    +.media-library-menu-link {
    ...
    +.media-library-menu-link:active,
    +.media-library-menu-link:hover,
    +.media-library-menu-link:focus {
    ...
    +.media-library-menu-link:focus,
    +.media-library-menu-link:active {
    ...
    +.media-library-menu-link.active {
    

    This should be named using BEM pattern. This class could be media-library-menu__link for example.

  3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,272 @@
    +   * Load media library content through AJAX.
    

    This documentation should be adjusted to comply with our JavaScript documentation standards.

  4. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,272 @@
    +                '@title<span class="active-tab visually-hidden"> (active tab)</span>',
    

    We probably should make this overridable by creating a theme function.

seanB’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
139.96 KB

#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.

phenaproxima’s picture

In retrospect, I don't know why I added the "needs release manager review" tag. Probably for completeness' sake. I'm removing it.

seanB’s picture

I 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.

lauriii’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,273 @@
    +   */
    +  Drupal.behaviors.MediaLibraryWidgetWarn = {
    +    attach(context) {
    ...
    +  Drupal.behaviors.MediaLibraryTabs = {
    ...
    +  Drupal.behaviors.MediaLibraryItemSelection = {
    ...
    +  Drupal.behaviors.MediaLibraryModalClearSelection = {
    

    We should document behaviors according to our JavaScript coding standards.

  2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,273 @@
    +      const $menu = $('.media-library-menu');
    ...
    +      const $form = $('.media-library-views-form', context);
    

    We should attach this using js- prefixed class.

  3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,273 @@
    +        // Set the selection  in the hidden form element.
    

    Nit: double space

  4. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,273 @@
    +          .find('input#media-library-modal-selection')
    ...
    +      $('input#media-library-modal-selection', $form)
    

    We shouldn't include the element type in the selector.

seanB’s picture

Status: Needs work » Needs review
FileSize
14.6 KB
146.14 KB

#50:
1. Fixed.
2. Fixed.
3. Fixed.
4. Fixed.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Accessibility, -Needs accessibility review +accessibility

This 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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/media_library/js/media_library.ui.js
@@ -0,0 +1,159 @@
+  };
+})(jQuery, Drupal, window);
\ No newline at end of file

+++ b/core/modules/media_library/js/media_library.widget.js
@@ -36,32 +36,4 @@
-  };
 })(jQuery, Drupal);
\ No newline at end of file
Gábor Hojtsy’s picture

Re: 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.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

@Gábor Hojtsy those are computer generated JS files. All of our pre-existing computer generated JS files have the same problem.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Ok. 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.

seanB’s picture

Status: Needs work » Needs review
FileSize
145.66 KB

Rerolled #51.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Everything is still green, so I guess we can go back to RTBC.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Upon attempting to commit there were a few yarn and one phpcs error. These seemed too numerous to fix on commit.

Checking changed files...
PHPCS: core/modules/media_library/config/install/views.view.media_library.yml passed
PHPCS: core/modules/media_library/config/schema/media_library.schema.yml passed
STYLELINT: core/modules/media_library/css/media_library.module.css passed
STYLELINT: core/modules/media_library/css/media_library.theme.css passed
yarn run v1.6.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.click_to_select.es6.js
[13:25:47] '/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.click_to_select.es6.js' is being checked.
✨  Done in 0.93s.
yarn run v1.6.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.ui.es6.js
[13:25:50] '/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.ui.es6.js' is being checked.
✨  Done in 0.71s.

/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.ui.es6.js
  202:31  error  Replace `'.media-library-widget-modal·.ui-dialog-buttonpane'` with `⏎··········'.media-library-widget-modal·.ui-dialog-buttonpane',⏎········`  prettier/prettier
  213:49  error  Replace `'.media-library-selected-count'` with `⏎··········'.media-library-selected-count',⏎········`                                          prettier/prettier

✖ 2 problems (2 errors, 0 warnings)
  2 errors, 0 warnings potentially fixable with the `--fix` option.

yarn run v1.6.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.ui.es6.js
[13:25:52] '/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.ui.es6.js' is being checked.
✨  Done in 0.71s.

/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.ui.es6.js
  202:31  error  Replace `'.media-library-widget-modal·.ui-dialog-buttonpane'` with `⏎··········'.media-library-widget-modal·.ui-dialog-buttonpane',⏎········`  prettier/prettier
  213:49  error  Replace `'.media-library-selected-count'` with `⏎··········'.media-library-selected-count',⏎········`                                          prettier/prettier

✖ 2 problems (2 errors, 0 warnings)
  2 errors, 0 warnings potentially fixable with the `--fix` option.

yarn run v1.6.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.view.es6.js
[13:25:55] '/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.view.es6.js' is being checked.
✨  Done in 0.67s.

/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.view.es6.js
  15:9  error  Replace `⏎········'.js-click-to-select-trigger,·.js-click-to-select-checkbox',⏎········context,⏎······` with `'.js-click-to-select-trigger,·.js-click-to-select-checkbox',·context`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

yarn run v1.6.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.view.es6.js
[13:25:57] '/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.view.es6.js' is being checked.
✨  Done in 0.71s.

/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.view.es6.js
  15:9  error  Replace `⏎········'.js-click-to-select-trigger,·.js-click-to-select-checkbox',⏎········context,⏎······` with `'.js-click-to-select-trigger,·.js-click-to-select-checkbox',·context`  prettier/prettier

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

yarn run v1.6.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.widget.es6.js
[13:26:00] '/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.widget.es6.js' is being checked.
✨  Done in 0.65s.
yarn run v1.6.0
warning From Yarn 1.0 onwards, scripts don't require "--" for options to be forwarded. In a future version, any explicit "--" will be forwarded as-is to the scripts.
$ cross-env BABEL_ENV=legacy node ./scripts/js/babel-es6-build.js --check --file /Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.widget.es6.js
[13:26:02] '/Users/gaborhojtsy/Drupal/commits/core/modules/media_library/js/media_library.widget.es6.js' is being checked.
✨  Done in 0.66s.
PHPCS: core/modules/media_library/media_library.install passed
PHPCS: core/modules/media_library/media_library.libraries.yml passed
PHPCS: core/modules/media_library/media_library.module passed
PHPCS: core/modules/media_library/media_library.routing.yml passed
PHPCS: core/modules/media_library/media_library.services.yml passed
PHPCS: core/modules/media_library/src/Form/MediaLibraryUploadForm.php passed
PHPCS: core/modules/media_library/src/MediaLibraryState.php passed

FILE: ...commits/core/modules/media_library/src/MediaLibraryUiBuilder.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 185 | WARNING | [x] A comma should follow the last multiline array
     |         |     item. Found: ]
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 53ms; Memory: 6Mb

PHPCS: core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php passed
PHPCS: core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php passed
PHPCS: core/modules/media_library/tests/fixtures/update/drupal-8.media_library-update-widget-view-3020716.php passed
PHPCS: core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_form_display.node.basic_page.default.yml passed
PHPCS: core/modules/media_library/tests/modules/media_library_test/config/install/core.entity_view_display.node.basic_page.default.yml passed
PHPCS: core/modules/media_library/tests/modules/media_library_test/config/install/field.field.node.basic_page.field_single_media_type.yml passed
PHPCS: core/modules/media_library/tests/modules/media_library_test/config/install/field.storage.node.field_single_media_type.yml passed
PHPCS: core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateWidgetViewTest.php passed
PHPCS: core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php passed
PHPCS: core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php passed
PHPCS: core/modules/media_library/tests/src/Kernel/MediaLibraryStateTest.php passed
seanB’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
145.81 KB

Sorry about that. I ran yarn prettier and fixed the PHPCS error.

seanB’s picture

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

OK, here we go again.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work

Uh-oh. Kicking back for PHP 5.x failures...

seanB’s picture

Strange 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.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

That's a big wall of green. I'm satisfied.

xjm’s picture

Priority: Normal » Major

This is not a normal issue. :)

larowlan’s picture

Copying 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.

  1. +++ b/core/modules/media_library/css/media_library.theme.css
    @@ -5,6 +5,84 @@
    +  border-right: 1px solid #fcfcfa;
    ...
    +  border-left: 1px solid #b3b2ad;
    

    do we need RTL equivalents here?

  2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,312 @@
    +            $('#media-library-content').focus();
    

    should we use getElementById here instead of jQuery?

  3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
    @@ -0,0 +1,312 @@
    +          .find('#media-library-modal-selection')
    +          .val(currentSelection.join())
    

    note to self, do we check access on this

  4. +++ b/core/modules/media_library/media_library.module
    @@ -90,11 +68,7 @@ function media_library_views_post_render(ViewExecutable $view, &$output, CachePl
    +      $query = MediaLibraryState::fromRequest()->all();
    

    we should pass along the request here

  5. +++ b/core/modules/media_library/media_library.services.yml
    @@ -0,0 +1,4 @@
    +  media_library.ui_builder:
    

    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.

  6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,401 @@
    +    $source_field_name = $this->getSourceFieldName($media->bundle->entity);
    +    if (isset($element['fields'][$source_field_name])) {
    +      $element['fields'][$source_field_name]['#attributes']['class'][] = 'media-library-add__source-field';
    +    }
    

    Why do we hide them with CSS and not use form modes?

  7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,401 @@
    +    return $this->entityTypeManager
    +      ->getStorage('media')
    +      ->create([
    +        'bundle' => $media_type->id(),
    +        $this->getSourceFieldName($media_type) => $source_field_value,
    

    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

  8. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,204 @@
    +      $request->query->get('media_library_opener_id'),
    +      $request->query->get('media_library_allowed_types'),
    +      $request->query->get('media_library_selected_type'),
    +      $request->query->get('media_library_remaining')
    

    I think we need an anti-tamper token here too

  9. +++ b/core/modules/media_library/src/MediaLibraryState.php
    @@ -0,0 +1,204 @@
    +    $existing_ids = \Drupal::entityTypeManager()->getStorage('media_type')
    

    should we inject this?

  10. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +    $state = MediaLibraryState::fromRequest();
    ...
    +    $state = MediaLibraryState::fromRequest();
    

    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

  11. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +    $view = Views::getView('media_library');
    

    should we inject here instead of using the singleton?

  12. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +    $allowed_types = $this->entityTypeManager->getStorage('media_type')->loadMultiple($allowed_type_ids);
    

    yeah, defs should be adding an anti-tamper token to prevent people meddling with the allowed types

  13. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +      $query['media_library_selected_type'] = $allowed_type_id;
    

    the last one is always selected?

  14. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -41,6 +43,13 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
    +  protected static $openerIdPrefix = 'field:';
    

    are we sure a global static is the right choice? - from the uses I see, could be a constant

  15. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -64,6 +64,17 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +    $selection_field_id = $this->options['id'] . '_selection';
    +    $form[$selection_field_id] = [
    +      '#type' => 'hidden',
    +      '#attributes' => [
    +        // This is used to identify the hidden field in the form via JavaScript.
    +        'id' => 'media-library-modal-selection',
    +      ],
    +    ];
    

    what controls are in place to prevent someone from editing the dom on this and adding in references to media entities they cannot access?

  16. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -95,17 +109,22 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +    $opener_id = MediaLibraryState::fromRequest()->getOpenerId();
    

    we should pass on the request here, obtained from DI on the plugin

dww’s picture

Status: Reviewed & tested by the community » Needs work

Great 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. ;)

seanB’s picture

Status: Needs work » Needs review
FileSize
14.16 KB
147.68 KB

1. 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:

seanb: @larowlan Looking at your review now. I have a question about the tamper token. Since this is “just” a widget for and entity reference field, the validation of that field ensures users can not select anything they shouldn’t. So my question is, do we then really need it?

larowlan: I’d need to think about it, the allowed types feels like something we’d want to prevent fiddling with, but happy to be wrong

seanb: I personally think the media library shouldn’t care about that. The validation of the selected values is on the caller (in this case an entity reference field).

seanb: Same as with the autocomplete entity reference field, where users can type in whatever they want.

larowlan: ok, seems reasonable, can you comment to that effect?

seanb: Yes, new patch coming up.
seanB’s picture

Issue summary: View changes
FileSize
844.88 KB
846.09 KB
550.79 KB
540.56 KB

Screenshots of the LTR vs RTL display.

Media library selection:
Media Library LTR
Media Library RTL

Add media form:
Added items LTR
Added items RTL

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

This looks pretty good to me. I think @larowlan's feedback has been adequately addressed. Let's get it in.

dww’s picture

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
index 72a4b50bbf..d1d6b4cad6 100644
--- a/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php

+++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
@@ -95,17 +109,22 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
+    $opener_id = MediaLibraryState::fromRequest(\Drupal::request())->getOpenerId();

Very 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

seanB’s picture

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.

That code is in updateWidget() which is a static method.

knyshuk.vova’s picture

The patch looks good and applies successfully. +1 for RTBC.

  • Gábor Hojtsy committed 012172e on 8.7.x
    Issue #3020716 by seanB, lauriii, phenaproxima, Gábor Hojtsy, dww,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Superb thanks all for the thorough reviews and diligent implementation.

seanB’s picture

YAY! Thanks everyone.

andrewmacpherson’s picture

Re #52:

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

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.

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.

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:

Re: the accessibility review, looking at #20 and #21 as far as I see all but one of the issues raised were fixed.

You mean from #18? I can't tell from the issue comments which one remains. Is there a follow-up issue for it?

Gábor Hojtsy’s picture

So #18 is referred in #20 and #21.

  • #20 left 1, 2, 4.3 and 5.4 unresolved from #18.
  • Then #21 resolved 1 and 2 and called out 4.3 covered by #3023809: Add a selection overview to the media library widget modal.
  • Then it posed this question about 5.4:
    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?

That said, that remaining point may not need a followup.

andrewmacpherson’s picture

Thanks 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.

andrewmacpherson’s picture

Added a follow-up, the ARIA live region for the selection count isn't managed correctly. I wasn't explicit enough in #18.4...

When this count changes, it would be good to inform all users. Replacing the text inside a <div aria-live="polite"> should suffice.

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.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

andrewmacpherson’s picture

Issue tags: -accessibility (duplicate tag) +Accessibility

fixing accessibility tag