Create a field widget for the Media library module

@samuel.mortenson demoed this patch in the UX meeting today. Here's the feedback we got:

• For the most part, it works very well ("it does what you expect it to do") and satisfied accessibility requirements. Everyone really liked it. Great work!
• The weight fields are not labeled specifically enough for accessibility purposes. Their labels should include a visually hidden span tag with the name of the item being weighted. For example: <label>Weight<span class="visually-hidden"> for $title_of_item</span></label>/li>
• The media library, when opened in the modal dialog, is affected by #2861860: View inside core modal doesn't refresh correctly when reopened. I don't think that issue needs to block this one, but it's likely a blocker for the media library module (as a whole) to be considered stable.
• In the dialog, the "Apply filters" button is...techy. Could we use better wording? I'm not sure what would be better; this likely needs UX input and can be tackled in a follow-up.
• Is it possible to move the "Apply filters" button to be inline with the filters? It is jarring to put it at the bottom of the dialog box, since those actions are placed in a way which communicates that clicking any of them will finish the dialog. If this is easy, let's do it now; otherwise, we can open a follow-up.

The remove button and weight are accessible, which is good, but more review would be nice.

We reviewed this in the UX meeting today. The weight field just has a label which says "weight" and it's not clear which media item it refers to. So we'd want either:

1. An invisible span inside the label to clarify what media item the label was for, e.g. <label>Weight<span class="visually-hidden"> for blue bunny</span></label>. This option is the more robust, because assistive tech will always have it as part of the "accessible name" of the input.
2. Use Form API #description so the input gets an aria-describedby relationship. You could put a .visually-hidden class on the description. This option would put the information in the "accessible description" of the input, which isn't guaranteed to be announced by all screen readers, say.

Don't try to convey this with a title attribute, that won't work.

We didn't look at the remove button in detail, but the same point applies; the button label needs to make it clear which media item will be removed.

The multi-value widgets currently used in core have table rows wrapping the weight and remove button, so their context comes from that semantic table row wrapper.

Leaving the needs accessibility review tag on this, I'm sure I'll want to have another look.

Looking pretty good :)

1. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   @@ -416,6 +417,166 @@
   +      path: admin/content/media-widget
   

   Should we create a new display plugin so that we don't have to expose a full path to this display? After all, it's never meant to be used for anything but the media library widget. I'm not sure if that's useful, feasible, or how much work it would take.

2. +++ b/core/modules/media_library/media_library.libraries.yml
   @@ -23,0 +24,10 @@
   +  dependencies:
   +    - core/drupal
   +    - core/jquery
   +    - core/jquery.ui.sortable
   

   jquery.ui.sortable already depends on jquery, so we can remove the redundant dependency.

3. +++ b/core/modules/media_library/media_library.module
   @@ -41,6 +41,11 @@
   +    // Ensure that the widget ID passed to the modal persists across rebuilds.
   +    $request = \Drupal::request();
   +    if ($request->query->has('media_library_widget_id')) {
   +      $output['#attached']['drupalSettings']['views']['ajax_path'] .= '?media_library_widget_id=' . $request->query->get('media_library_widget_id');
   +    }
   

   Can we add a comment linking to the related issues/blockers?

4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,62 @@
   + * @file media_library.widget.js
   

   We'll need JavaScript review on this file, and it couldn't hurt to run it through eslint as well.

5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,62 @@
   +  Drupal.AjaxCommands.prototype.addMediaToWidget = function (ajax, response, status) {
   

   I'm not sure we need this command -- we could just use a pair of InvokeCommands to call .val() and .trigger() on the relevant elements.

6. +++ b/core/modules/media_library/media_library.views.inc
   @@ -0,0 +1,22 @@
   +  $data['media']['media_library_select_form'] = [
   +    'title' => t('Select media'),
   +    'help' => t('Provides a field for selecting media entities in our media library view'),
   +    'real field' => 'mid',
   +    'field' => [
   +      'id' => 'media_library_select_form',
   +    ],
   

   Since this functionality will hopefully be ported into Views at some point in the future, should we rename this to something more generic, like entity_selection_form?

7. +++ b/core/modules/media_library/src/Ajax/AddMediaToWidget.php
   @@ -0,0 +1,61 @@
   +class AddMediaToWidget implements CommandInterface {
   

   We can remove this entire class if we use InvokeCommand as suggested previously.

8. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,443 @@
   + *   description = @Translation("Allows you to re-use Media Entities."),
   

   Nit: This can be "media entities".

9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,443 @@
   +    $build = parent::form($items, $form, $form_state, $get_delta);
   +
   +    return $build;
   

   Nit: Let's just return parent::form().

10. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +        '#markup' => $this->t('<p>No media selected.</p>'),
    

    Can we say "No media items selected"?

11. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +        '#value' => $this->t('Toggle media item weights'),
    

    This will make the JS a little more complex, but can we say "Show" and "Hide" instead of "Toggle"? Also, are there any relevant aria-* accessibility attributes that might be useful here?

12. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +              '#media_library_remove_delta' => $delta,
    

    Could we extract this delta from the triggering element's #parents? If not, no big deal; just curious.

13. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +    // @todo Contextual filters only work the first time the modal is loaded.
    +    // Investigate and file an issue for Views if possible.
    

    If there is already an issue, let's link to it here.

14. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +    $element['media_library_open_container'] = [
    

    Why do we need a container here? I feel like we could use #theme_wrappers on the button itself for that.

15. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +        // @todo Make the View configurable.
    +        '#url' => Url::fromRoute('view.media_library.widget', [], [
    +          'query' => ['media_library_widget_id' => $field_name . $id_suffix],
    +        ]),
    

    I don't think we need to expose a UI for it right now, but let's go ahead and make it a setting now, and open a follow-up to expose it as an option.

16. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +          'data-dialog-options' => json_encode([
    +            'dialogClass' => 'media-library-widget-modal',
    +            'height' => '75%',
    +            'width' => '75%',
    +            'title' => $this->t('Media library'),
    +          ]),
    

    We should use \Drupal\Component\Serialization\Json::encode().

17. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +        $form_state->setError($element, t('More items have been selected than this field allows (max @count).', [
    +          '@count' => $element['#cardinality'],
    +        ]));
    

    We can phrase this more lucidly, I think, using the plural: "This field cannot accept more than @count item(s)."

18. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +        if ($element['#target_bundles'] && !in_array($media_item->bundle(), $element['#target_bundles'], TRUE)) {
    +          $form_state->setError($element, t('The media item "@label" is not of an accepted type.', [
    +            '@label' => $media_item->label(),
    +          ]));
    +        }
    

    This points to a UX hole; the view should not even *show* any media items that are of an unacceptable type. We can open a follow-up for that, but it may well be a stability blocker. =\

    To mitigate this for now, though, let's add a helpful description to the widget which says something like "You may select media items of the Foo, Bar, and Baz types."

19. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +  public static function getNewMediaItems(array $element, FormStateInterface $form_state) {
    

    Does this need to be public? We should avoid adding API surface if possible.

20. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +  public static function getFieldState(array $element, FormStateInterface $form_state) {
    

    Same here -- not sure this needs to be public.

21. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,443 @@
    +  public static function setFieldState(array $element, FormStateInterface $form_state, array $field_state) {
    

    Here too.

22. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,129 @@
    +    // Only add the bulk form options and buttons if there are results.
    +    if (!empty($this->view->result)) {
    

    Lets just return early if $this->view->result is empty, so we don't have to indent the entire function.

23. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,129 @@
    +        $form[$this->options['id']][$row_index] = [
    +          '#type' => 'checkbox',
    +          '#title' => $this->t('Select this item'),
    

    For accessibility, can we improve the label here? Something like 'Select "$item_label"' would do the trick.

24. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,129 @@
    +      $form['actions']['submit']['#ajax'] = [
    +        'url' => Url::fromUserInput($url),
    +        'options' => [
    +          'query' => \Drupal::request()->query->all() + [
    +            FormBuilderInterface::AJAX_FORM_REQUEST => TRUE,
    +          ],
    +        ],
    +        'callback' => [static::class, 'updateWidget'],
    +      ];
    

    Can there be a comment explaining this?

Tagging for JavaScript review.

Tagging for a follow-up issue per #13.15.

I also discovered an issue we need to address - the Media library widget display currently requires the "Access media overview" permission to use. This was inherited by the default display (Page), and we can override it so that the "View media" permission is used instead which makes sense to me.

This is a question that should be addressed by the UX team, IMHO.

But here's an interesting case - if a user with permission to use the widget does not have the "view the administration theme", the Media library would render using the frontend theme, which would make styling super inconsistent. Is there a way for us to always use the admin theme for the widget path, even if a user does not have the "view the administration theme" permission?

And this should be answered by a front-end framework manager.

But here's an interesting case - if a user with permission to use the widget does not have the "view the administration theme", the Media library would render using the frontend theme, which would make styling super inconsistent. Is there a way for us to always use the admin theme for the widget path, even if a user does not have the "view the administration theme" permission?

Theme authors should provide styles for this part of the system just like for any other part. We already have this problem for example with node forms that by a good chance they might be rendered using either, the admin or the frontend theme.

I'm leaving the tag since I didn't have a chance to review the frontend parts of the patch yet.

We discussed that we can make adding oEmbed media in the widget something that we discuss in a followup.

The media library has been committed, so let's put booster rockets on this thing.

Marking for a re-roll.

1. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   @@ -498,0 +458,127 @@
   +      path: admin/content/media-widget
   

   It'd be nice to not expose this display as a page, but we can deal with that in a follow-up.

2. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   @@ -498,0 +458,127 @@
   +          element_class: media-library-item__content
   

   Is there an existing class name we could re-use?

3. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   @@ -498,0 +458,127 @@
   +      access:
   +        type: perm
   +        options:
   +          perm: 'view media'
   

   I wonder if this might raise some eyebrows. Even if it does, I'm not sure there's much we can do about it until we are using a different display plugin.

4. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -47,6 +48,22 @@
   +.media-library-view.view-display-id-widget .media-library-select-all {
   +  display: none;
   +}
   

   I'm not sure if this would annoy the UX team, but why shouldn't we expose the ability to select all items?

5. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -47,6 +48,22 @@
   +.media-library-selection .media-library-item::after {
   +  display: none;
   +}
   

   What does this do?

6. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -19,7 +19,53 @@
   +.media-library-item .media-library-item-remove:hover,
   +.media-library-item .media-library-item-remove:focus,
   +.media-library-item .media-library-item-remove.button:disabled {
   

   Not a CSS expert, but .media-library-item-remove seems like it should be renamed to media-library-item__remove, for BEM conformance.

7. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -19,7 +19,53 @@
   +  background: url('../../../misc/icons/787878/ex.svg') #ffffff center no-repeat;
   

   Nit: Could use #fff.

8. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -19,7 +19,53 @@
   +.media-library-item .media-library-item-remove {
   +  position: absolute;
   +  z-index: 1;
   +  top: 0;
   +  right: 0;
   +  width: 24px;
   +  height: 24px;
   +  margin: 5px;
   +  padding: 0;
   +  background: url('../../../misc/icons/787878/ex.svg') #ffffff center no-repeat;
   +  background-size: 16px 16px;
   +  border: 2px solid #ccc;
   +  border-radius: 20px;
   +  color: transparent;
   +  text-shadow: none;
   +  transition: .2s border-color;
   

   Let's group this one with the :hover and :focus pseudo-selectors.

9. +++ b/core/modules/media_library/media_library.module
   @@ -43,6 +44,21 @@
   +    if ($view->current_display === 'widget') {
   

   I'm not a huge fan of the fact that this is hard-coded, but I think we could address that later. Maybe a follow-up?

10. +++ b/core/modules/media_library/media_library.module
    @@ -43,6 +44,21 @@
    +      if (!empty($query)) {
    +        $output['#attached']['drupalSettings']['views']['ajax_path'] .= '?' . http_build_query($query);
    +        if (isset($query['media_library_remaining'])) {
    +          $output['#attached']['drupalSettings']['media_library']['selection_remaining'] = $query['media_library_remaining'];
    +        }
    +      }
    

    This could use a comment or two.

11. +++ b/core/modules/media_library/media_library.module
    @@ -90,0 +107,62 @@
    + * Alters the widget view's query to only show media that can be selected.
    

    Can we expand on what "can be selected" means? By what criteria are we determining selectability?

12. +++ b/core/modules/media_library/media_library.module
    @@ -90,0 +107,62 @@
    +  if ($view->id() == 'media_library' && $view->current_display === 'widget') {
    

    The first condition needs ===.

13. +++ b/core/modules/media_library/media_library.module
    @@ -90,0 +107,62 @@
    +    $types = \Drupal::request()->query->get('media_library_allowed_types');
    +    if ($types && is_array($types)) {
    

    This code is repeated later in the module, so can we make a helper function? Also, we should ensure that $types is non-empty and consists entirely of strings.

14. +++ b/core/modules/media_library/media_library.module
    @@ -90,0 +107,62 @@
    +          'field' => 'media_field_data.bundle',
    

    Is there any way we can avoid hard-coding the name of the table?

15. +++ b/core/modules/media_library/media_library.module
    @@ -90,0 +107,62 @@
    +          'operator' => 'in',
    

    Should 'in' be all-caps?

16. +++ b/core/modules/media_library/media_library.module
    @@ -90,0 +107,62 @@
    + * filter by a type that is not un-selectable.
    

    Not sure that "not" should be here.

17. +++ b/core/modules/media_library/media_library.module
    @@ -90,0 +107,62 @@
    +  /** @var \Drupal\Core\Field\FieldTypePluginManager $field_type_manager */
    +  $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
    +  $class = $field_type_manager->getPluginClass($field_type);
    

    We don't need to keep $field_type_manager in its own variable.

18. +++ b/core/modules/media_library/tests/modules/media_library_test/media_library_test.info.yml
    @@ -9,2 +9,3 @@
    +  - drupaL:node
    

    drupaL! Sounds like a fork =P

19. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -92,11 +99,104 @@
    +    $this->getSession()->executeScript("jQuery('$checkbox_selector:eq(3)').click()");
    +    $this->getSession()->executeScript("jQuery('$checkbox_selector:eq(4)').click()");
    +    $this->getSession()->executeScript("jQuery('$checkbox_selector:eq(5)').click()");
    +    $this->getSession()->executeScript('jQuery(\'button:contains("Select media")\').click()');
    

    Let's keep $session in its own variable to avoid calling getSession() repeatedly.

1. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +        }
   

   Missing trailing comma.

2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +        'show': Drupal.t('Show media item weights'),
   +        'hide': Drupal.t('Hide media item weights')
   

   These properties don't need to be quoted. Additionally, probably don't need to be in an Object since they are used infrequently.

3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +            .find('.media-library-item-weight').parent().toggle();
   

   Both `.parent()` and `.toggle()` should be on their own line.

4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +              const $view = $(currentTarget).closest('.media-library-view');
   

   `$view` is already declared in the previous scope.

5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +              if ($checkboxes.filter(':checked').length == settings.media_library.selection_remaining) {
   

   Use triple-equals.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInterface {
   

   Let's mark this explicitly @internal, as all plugin classes are.

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
   

   I think a lot of the logic in here can potentially be encapsulated as its own form element class -- essentially it is "select some things from a view". What do you think -- could we implement that semi-generically? I don't know if we necessarily should bother with that now, if it will be very complicated, but it might be worth exploring in a follow-up.

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +            '#limit_validation_errors' => [array_merge($form['#parents'], [$field_name])],
   

   #limit_validation_errors is a tricky little devil. If we can avoid using it, that'd be nice. Otherwise, let's add a comment explaining what this is for.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +        'weight' => [
   +          '#type' => 'textfield',
   +          '#title' => $this->t('Weight'),
   +          '#default_value' => $delta,
   +          '#attributes' => [
   +            'class' => ['media-library-item-weight'],
   +          ],
   +        ],
   

   Should this element's #type be 'number', since it must be a number?

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +    // Inform the user of how many items are remaining.
   +    if (!$cardinality_unlimited) {
   +      if ($element['#cardinality'] === 1 && $remaining === 1) {
   +        $cardinality_message = 'You can select one media item.';
   +      }
   +      elseif ($remaining) {
   +        $cardinality_message = 'You can select up to @maximum media (@remaining left).';
   +      }
   +      else {
   +        $cardinality_message = 'The maximum number of media has been selected.';
   +      }
   +      $element['#description'] .= '<br />' . $this->t($cardinality_message, [
   +        '@maximum' => $element['#cardinality'],
   +        '@remaining' => $remaining,
   +      ]);
   +    }
   

   I love this.

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +          'media_library_remaining' => $cardinality_unlimited ? -1 : $remaining,
   

   We should use the CARDINALITY_UNLIMITED constant, rather than hard-code -1.

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +      '#limit_validation_errors' => [array_merge($parents, [$field_name])],
   

   A comment would be good here, since #limit_validation_errors is so opaque.

8. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +        'data-media-library-widget-value' => $field_name . $id_suffix,
   

   A comment would be useful here.

9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,470 @@
   +    $element['media_library_update_widget'] = [
   +      '#type' => 'submit',
   +      '#value' => $this->t('Update widget'),
   +      '#name' => $field_name . '-media-library-update' . $id_suffix,
   +      '#ajax' => [
   +        'callback' => [static::class, 'updateWidget'],
   +        'wrapper' => $wrapper_id,
   +      ],
   +      '#attributes' => [
   +        'data-media-library-widget-update' => $field_name . $id_suffix,
   +        'class' => ['visually-hidden', 'media-library-update'],
   +      ],
   +      '#validate' => [[static::class, 'validateItems']],
   +      '#submit' => [[static::class, 'updateItems']],
   +      '#limit_validation_errors' => [array_merge($parents, [$field_name])],
   +    ];
   

   What does this do? A comment would be useful here as well.

10. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +    $length = end($triggering_element['#parents']) === 'remove_button' ? -4 : -1;
    

    Can we get a comment here?

11. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +    if (isset($values['selection'])) {
    +      if (isset($values['selection'][$delta])) {
    +        array_splice($values['selection'], $delta, 1);
    +        $field_state['items'] = $values['selection'];
    +        $field_state['items_count'] = count($field_state['items']);
    +      }
    +    }
    

    I don't know if this makes sense, but I almost feel like the "current selection" should be encapsulated in some kind of value object or something, just to make it easier to understand and manipulate. But, that's not a priority right now unless it's easy.

12. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +    $field_state = self::getFieldState($element, $form_state);
    +    $media = self::getNewMediaItems($element, $form_state);
    

    Why self instead of static here?

13. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +      $form_state->setError($element, t('This field cannot accept more than @count item(s).', [
    +        '@count' => $element['#cardinality'],
    +      ]));
    

    I think we'll need a PluralMarkup here.

14. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +        $form_state->setError($element, t('The media item "@label" is not of an accepted type. Allowed types: @types', [
    +          '@label' => $media_item->label(),
    +          '@types' => implode(', ', $element['#target_bundles']),
    +        ]));
    

    "Not of an accepted type" sounds developer-ey to me. Not quite sure how to rephrase it; maybe this is one for the UX team.

15. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +   * Updates the field state based on the form state and sets the form rebuild.
    

    Should be "...and flags the form to be rebuilt."

16. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +    $field_state = self::getFieldState($element, $form_state);
    +
    +    $media = self::getNewMediaItems($element, $form_state);
    

    Should self be static?

17. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +      if ($media && $media_item->access('view')) {
    

    Why is the ->access() check being done here? Needs a comment, I think.

18. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,470 @@
    +    // Get the new media ids passed to our hidden button.
    

    Nit: 'ids' --> 'IDs'

19. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,137 @@
    +class MediaLibrarySelectForm extends FieldPluginBase {
    

    Let's mark this guy explicitly @internal too.

Good stuff. This is only a partial review, but I think this patch is shaping up. We can get away with a lot since Media Library is experimental.

1. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -2,22 +2,19 @@
   +.media-library-views-form__header > div,
   

   Is there any more specific CSS class/selector we can use to target the inner elements, not just div?

2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,94 @@
   +          $(target).children().each((index, child) => {
   +            $(child).find('.media-library-item-weight').val(index);
   +          });
   

   Correct me if I'm wrong, but can't this be rephrased to just $(target).find('.media-library-item-weight').each((weight, element) => { element.value = weight; })?

3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,94 @@
   +        .attr('title', Drupal.t('Re-order media by numerical weight instead of dragging.'))
   

   Can we set this attribute on the server side, rather than in JavaScript?

4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,94 @@
   +      if ($view.length && $view.find('.media-library-item').length) {
   

   I'm not sure we need this if block. We could just do the .find().on('change') stuff, and if nothing is found...it'll have no effect.

5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,94 @@
   +              if ($checkboxes.filter(':checked').length == settings.media_library.selection_remaining) {
   

   ===

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   + *   description = @Translation("Allows you to re-use media entities."),
   

   Let's improve this -- how about "Allows you to select items from the media library"?

Another partial review.

1. +++ b/core/modules/media_library/media_library.module
   @@ -43,6 +45,22 @@ function media_library_theme() {
   +        $output['#attached']['drupalSettings']['views']['ajax_path'] .= '?' . http_build_query($query);
   

   Will this query string be attached to *all* AJAX rebuilds on the page? If so, it's probably not a problem because all our query string parameters are prefixed with media_library...but we should mention that in a comment, and probably have a test scenario with the media library co-existing with another AJAX-enabled view on the same page. I don't mind deferring that to a follow-up, though.

2. +++ b/core/modules/media_library/media_library.module
   @@ -93,6 +109,81 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   +  if ($form_object instanceof ViewsForm && strpos($form_object->getBaseFormId(), 'views_form_media_library') === 0) {
   

   Couldn't we implement hook_form_BASE_FORM_ID_alter() for this?

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   +    $element['empty_selection'] = [
   +      '#markup' => $this->t('<p>No media items are selected.</p>'),
   +      '#access' => empty($referenced_entities),
   +    ];
   +
   +    $element['weight_toggle'] = [
   +      '#type' => 'html_tag',
   +      '#tag' => 'button',
   +      '#value' => $this->t('Show media item weights'),
   +      '#attributes' => [
   +        'class' => ['link', 'media-library-toggle-weight'],
   +      ],
   +      '#access' => !empty($referenced_entities),
   +    ];
   

   Nit: Why not add this to the big $element += operation above?

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   +          'class' => ['media-library-item', 'media-library-item-in-widget'],
   

   What is the 'media-library-item-in-widget' class used for?

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   +          'view' => $view_builder->view($media_item, 'media_library'),
   

   Can we rename this element to rendered_entity, to prevent confusion with Views?

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   +            'class' => ['media-library-item-weight'],
   

   Maybe this should be media-library-item__weight for BEM compliance.

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   +    $remaining = $element['#cardinality'] - count($referenced_entities);
   

   We should only be calculating this if !$cardinality_unlimited.

8. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   +        $cardinality_message = $this->t('You can select one media item.');
   +      }
   +      elseif ($remaining) {
   +        $cardinality_message = $this->t('You can select up to @maximum media (@remaining left).', [
   +          '@maximum' => $element['#cardinality'],
   +          '@remaining' => $remaining,
   +        ]);
   +      }
   +      else {
   +        $cardinality_message = $this->t('The maximum number of media has been selected.');
   

   Let's say "media item" or "media items" everywhere, for consistency.

9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,479 @@
   +    $element['media_library_selection'] = [
   +      '#type' => 'textfield',
   +      '#attributes' => [
   +        // This is used to pass the selection from the modal to the widget.
   +        'data-media-library-widget-value' => $field_name . $id_suffix,
   +        'class' => ['visually-hidden'],
   +      ],
   +    ];
   

   Why not use a hidden field?

10. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,479 @@
    +      '#attributes' => [
    +        'data-media-library-widget-update' => $field_name . $id_suffix,
    +        'class' => ['visually-hidden', 'media-library-update'],
    +      ],
    

    I'm not sure this should be visually hidden; it seems like something we *always* want to hide, even from screen readers. Maybe we should just hard-code a display: none there?

11. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,479 @@
    +    else {
    +      return [];
    +    }
    

    No need for the else {} boilerplate. We can just return [].

12. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,479 @@
    +    $length = end($triggering_element['#parents']) === 'remove_button' ? -4 : -1;
    

    Am I overly paranoid for thinking this is potentially brittle?

13. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,479 @@
    +    $parents = array_slice($triggering_element['#array_parents'], 0, -4);
    +    $delta = array_slice($triggering_element['#array_parents'], -3, 1)[0];
    

    This is confusing. Can we get a comment explaining what we expect these arrays to look like?

14. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,479 @@
    +    if (isset($values['selection'])) {
    +      if (isset($values['selection'][$delta])) {
    

    These can be combined into a single if statement. isset($values['selection'][$delta])

15. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,479 @@
    +        array_splice($values['selection'], $delta, 1);
    +        $field_state['items'] = $values['selection'];
    +        $field_state['items_count'] = count($field_state['items']);
    

    This could use some comments as well.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +    if (isset($values['selection']) && isset($values['selection'][$delta])) {
   

   Pro tip: isset() is variadic. This could be isset($values['selection'], $values['selection'][$delta]), and it will only be TRUE if both are set. However, I don't think we need to explicitly check for $values['selection'], so just isset($values['selection'][$delta]) should work fine here.

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +    static::setFieldState($element, $form_state, $field_state);
   

   Nit: This can go inside the if block, since that's the only place we're actually changing $field_state.

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +   * Making an invalid selection from the View should not be possible, but we
   

   Supernit: 'view' should be lowercase.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +      $form_state->setError($element, \Drupal::translation()->formatPlural($element['#cardinality'], 'This field only accepts one item.', 'This field cannot accept more than @count item.'));
   

   Correct me if I'm wrong, but isn't formatPlural() part of StringTranslationTrait, which I believe we're using in this class? Also, the plural string should end with "items", not "item".

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +      if ($element['#target_bundles'] && !in_array($media_item->bundle(), $element['#target_bundles'], TRUE)) {
   +        $form_state->setError($element, t('The media item "@label" is not of an accepted type. Allowed types: @types', [
   +          '@label' => $media_item->label(),
   +          '@types' => implode(', ', $element['#target_bundles']),
   +        ]));
   

   Is #target_bundles a set of machine names? If so, we should put the human-readable labels of the media types in this message, rather than the machine names.

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +    $field_state['items'] = isset($field_state['items']) ? $field_state['items'] : [];
   

   Why not have have getWidgetState() always return an empty array of items, if one is not already set?

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +      if ($media_item && $media_item->access('view')) {
   

   We can remove the first condition -- if $media_item is falsy, it means there is an error condition and we should allow it to fail hard so we can easily trace and debug the problem.

8. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +          'weight' => $weight++,
   

   Do we need an explicit weight? The items are numerically indexed, which implicitly serves as a weight.

9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,482 @@
   +   * @return array|\Drupal\media\MediaInterface[]
   

   We don't need the 'array' type hint; the second one is more useful and specific.

10. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,482 @@
    +  public static function getNewMediaItems(array $element, FormStateInterface $form_state) {
    

    I don't know if this needs to be public.

11. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,482 @@
    +      $ids = explode(',', $value['media_library_selection']);
    

    We should probably validate
    that these are all integers.

12. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,482 @@
    +   * @return array|mixed
    +   *   The field state.
    +   */
    +  public static function getFieldState(array $element, FormStateInterface $form_state) {
    

    It's always an array, so I'm not sure we need the mixed type hint. It would also be nice to document what is found in the array. Also, does the method need to be public?

13. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,482 @@
    +  public static function setFieldState(array $element, FormStateInterface $form_state, array $field_state) {
    

    Is there any reason for this to be public?

14. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,138 @@
    +        '#return_value' => $entity->getEntityTypeId() . ':' . $entity->id(),
    

    updateWidget() expects the entity type ID to be 'media' -- hard-coded -- so we should hard-code 'media' here too.

15. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,138 @@
    +    $form['actions']['submit']['#ajax'] = [
    +      'url' => Url::fromUserInput($url),
    +      'options' => [
    +        'query' => \Drupal::request()->query->all() + [
    +          FormBuilderInterface::AJAX_FORM_REQUEST => TRUE,
    +        ],
    +      ],
    

    I didn't know you could do this! Neat stuff.

16. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,138 @@
    +    $widget_id = \Drupal::request()->query->get('media_library_widget_id');
    

    Let's throw a BadRequestHttpException if $widget_id is invalid.

17. +++ b/core/modules/media_library/src/Routing/RouteSubscriber.php
    @@ -0,0 +1,22 @@
    +    if ($route = $collection->get('view.media_library.widget')) {
    +      $route->addRequirements(['_csrf_token' => 'TRUE']);
    +    }
    

    If my understanding of Drupal's CSRF system is correct -- and it's a very sketchy understanding indeed -- this will prevent the media library from being usable by anonymous users. Which might be an edge case, but are we sure we want to cut off this feature for anonymous users? Seems like it might be better to expose a new permission or something. However, I don't mind moving that to a follow-up.

        $cardinality_message = $this->t('You can select one media item.');
      }
      elseif ($remaining) {
        $cardinality_message = $this->t('You can select up to @maximum media items (@remaining left).', [

1. Remove "You can", we don't speak to our users in this form in our text.
2. The first one should also be *up to* one media item no? Or it depends on whether its a required field? Do we express the requiredness in some form?
3. If we can avoid the "@remaining left" if the maximum equals remaining, that would be nicer.

#38 was faster ... The "You can" immediately catched my eye. Agreed on all points of Gábor. Plus, I would love to know if this (modal popup) has been tested mobile and if it breaks if not Seven is admin theme?

EDIT: Let me rephrase the last question. I would like to prevent that the modal polish is "hard" connected with Seven and want to make sure that it is following general standarts for other themes to adapt, since otherwise this would cause a lot of trouble and work arounds in contrib again. (Or) we should add the option to switch between modal and a collapsible form or something similar. We all remember the issues raising regarding modals in admin area in D7 times? But its just a thought which came in mind...

Thanks so much for recording this video! The widget looks + works AMAZINGLY! :D Brilliant touch with "greying out" media when it can no longer be selected.

Two points of feedback:

1) I think the "add" help text should remain as part of this patch, and can be removed if/when #2938116: Allow media to be uploaded with the Media Library field widget makes it in. We want to always keep core in a "shippable state" and at the moment if we remove that text, we are regressing from the current level of functionality (clunky as it may be).

2) I noticed that even though you've attached Newman to the node, you can go back and re-attach Newman again the second time. I guess it's possible there's a use case for attaching the same image twice, but it seems like undesirable behaviour that's easy for someone to unintentionally do. Is it possible/easy to filter out from the Media Library selection media that are already attached to the node via the same field?

Is it possible/easy to filter out from the Media Library selection media that are already attached to the node via the same field?

It should be possible, but I don't know if that would be desirable. Entity reference fields by their nature allow one to reference the same item multiple times if desired, so I'm not sure if we should prevent that here. If anything, I'd prefer to open a follow-up to explore that possibility, rather than disable it in the initial patch.

Oh, and I guess a third point, though unrelated to this patch... is there an issue somewhere for making the "Add media" flow vastly simpler? I know we inherit all those bogus fields (URL alias, Revisions, etc.) from other parts of core, but here especially it is cringe-worthy how bad of a UX this creates. :\ Literally 3/4 of the screen is dedicated to completely irrelevant stuff to the behaviour you're trying to do. It might almost be worth one-offing with some form alters here to create a much slicker experience, as long as we're still in experimental mode. (Would need framework manager approval.)

OTOH, if the mocks at #2938116: Allow media to be uploaded with the Media Library field widget seem doable for 8.6, that probably negates this feedback.

Cross-post. A follow-up to discuss about re-attaching the same media to the same field sounds good!

Follow-up filed: #2981895: Prevent already-selected media items from being chosen twice

This was demoed in the UX call today -- everyone agrees that it's lightyears better than the current widget in core. @yoroy was not in the call, so we don't have official UX sign-off yet, but everyone in the call seemed very happy with it. @ckrina had some design feedback, but she didn't express any major concerns (potentially doable in a follow-up). She promised to post her feedback here too.

The issue about the design was that the first design styles are the ones implemented while the final version proposed has some modifications:
- Removes the white padding around the image
- Reduces the space for the title/link to the detail

My other concern was the dimensions of the cross icon, because I think it's too big for the space around it. Anyway it's a really minor issue and I'll check it with some more time because I'm not sure.

Every time I review this patch, I find less and less to complain about. The field widget makes a lot of sense to me. It's dense, but also deceptively simple. Really nice work here.

1. +++ b/core/modules/media/media.module
   @@ -336,3 +314,42 @@ function media_preprocess_media_reference_help(&$variables) {
   + * Returns the appropriate url to add media for the current user.
   

   Supernit: url --> URL

2. +++ b/core/modules/media/media.module
   @@ -336,3 +314,42 @@ function media_preprocess_media_reference_help(&$variables) {
   + *   The help url, or FALSE if the user cannot create any media.
   

   Same here. Also, what is meant by "help URL"?

3. +++ b/core/modules/media/media.module
   @@ -336,3 +314,42 @@ function media_preprocess_media_reference_help(&$variables) {
   +function _media_get_add_url($allowed_bundles) {
   

   Let's mark this function explicitly deprecated and internal, and link to the issue where we intend to remove it.

4. +++ b/core/modules/media/media.module
   @@ -336,3 +314,42 @@ function media_preprocess_media_reference_help(&$variables) {
   +  $bundle_labels = [];
   +  $create_bundles = [];
   +  foreach ($allowed_bundles as $bundle) {
   +    $bundle_labels[] = $all_bundles[$bundle]['label'];
   +    if ($access_handler->createAccess($bundle)) {
   +      $create_bundles[] = $bundle;
   +      if (count($create_bundles) > 1) {
   +        // If the user has access to create more than 1 bundle then the
   +        // individual media type form can not be used.
   +        break;
   +      }
   +    }
   +  }
   

   This seems like a good candidate for array_map() and array_filter().

5. +++ b/core/modules/media/media.module
   @@ -336,3 +314,42 @@ function media_preprocess_media_reference_help(&$variables) {
   +  // Add a section about how to create media if the user has access to do so.
   +  if (!empty($create_bundles)) {
   +    if (count($create_bundles) === 1) {
   +      $add_url = Url::fromRoute('entity.media.add_form', ['media_type' => $create_bundles[0]])->toString();
   +    }
   +    elseif (count($create_bundles) > 1) {
   +      $add_url = Url::fromRoute('entity.media.add_page')->toString();
   +    }
   +  }
   

   I don't think we need the !empty() check, and we can simply do early returns in each of the inner conditions. Then the whole function can simply end with return FALSE. I think that'll be a little more readable.

6. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -53,6 +54,18 @@
   +.media-library-view.view-display-id-widget .media-library-select-all {
   +  display: none;
   +}
   

   Do we have test coverage to assert that this checkbox is never visible in the widget?

7. +++ b/core/modules/media_library/media_library.module
   @@ -43,6 +45,24 @@ function media_library_theme() {
   +        $output['#attached']['drupalSettings']['views']['ajax_path'] .= '?' . http_build_query($query);
   

   Let's use UrlHelper::buildQuery() instead of http_build_query().

8. +++ b/core/modules/media_library/media_library.views.inc
   @@ -0,0 +1,22 @@
   + * Contains views integration for the media_library module.
   

   Supernit: 'views' should be 'Views', since it's the proper name of a module.

9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,503 @@
   +    $field_state = static::getWidgetState($parents, $field_name, $form_state);
   +    if ($field_state && isset($field_state['items'])) {
   

   I think static::getWidgetState() should always return an array with an 'items' key, which defaults to an empty array.

10. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      $element['create_help']['description'] = [
    +        '#type' => 'html_tag',
    +        '#tag' => 'div',
    +        '#attributes' => [
    +          'class' => ['description'],
    +        ],
    +        '#value' => t('Create your media on the <a href=":add_page" target="_blank">media add page</a> (opens a new window), then select it in the library.', [':add_page' => $add_url]),
    +      ];
    

    Sigh...oh, for the day when we can remove this.

11. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    $element['empty_selection'] = [
    +      '#markup' => $this->t('<p>No media items are selected.</p>'),
    +      '#access' => empty($referenced_entities),
    +    ];
    

    Rather than use #access, let's just generate this if (empty($referenced_entities)). There's no other conceivable reason why this element should exist at all.

12. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    $element['weight_toggle'] = [
    +      '#type' => 'html_tag',
    +      '#tag' => 'button',
    +      '#value' => $this->t('Show media item weights'),
    +      '#attributes' => [
    +        'class' => [
    +          'link',
    +          'media-library-toggle-weight',
    +        ],
    +        'title' => $this->t('Re-order media by numerical weight instead of dragging'),
    +      ],
    +      '#access' => !empty($referenced_entities),
    +    ];
    

    As with the previous thing, let's only create this element if $referenced_entities is not empty to begin with.

13. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +        'weight' => [
    +          '#type' => 'textfield',
    +          '#title' => $this->t('Weight'),
    +          '#default_value' => $delta,
    +          '#attributes' => [
    +            'class' => ['media-library-item__weight'],
    +          ],
    +        ],
    

    Although it breaks precedent, let's have #type => 'number' here. Weights cannot be anything except integers, so we should let the generated HTML reflect that.

14. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      // @todo Make the View configurable in https://www.drupal.org/project/drupal/issues/2971209
    

    Supernit: 'View' should be lowercase.

15. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      '#attributes' => [
    +        'class' => ['button', 'use-ajax', 'media-library-open-button'],
    +        'data-dialog-type' => 'modal',
    +        'data-dialog-options' => Json::encode([
    +          'dialogClass' => 'media-library-widget-modal',
    +          'height' => '75%',
    +          'width' => '75%',
    +          'title' => $this->t('Media library'),
    +        ]),
    +      ],
    

    Today I learned that the use-ajax class will have no effect if the core/drupal.ajax library is not loaded. So let's explicitly attach it to this button. :)

16. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    // items based on the  "media_library_selection" value.
    

    Superdupermeganit: There's an extra space after "...based on the".

17. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      '#attributes' => [
    +        'data-media-library-widget-update' => $field_name . $id_suffix,
    +        'class' => ['media-library-update'],
    +      ],
    

    Let's use the js-hide class to keep this button hidden, rather than a custom class.

18. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      '#validate' => [[static::class, 'validateItems']],
    

    Should we also apply this validation to the other AJAX-ey buttons exposed by this widget? Probably couldn't hurt.

19. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      '#limit_validation_errors' => [array_merge($parents, [$field_name])],
    

    Let's generate this array once at the top of the method and re-use it as needed. For readability.

20. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    $parents = $triggering_element['#array_parents'];
    +    $parents = array_slice($parents, 0, $length);
    

    We can condense these into one line: $parents = array_slice($triggering_element['#array_parents'], 0, $length).

21. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    // Get the parents required to find the top-level widget element.
    +    $parents = array_slice($triggering_element['#array_parents'], 0, -4);
    +    // Get the delta of the item being removed.
    +    $delta = array_slice($triggering_element['#array_parents'], -3, 1)[0];
    

    We should probably check that $parents is of the correct length before the second array_slice() call, and throw LogicException if it's not.

22. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    if (isset($values['selection'], $values['selection'][$delta])) {
    

    We can remove the first argument to isset().

23. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      $field_state['items_count'] = count($field_state['items']);
    

    Out of curiosity, why do we need $field_state['items_count']? Can't we just call count($field_state['items']) as needed?

24. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      $form_state->setError($element, \Drupal::translation()->formatPlural($element['#cardinality'], 'This field only accepts one item.', 'This field cannot accept more than @count items.'));
    

    I know this error message is not ever supposed to appear to the user, but I'm not certain we can count on that. Therefore, I'm not sure about the use of the phrase "This field", since we can't be entirely sure where the message may be printed. How about injecting the field label into the message?

25. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    static::setFieldState($element, $form_state, $field_state);
    

    We probably only need to update the field state if $media is not empty.

26. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +      $media = Media::loadMultiple($ids);
    

    Let's just return this directly. Then we can remove the $media = [] line, and return an empty array as the final line of the method.

27. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    $field_name = $element['#field_name'];
    +    $parents = $element['#field_parents'];
    +    $widget_state = static::getWidgetState($parents, $field_name, $form_state);
    

    We don't need to keep $field_name and $parents in their own variables.

28. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +   * @param array $field_state
    +   *   The field state to set.
    

    Can we get a little explanation about what is expected to be in $field_state?

29. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,503 @@
    +    $field_name = $element['#field_name'];
    +    $parents = $element['#field_parents'];
    

    Same here.

What are the concerns around just using the "view media" permission? This seems consistent with how nodes, for example, are handled (anon users see all posts that are listed on the front page view, for example). As long as the media library view respects any access control modules that might be in play, I'm not sure why this would be a problem?

There are no immediate concerns, so let's not bother with anything more advanced than that. I suggest we open a follow-up to investigate a more robust access check. Depending on how much concern there is about that, it could potentially be a stable blocker, but by no means should it block progress in this issue and getting something usable into 8.6. So, full steam ahead!

Some minor points here,

1. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +        }
   

   Missing a trailing comma.

2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +        hide: Drupal.t('Hide media item weights')
   

   Missing a trailing comma.

3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +            .parent()
   +            .find('.media-library-item__weight')
   +            .parent()
   

   Could we use .closest()?

4. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +            if ($checkboxes.filter(':checked').length === settings.media_library.selection_remaining) {
   +              $checkboxes.filter(':not(:checked)')
   +                .prop('disabled', true)
   +                .closest('.media-library-item')
   +                .addClass('disabled');
   

   Lets be consistent and put each method on a new line.

5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +              $checkboxes.prop('disabled', false)
   +                .closest('.media-library-item')
   +                .removeClass('disabled');
   

   Lets be consistent and put each method on a new line.

For #55.1 🤦‍♀️

Well, folks, after this review I'm pretty much out of things to nitpick.

1. +++ b/core/modules/media/media.module
   @@ -336,3 +318,35 @@ function media_preprocess_media_reference_help(&$variables) {
   + * @internal
   + */
   +function _media_get_add_url($allowed_bundles) {
   

   Under the @internal, should we explicitly mention that this function may be removed in a minor release?

2. +++ b/core/modules/media/media.module
   @@ -336,3 +318,35 @@ function media_preprocess_media_reference_help(&$variables) {
   +  $create_bundles = array_filter($allowed_bundles, function ($bundle) use ($access_handler) {
   +    return $access_handler->createAccess($bundle);
   +  });
   

   We can condense this to one line: $create_bundles = array_filter($allowed_bundles, [$access_handler, 'createAccess']).

3. +++ b/core/modules/media/media.module
   @@ -336,3 +318,35 @@ function media_preprocess_media_reference_help(&$variables) {
   +  // Add a section about how to create media if the user has access to do so.
   +  if (!empty($create_bundles)) {
   +    if (count($create_bundles) === 1) {
   +      return Url::fromRoute('entity.media.add_form', ['media_type' => reset($create_bundles)])->toString();
   +    }
   +    elseif (count($create_bundles) > 1) {
   +      return Url::fromRoute('entity.media.add_page')->toString();
   +    }
   +  }
   

   We can remove the if (!empty($create_bundles)) check, as it doesn't really add anything to this logic.

4. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   @@ -445,3 +404,130 @@ display:
   +  widget:
   +    display_plugin: page
   

   Let's open a follow-up, and add a todo here, to look into not exposing this display as a page (possibly with a custom display plugin).

5. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +            $(child).find('.media-library-item__weight').val(index);
   

   Can the selector be input.media-library-item__weight, just to be sure we're targeting the right kind of thing?

6. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +            .parent()
   +            .find('.media-library-item__weight')
   +            .parent()
   

   Same here.

7. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +      $('.media-library-item__weight', context).once('media-library-toggle').parent().hide();
   

   And here.

8. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,91 @@
   +              $checkboxes.filter(':not(:checked)')
   

   This can be $checkboxes.not(':checked').

9. +++ b/core/modules/media_library/media_library.module
   @@ -43,6 +46,23 @@ function media_library_theme() {
   +      if (!empty($query)) {
   +        $output['#attached']['drupalSettings']['views']['ajax_path'] .= '?' . UrlHelper::buildQuery($query);
   +        if (isset($query['media_library_remaining'])) {
   +          $output['#attached']['drupalSettings']['media_library']['selection_remaining'] = (int) $query['media_library_remaining'];
   +        }
   +      }
   

   I wonder if we should open a follow-up against Views to allow each view to have its own AJAX path. Might be more trouble than it's worth, but at least we can raise the issue.

10. +++ b/core/modules/media_library/media_library.module
    @@ -93,6 +111,82 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
    +    $form['header']['#attributes']['class'][] = 'media-library-views-form__header';
    +    $form['header']['media_bulk_form']['#attributes']['class'] = 'media-library-views-form__bulk_form';
    

    Let's wrap this in an isset($form['header']) check.

11. +++ b/core/modules/media_library/media_library.module
    @@ -93,6 +111,82 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
    +function media_library_form_views_exposed_form_alter(array &$form, FormStateInterface $form_state) {
    +  if (isset($form['#id']) && $form['#id'] === 'views-exposed-form-media-library-widget') {
    +    $types = _media_library_get_allowed_types();
    +    if ($types && isset($form['type']['#options'])) {
    +      $keys = array_flip($types);
    +      // Ensure that the default value (by default "All") persists.
    +      if (isset($form['type']['#default_value'])) {
    +        $keys[$form['type']['#default_value']] = TRUE;
    +      }
    +      $form['type']['#options'] = array_intersect_key($form['type']['#options'], $keys);
    +    }
    +  }
    +}
    

    Let's open a follow-up to implement a custom Views filter plugin which can do this in a nicer way (i.e., expose an API to limit exposed filters in a contextual way).

12. +++ b/core/modules/media_library/media_library.module
    @@ -93,6 +111,82 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
    +  if (!is_a($class, 'Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem', TRUE)) {
    

    Can we use the ::class format for EntityReferenceItem?

13. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    + *   multiple_values = TRUE
    

    Nit: Should have a trailing comma.

14. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +    $field_name = $this->fieldDefinition->getName();
    +    $parents = $form['#parents'];
    

    Nit: We can pass these directly to getWidgetState(), we don't need to keep them in their own local variables.

15. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +    if ($field_state && isset($field_state['items'])) {
    

    We can lose the first condition; isset() will check it implicitly.

16. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +    $view_builder = $this->entityTypeManager->getViewBuilder($this->getFieldSetting('target_type'));
    

    We don't need to call getFieldSetting(); this widget is hard-coded to work only with media items.

17. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +        '#value' => t('Create new media'),
    

    Can we use $this->t()?

18. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +        '#value' => t('Create your media on the <a href=":add_page" target="_blank">media add page</a> (opens a new window), then select it in the library.', [':add_page' => $add_url]),
    

    Same here.

19. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +      return Media::loadMultiple($ids);
    

    We should probably only return this if $ids is not empty, now that I think of it.

20. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +   * @return array
    

    Should be array[], since it's an array of arrays.

21. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,514 @@
    +   * @param array $field_state
    

    Same here.

22. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,140 @@
    +        'query' => \Drupal::request()->query->all() + [
    +          FormBuilderInterface::AJAX_FORM_REQUEST => TRUE,
    +        ],
    

    This logic could allow external forces to inject the value of the FormBuilderInterface::AJAX_FORM_REQUEST query parameter. I doubt it's a security hazard, but let's overwrite the value fully instead.

23. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,140 @@
    +    // Pass the selection to the field widget based on the current widget ID.
    +    $response->addCommand(new InvokeCommand("[data-media-library-widget-value=\"$widget_id\"]", 'val', [implode(',', $mids)]));
    +    $response->addCommand(new InvokeCommand("[data-media-library-widget-update=\"$widget_id\"]", 'trigger', ['mousedown']));
    +    $response->addCommand(new CloseDialogCommand());
    +    return $response;
    

    Nit: We can make this a beautiful chain: return (new AjaxResponse())->addCommand()->addCommand()->addCommand().

24. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -92,11 +101,158 @@ public function testAdministrationPage() {
    +    $session->executeScript('jQuery(\'button:contains("Apply Filters")\').click()');
    

    Can we do this without executing JavaScript? Won't $assert_session->buttonExists('Apply Filters')->press() do the same thing?

25. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -92,11 +101,158 @@ public function testAdministrationPage() {
    +    $session->executeScript("jQuery('$checkbox_selector:eq(0)').click()");
    +    $session->executeScript("jQuery('$checkbox_selector:eq(1)').click()");
    +    $session->executeScript("jQuery('$checkbox_selector:eq(2)').click()");
    

    Same here -- any way we can accomplish this without JavaScript?

26. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -92,11 +101,158 @@ public function testAdministrationPage() {
    +   * Tests that the Media library's widget works as expected for anonymous.
    

    Nit: "...for anonymous users".

I'd like to get official sign-off from the UX team in this issue, as well as official accessibility sign-off. Follow-ups should be opened for anything stable-blocking. Otherwise, I'm pretty close to being ready to RTBC this sucker.

Responding to #30.4

4. I don't think we should allow selecting all items in the context of a widget. You would have to have filtered the view to be seeing exactly what you want to select, which is extremely rare (I've never been in that situation).

While testing this patch I also had the feeling that the "select all" would be nice. For example, if you extend the view with filtering for tags, then selecting all items could be useful. So I think it doesn't hurt if we display the checkbox.

Could we get some default height for the library? It looks a bit strange if it's empty.

While testing this patch I also had the feeling that the "select all" would be nice. For example, if you extend the view with filtering for tags, then selecting all items could be useful. So I think it doesn't hurt if we display the checkbox.

I think this is a decision that should be deferred to the UX team.

I consider the code for this widget RTBC. Now we just need review and sign-off from everyone mentioned in the tags. I'll start pinging folks.

Just started testing this. First off, this is looking really nice! Thanks samuel.mortenson. I only have some small usability related comments:

1. The button in the widget currently says 'Add Media'. I think this is confusing since the button on /admin/content/media also says 'Add media' but does something different. Maybe we should use 'Select media' to avoid confusion? Also, the word 'media' should be lowercase here.

   

2. When opening the modal the 'Apply filters' button is not shown near the actual filter form. Can we show this next to the form? This was already mentioned in #5 but I can't find if there is a good reason not to do this.

   

3. In the modal, the media library shows edit links for media below the thumbnail. I don't think we currently have a good reason to show these links. Even though there is an alert to warn you, when you leave the page you lose all the information. This is probably going to frustrate users.

   

4. When media is selected, the title of the media item again links to the edit page. I think removing the link is best for now. We could find a solution to edit media in a follow-up (like showing an edit button to load the media form in a modal).

   

5. Last thing I noticed is we are not showing the media type in the library. This is probably a separate issue, but it could be hard to see the difference between a remote video and an image at first glance. They both show an image thumbnail. This might be an issue when users enable multiple types for a field (like for a slideshow or something).

About #1 in above comment, how about using 'Browse media' or 'Browse Media'?

About #2 -- Apply Filters, how about adding a button in line of actual filter that reads 'Apply'?
About #3 -- Let there be only media, nothing else, no links to edit or actual media file
About #4 -- Remove the link
About #5 -- For videos, how about displaying a 'play' icon somewhere on the image, without allowing it to play actually?

1. Maybe not, but if you are able to add media we can change the name of the button back later. The widget would change a bit anyway when we add that functionality? Might be good to also ask the UX team about this.
2. Having a follow up to fix the core functionality seems to make sense. I don't think this is a blocker.
3. I will ask the UX team what they think about removing the link.
4. Same as 3.
5. Let's have a follow up to talk about this separately.

We just talked about this issue in the UX meeting. We decided on the following regarding my feedback in #70:

1. The button should say 'Browse media' for now. When we add the possibility to create new media, we can change the button again. The widget would change a bit anyway.
2. This is not a blocker for this issue. We should make it a follow up with a high priority.
3. Since the link could lead to data loss, we should remove it in this issue. We do recognize information and options are missing (view media link, edit link and the media type are the main things). There should be a follow up to improve the grid view.
4. Same as 3.
5. Same as 3.

To sum it up for this patch:

• Change the Add media button to Browse media.
• Remove the edit link from the grid display for the widget and modal.
• Create the follow ups.

I think we can remove some of the issue tags as well, but leaving those for now.

I feel that #70.2 is going to confuse a lot of users. Applying a temporary and quick fix/workaround might be worth considering.

When opening the modal the 'Apply filters' button is not shown near the actual filter form.

The 'Filter' button is being moved into the dialog's buttons via dialog.ajax.js

The attached patch removes the .form-actions class from the views exposed filter form and replaces it with .media-library-view--form-actions class. This patch fixes the immediate issue with the media library but we should open up a dedicated ticket to figure out a better long-term solution.

Reviewed the JS and the CSS. I would like to review the ajax code before removing the tag which I didn't have a chance to do yet.

1. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -51,6 +48,25 @@
   +.media-library-item.disabled {
   
   +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -135,6 +143,68 @@
   +.media-library-item.disabled {
   

   Should we just use .media-library-item--disabled since this is not proper usage of SMACSS state classes?

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -135,6 +143,68 @@
   +.media-library-toggle-weight {
   

   Should we rename this to .media-library-widget__toggle-weight since this actually depends being child of .media-library-widget?

3. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +      $('.media-library-selection', context).once('media-library-sortable').sortable({
   ...
   +        handle: '.media-library-item__preview',
   ...
   +            $(child).find('input.media-library-item__weight').val(index);
   ...
   +      $('.media-library-toggle-weight', context).once('media-library-toggle')
   ...
   +            .find('input.media-library-item__weight')
   ...
   +      $('input.media-library-item__weight', context).once('media-library-toggle').parent().hide();
   ...
   +      $('.media-library-item a[href]', context).once('media-library-warn-link')
   ...
   +      const $view = $('.media-library-view', context).once('media-library-remaining');
   +      $view.find('.media-library-item input[type="checkbox"]')
   ...
   +            const $checkboxes = $view.find('.media-library-item input[type="checkbox"]');
   

   We should use .js- prefixed class names when attaching JS to HTML elements to make sure that the frontend developers are aware of this. While doing this, we should also remove the HTML elements from the selectors.

Thanks @samuel.mortenson for pointing out the conflicting feedback from @phenaproxima on #58.5. Generally we don't add the element type on the selectors because it is hard to predict selectors like that when making changes to markup. For the same reason we use .js- prefixed classes for targeting elements so that it's clear there's some logic attached to an element, and that it has to be taken into account when making modifications to the markup.

1. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +        handle: '.media-library-item__preview',
   

   This one is still missing a .js prefix.

2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -0,0 +1,93 @@
   +            $(child).find('.js-media-library-item__weight').val(index);
   ...
   +      $('.js-media-library-widget__toggle-weight', context).once('media-library-toggle')
   ...
   +            .find('.js-media-library-item__weight')
   ...
   +      $('.js-media-library-item__weight', context).once('media-library-toggle').parent().hide();
   

   The js- prefixed classes doesn't have to follow BEM because generally there shouldn't be CSS attached to them.

We are 24 hours from 8.6.0 alpha. It is the 11th hour. And it's time for this to go into Drupal. I am removing the remaining accessibility and usability review tags.

Why?

• Code-wise, this patch has been ready for a fair two weeks now. I base this on the fact that I have read it through several times, understand it thoroughly, and can no longer find anything to complain about.
• Our only hard blocker, #2861860: View inside core modal doesn't refresh correctly when reopened, has been committed.
• This patch has been demoed to the UX team (including such luminaries as @ckrina, @webchick, and @Gábor Hojtsy) repeatedly in the last several UX calls...and no serious problems have been identified. Anything that was identified as commit-blocking has been addressed in this patch, and everything else has been moved to into follow-up issues (some of which must be fixed before the media library module can be marked stable).
• We have tried repeatedly to get accessibility review on this patch, but have not been able to get any time from the accessibility team. In the last UX call, @webchick felt (and I agree) that, at this point, we have done our due diligence and any accessibility issues that do turn up will be turned up as bugs, which can be stable blockers if needed.
• @samuel.mortenson has made tremendous progress on #2938116: Allow media to be uploaded with the Media Library field widget, which is the only thing needed to take the media library from "useful" (once this patch is committed) to "truly freaking awesome". It is blocked only on this issue.
• We have completed reviews from a front-end framework manager and a JavaScript maintainer.
• This is an experimental module which exposes no APIs, and this patch contains the absolute minimum functionality we must have in Drupal in order for the media library not to get reverted. If the media library is removed from core until 8.7, it will be a tragedy for the Media Initiative, which has made incredible progress in this release cycle. We are more than happy to deal with any stable blockers that arise once this is in.

Speaking as a Media subsystem maintainer: it's magic time. Let's do this. RTBC once green on all backends.

We are now in pre-alpha commit freeze, so I think it's unlikely that this will be part of 8.6 alpha, however, according to https://www.drupal.org/core/d8-allowed-changes#alpha, exceptions are still possible after alpha is tagged:

There are no specific restrictions on what changes are allowed in alpha releases beyond the allowed changes for minors, but additions and disruptive changes are backported to the alpha only at committer discretion, with criticals, strategic initiatives, and product management priorities as the first focus.

I would be surprised if this weren't considered a priority.

Uh-oh...looks like we have a failure on Postgres. Kicking back for that.

Nice detective work! Thanks, @samuel.mortenson. Restoring RTBC once green on all backends.

D'oh!

Ah, doesn't actually need a reroll; just need to change all testing to 8.7.x since the Media Library module has been (temporarily) removed from 8.6.x.

Restoring RTBC once all backends are green on 8.7.x.

Overall this is looking very good and a much needed improvement, great work!

I found a few issues while reviewing and manually testing this, however I don't think any of these needs to be a blocker, as this is experimental code and the functionality is complete and working very well for the common use cases, so it should serve well for evaluation and beta testing.

Testing issues:

• I got a notice while adding a media reference field (see attached screenshot).
• I was not able to make the widget work without Javascript: after submitting the media library form, I wasn't brought back to the parent node form.

Code review:

1. +++ b/core/modules/media_library/media_library.module
   @@ -43,6 +47,24 @@ function media_library_theme() {
   +        $output['#attached']['drupalSettings']['views']['ajax_path'] .= '?' . UrlHelper::buildQuery($query);
   

   Are we sure this URL will never contain a query string already? I guess it would be safer to parse it and add the ML parameters rather than just appending them.

2. +++ b/core/modules/media_library/media_library.module
   @@ -93,6 +113,112 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   +      $form['header']['media_bulk_form']['#attributes']['class'] = 'media-library-views-form__bulk_form';
   

   Isn't this is missing a [] operator?

3. +++ b/core/modules/media_library/media_library.module
   @@ -93,6 +113,112 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   +  // Add after build to fix media library views exposed filter's
   +  // 'Apply filter' submit button.
   ...
   +  // This prevents the views exposed filter's 'Apply filter' submit button
   +  // from being moved into the dialog's buttons.
   ...
   + * Alters the widget view's query to only show media that can be selected,
   + * based on what types are allowed in the field settings.
   

   These comments do not wrap at column 80.

4. +++ b/core/modules/media_library/media_library.module
   @@ -93,6 +113,112 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   +    $entity_type = \Drupal::entityTypeManager()->getDefinition('media');
   

   Can we move this line inside the if branch? It's used only there.

5. +++ b/core/modules/media_library/media_library.module
   @@ -93,6 +113,112 @@ function media_library_form_views_form_media_library_page_alter(array &$form, Fo
   +      $query->where[] = [
   

   Can we use $query->addWhere()? It should make the code simpler and easier to read.

6. +++ b/core/modules/media_library/media_library.module
   @@ -107,3 +233,17 @@ function media_library_local_tasks_alter(&$local_tasks) {
   +  $types = \Drupal::request()->query->get('media_library_allowed_types');
   

   This is the main issue that caught my eye while reviewing the code: we are filtering the view via user input, but this can be easily altered to produce unwanted results: I confirmed this is the case while testing with JS disabled. It would feel way less fragile if we passed the field name and retrieved the allowed bundles from that.

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,524 @@
   + * @internal
   

   I think it's ok for now to mark this as @internal, but in the future it would be nice to allow contrib to subclass the widget.

8. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,524 @@
   +        '#type' => 'html_tag',
   

   Isn't there any reusable code for the weight toggle?

9. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -0,0 +1,524 @@
   +      throw new \LogicException('The element that triggered the widget update was at an unexpected depth.');
   

   The exception message could contain the actual/wrong data to make the issue easier to troubleshoot.

10. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,524 @@
    +      $form_state->setError($element, \Drupal::translation()->formatPlural($element['#cardinality'], 'Only one item can be selected.', 'Only @count items can be selected.'));
    ...
    +    $all_bundles = \Drupal::service('entity_type.bundle.info')->getBundleInfo('media');
    

    Can we inject these services?

11. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,524 @@
    +        if ($media_item->access('view')) {
    

    At first sight I was a bit surprised we are checking "view" access, OTOH we don't have a permission for selection AFAIK and I guess view is the closest operation I can think of, so this should be ok.

12. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,139 @@
    +        '#return_value' => 'media:' . $entity->id(),
    

    It would be nice to add a comment explaining why we need the media: prefix, given that we are discarding it later.

13. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,139 @@
    +    $widget_id = \Drupal::request()->query->get('media_library_widget_id');
    

    Can we inject this?

14. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,139 @@
    +      throw new BadRequestHttpException('The "media_library_widget_id" query parameter is required and must be a string.');
    

    Same as above: the exception message could contain the actual/wrong data to make the issue easier to troubleshoot.

15. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -0,0 +1,139 @@
    +  protected function emptySelectedMessage() {
    

    Why do we need a dedicated method for this?

Looks good, thanks!
(sorry for missing the static methods :)

Saving credits

Committed de52834 and pushed to 8.7.x. Awesome work, guys!

I (auto)fixed a few issues stylelint was complaining about on commit:

core/modules/media_library/css/media_library.theme.css
 147:12  ✖  Expected a leading zero   number-leading-zero
 165:11  ✖  Expected a leading zero   number-leading-zero
 190:15  ✖  Expected a leading zero   number-leading-zero

Here's the diff:

diff --git a/core/modules/media_library/css/media_library.theme.css b/core/modules/media_library/css/media_library.theme.css
index 004f37c4d3..5aab3c5bbd 100644
--- a/core/modules/media_library/css/media_library.theme.css
+++ b/core/modules/media_library/css/media_library.theme.css
@@ -144,7 +144,7 @@
 }
 
 .media-library-item--disabled {
-  opacity: .5;
+  opacity: 0.5;
 }
 
 .media-library-selection {
@@ -162,7 +162,7 @@
 }
 
 .media-library-item .form-item {
-  margin: .75em;
+  margin: 0.75em;
 }
 
 .media-library-item__remove,
@@ -187,7 +187,7 @@
   border-radius: 20px;
   color: transparent;
   text-shadow: none;
-  transition: .2s border-color;
+  transition: 0.2s border-color;
 }
 
 .media-library-item__remove:hover,

Discussed this with @webchick, @catch and @Gábor Hojtsy: we are fine with backporting this to 8.6.x, since it's experimental code, involves no data changes, and this commit will allow to mark "media_library" as beta.

Given the huge value this would provide, we hope the Media team will be able to have #2938116: Allow media to be uploaded with the Media Library field widget RTBC before beta. Also that change would still be considered allowed per our policy.

And this is backported now!

Untagging for release notes since this doesn't have upgrade path implications and fits more into highlights.