Add the Media Library module to Drupal core

I think the most important things would be:
- Empty text
- Bulk operations visible from the beginning and solve it in another issue.
- Agree on that the right chevron is not the appropriate pattern. Bottom/top chevron to follow admin toolbar pattern would be the ideal so we reuse patterns, but also html detail default visualization or +/- symbols.
- Accessibility: navigation&selection with keyboard. Is there any standard pattern?
- Editing & deleting media imho is really important. What about showing the "edit" and "delete" icons only when you're over an item? Like this Wordpress example. If we add the "check" on the right and the "edit"+"delete" icons on the left (or the opposite) I think it might be enough. Update: Just realized that we have bulk operations and the checkbox, so the actions can actually be done anyway.

- Expandable details upwards can be dangerous because it is really easy to have long names exceeding the height.
- I really liked the idea of having the selection pattern reusable!

So the needed designs might be:
- Bottom/top chevron
- Edit&delete buttons
- Checkbox (empty circle to indicate it's clickable)
- How to handle large amount of metadata: modal or new page?

Tagging for design changes per #5.

From #3:

We might want to allow users to toggle between the media library grid, and the old table-based list. This is definitely a follow-up, though.

Let's open a follow-up issue for this request.

Responding to #6:

So the needed designs might be...

Don't forget the ability to distinguish between published and unpublished media items. :)

How to handle large amount of metadata: modal or new page?

Would a tooltip be appropriate or useful for this?

Nice work! I think we will want accessibility review for that.

We're making excellent progress. Great teamwork from everybody! I'm really looking forward to landing this.

I agree that we're mostly blocked on design, but true to form I was still able to raise questions and find some things we can spruce up. :)

1. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   @@ -0,0 +1,418 @@
   +        description: 'Allows users to administer Media'
   

   Let's expand this a bit. How about "Allows users to browse and administer media assets."?

2. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,55 @@
   +  Drupal.behaviors.MediaLibraryCheckboxes = {
   

   Let's extract this into its own separate library and JavaScript file, so we can easily merge it into Views later (or at least let developers reuse it for things that aren't the media library). To keep things consistent, let's rename the behavior to something like viewsClickToSelect.

3. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,55 @@
   +          const $input = $(this).closest('.click-to-select').find('.click-to-select-checkbox input');
   

   Is it possible/easy to put the click-to-select-checkbox class directly on the checkbox, rather than having Yet Another Wrapper Element?

4. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,55 @@
   +      $('.media-library-item-attributes', context).after('<span class="media-library-item-toggle"></span>');
   

   Let's use $(context).find() here. I wonder if a <button> element would be more semantically correct here. Also, should the metadata toggling element have a title attribute, or some sort of text (suppressed with CSS) to indicate to screen readers etc. that it reveals metadata?

5. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,55 @@
   +      $('.media-library-item .media-library-item-preview', context).once('media-library-item').on('mouseover focus', function () {
   +        $(this).closest('.media-library-item').addClass('is-hover');
   +      }).on('mouseout blur', function () {
   +        $(this).closest('.media-library-item').removeClass('is-hover');
   +      });
   

   Let's use $(context).find() here too. Also, for readability, can we put the chained .once() and .on() calls onto their own lines?

6. +++ b/core/modules/media_library/media_library.install
   @@ -0,0 +1,35 @@
   +  // Disable the /admin/content view provided by media, so that we can properly
   

   The path should be /admin/content/media. :)

7. +++ b/core/modules/media_library/media_library.install
   @@ -0,0 +1,35 @@
   +    $display = &$view->getDisplay('media_page_list');
   

   It's possible that a site builder will have deleted the media_page_list display. We should probably check for that.

8. +++ b/core/modules/media_library/media_library.install
   @@ -0,0 +1,35 @@
   +  // Restore the /admin/content view provided by media.
   

   Should be /admin/content/media.

9. +++ b/core/modules/media_library/media_library.install
   @@ -0,0 +1,35 @@
   +    $display = &$view->getDisplay('media_page_list');
   

   Let's also sanity check for the presence of the media_page_list view here.

10. +++ b/core/modules/media_library/media_library.module
    @@ -0,0 +1,94 @@
    + * @todo Write online documentation for the Media library and update the link.
    

    We probably don't need to do this in order to land an experimental module, but we should open a follow-up for this and link to it here.

11. +++ b/core/modules/media_library/media_library.module
    @@ -0,0 +1,94 @@
    +      $output .= '<p>' . t('The Media library module overrides the /admin/content/media to provide a rich visual interface for performing administrative operations on media. For more information, see the <a href=":media">online documentation for the Media library module</a>.', [':media' => 'https://www.drupal.org/docs/8/core/modules/media']) . '</p>';
    

    Should be "...overrides the /admin/content/media view..."

12. +++ b/core/modules/media_library/media_library.module
    @@ -0,0 +1,94 @@
    +    $variables['preview_attributes'] = new Attribute();
    +    $variables['preview_attributes']->addClass('media-library-item-preview', 'click-to-select-trigger');
    +    $variables['preview_attributes']->setAttribute('tabindex', 0);
    

    Doesn't this mean that all the items will, by default, have tabindex="0"? Could that produce unexpected behavior? I'm not sure there's any way for us to test that, but if there is, we should. (Maybe we could take advantage of the shiny new Nightwatch testing system for this, if you're up for it.)

13. +++ b/core/modules/media_library/media_library.module
    @@ -0,0 +1,94 @@
    +  // Show formatted time differences for edits younger than a month.
    

    Can we make that threshold configurable? Maybe just introduce a hidden config setting for it.

14. +++ b/core/modules/media_library/media_library.module
    @@ -0,0 +1,94 @@
    +    $date = date('m/d/Y - h:i A', $time);
    

    Rather than hard-code the date format, let's ship a DateFormat entity and use that (see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Datetime%...).

15. +++ b/core/modules/media_library/templates/media--media-library.html.twig
    @@ -0,0 +1,50 @@
    +      <div class="media-library-item-name">
    +        <a href="{{ url }}" rel="bookmark">{{ name }}</a>
    +      </div>
    +      <div class="media-library-item-created">{{ created_ago }}</div>
    +      <div class="media-library-item-type">{{ media.bundle()|title }}</div>
    

    It feels a little clumsy to keep repeating the 'media-library-item' prefix. Could we just add the media-library-item class to the <article> tag, then use classes like 'created' and 'type' for the created time and media type? And could we use a semantic <time> element for the created time? I'm not sure if these ideas constitute best front-end practice, though, so maybe a front-end dev should sign off on these first. Not a big deal either way. (Just 'cause we *could* doesn't necessarily mean we *should*, after all!)

16. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -0,0 +1,98 @@
    +    $this->submitForm([], 'Apply to selected items');
    +    $assert_session->pageTextContains('media_1');
    +    $assert_session->pageTextNotContains('media_2');
    

    I assume this is testing that bulk actions work, by deleting one of the created media items. Just to be clear about that, let's explicitly select the delete action before clicking "Apply to selected items".

Looking at item 15 in comment #15, I'd keep the media-library-item prefix on all the items. If we're following BEM or a BEM-ish convention, we'd want to name the classes more like media-library-item__name, media-library-item__created, and so forth. Even if Drupal 8 doesn't follow BEM conventions for naming CSS classes, it looks cleaner to target a single class rather than start making longer selectors which increase specificity.

This code is clean. I can't find anything else to nitpick. So, in order, here's what we still need to do in order to land the patch:

1. We need design feedback from the UX team, and we need to implement that. I think we can get some expert assistance with the styling if needed.
2. After that, we'll need CSS review, then and official sign-off from the UX and accessibility teams.
3. It'd be nice to get sign-off from a JavaScript maintainer.
4. We need to open a follow-up to move the "pretty time" functionality into Twig. Done: #2964790: Move formatting code in TimestampAgoFormatter into DateFormatter
5. We also need to open a follow-up to move "click to select" functionality into Views.
6. We need to post screenshots (possibly a screencast) and have a sorcerer of breaking things (I'm thinking @webchick) test it.
7. Buy a six-pack of your favorite beer and wait for this to be committed.

Here's a proposal for the design trying to adapt it to Seven styles, based on proposals from @yoroy, @dennis-cohn and @rfmarcelino in the #ux Slack channel:
- Example with audio, docs, images and video.
- Example with non-squared images.
- Example of hover&selected.
- Example of unpublished items.

Some accessibility concerns mentioned in the #ux slack channel by @andrewmacpherson and @benjifisher addressed here:
- Checkbox always visible.
- No caps.
- No important actions reserved for only hover.

Since we can use flexbox to have the same the height on items in the same line, having multiple lines per title makes sense. Anyway, I would try to limit the title's height/length to 4 lines or so with CSS so the grid keeps being readable.

I applied the patch locally and really enjoyed seeing it!

Here is some quick review.

1. +++ b/core/modules/media_library/config/install/core.date_format.media_library.yml
   @@ -0,0 +1,10 @@
   +pattern: 'm/d/Y - h:i A'
   

   I just compared this with our existing 'short' date format: 'm/d/Y - H:i' Is there a good reason to switch to AM/PM in the media library?

2. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +      $('.click-to-select .click-to-select-trigger', context).once('click-to-select').on('click', function (event) {
   

   Note: There is quite a bit of code in Drupal which switched from classes to data attributes to do this kind of feature flagging. This way there are less problems when changing the markup.

@samuel.mortenson just demoed this patch in the UX meeting. @andrewmcpherson was there as well, and was able to offer accessibility input. Therefore, I'm removing the tag.

Summary of the feedback from that call:

• This is looking excellent and everyone is very pleased with the progress being made!
• The bulk selection checkboxes have a label which simply says "Update this item". This poses an accessibility problem because each item is wrapped by a div tag, which is not a semantic group, and therefore it's not clear to assistive technology what "this item" means. A couple of options were discussed, and the preferred solution is to change the label, which is visually hidden anyway, to "Update '$title_of_item'". Ideally, this pattern would be used for all views which use bulk selection, but that is a much bigger issue and very much out of the Media Initiative's scope. We've opened a follow-up to discuss fixing this everywhere: #2969660: Improve accessibility of bulk selection forms
• There is a "sort by" filter on the media library, and the default option is "Newest first", but the actual items don't have any date or time-related information (as per #26). It's not clear if, and how, we should add it back. Or if we should talk about that/do it in a follow-up.
• It would greatly improve the visual and user experience if the "Apply filters" and "Apply to selected items" buttons were displayed inline.
• This is a problem we've inherited from Views, and therefore not in scope to fix here, but it's jarring that "Delete media" is the first bulk operation that is shown. Action weights should be configurable by users. Is there already an issue open to fix this? If not, we might want to open one.
• If you have Content Moderation installed, and you're using it on media items, the published/unpublished status is next to useless. There's only so much we can do here in the short term (we can certainly make the relevant CSS styles easier to repurpose), but this may need more thought and work, for which I have opened #2969678: Integrate Media Library with Content Moderation

Yep to all of that. Here's the video for anyone who wants to see this patch in action and learn some cool things about accessibility! :) https://www.youtube.com/watch?v=L1Zh6YiQPIU

The only correction is I would say sorting by "Newest first" is the default expectation... you certainly would not want to see the same image of your dog you uploaded back in 2008 every single time you loaded this screen, and most likely the things added most recently are those you'd want to reuse. So we might need to keep that selection there so you can revert back to it after playing with the sort filter to be something else and then being all "nah."

I think the main point raised during the call was "should we also include a 'time ago' on the media element somehow so people can see how recent their picture is" and while we could, a) it's hard to cram much more data into that small "descriptive" area without it overtaking the thumbnail, and b) I noticed that Facebook, WordPress, and SquareSpace also have no exposed time ago field despite sorting recent first. (In fact they have no descriptive section at all—purely the visuals, so we're an outlier there...)

Something I just noticed - there's no bulk-operations checkbox for selecting ALL media items. On the table displays there's a select-all checkbox in the table header, but we don't have that on this new display. Is that by-design, or a regression we overlooked?

Only nitpicks, and very few at that. Code is RTBC from me otherwise, although I'd still like CSS review.

1. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +        $input.prop('checked', !$input.prop('checked'));
   +        $input.trigger('change');
   

   Nit: These can be chained, I think. $input.prop('checked', !$input.prop('checked')).trigger('change')

2. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,24 @@
   +      const $preview = $('.media-library-item .media-library-item__preview', context).once('media-library-item');
   +      $preview.on('mouseover focus', function () {
   +        $(this).closest('.media-library-item').addClass('is-hover');
   +      });
   +      $preview.on('mouseout blur', function () {
   +        $(this).closest('.media-library-item').removeClass('is-hover');
   +      });
   

   Nit: We don't need the $preview variable if we chain the calls like .once(...).on('mouseover focus')...on('mouseout blur')

Kicking back until we have answers on #31 and #33.

1. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +(function ($, Drupal) {
   

   This iife should get adjusted,

   (function ($, Drupal) {
     
   }(jQuery, Drupal))
   

2. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +  "use strict";
   

   Two things, use single quotes, and we also don't need use-strict anymore.

3. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +    attach: function (context) {
   

   Use the object-shorthand style for this function.

4. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +      $('.click-to-select .click-to-select-trigger', context).once('click-to-select').on('click', function (event) {
   

   While just a warning, this is an unnamed function. You can use an arrow (preferred), or name it.

   Additionally, it would be a bit nicer to namespace these `once` events a bit more.

5. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +      $('.click-to-select .click-to-select-checkbox input', context).once('click-to-select').on('change', function (event) {
   

   While just a warning, this is an unnamed function. You can use an arrow (preferred), or name it.

6. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,28 @@
   +    }
   

   Missing trailing comma.

7. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,24 @@
   +(function ($, Drupal) {
   

   This iife should get adjusted,

   (function ($, Drupal) {
     
   }(jQuery, Drupal))
   

8. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,24 @@
   +  "use strict";
   

   Two things, use single quotes, and we also don't need use-strict anymore.

9. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,24 @@
   +    attach: function (context) {
   

   Use the object-shorthand style for this function.

10. +++ b/core/modules/media_library/js/media_library.view.es6.js
    @@ -0,0 +1,24 @@
    +      $preview.on('mouseover focus', function () {
    

    While just a warning, this is an unnamed function. You can use an arrow (preferred), or name it.

11. +++ b/core/modules/media_library/js/media_library.view.es6.js
    @@ -0,0 +1,24 @@
    +      $preview.on('mouseout blur', function () {
    

    While just a warning, this is an unnamed function. You can use an arrow (preferred), or name it.

12. +++ b/core/modules/media_library/js/media_library.view.es6.js
    @@ -0,0 +1,24 @@
    +    }
    

    Missing a trailing comma here.

- fixes all my own JS feedback from #35

Review of patch #37

1. There's a layout problem on Edge, Firefox and IE11. The grid of media items doesn't clear to the left after the bulk operations controls. Chrome doesn't have this problem. More cross-browser testing needed. This screenshot is from Edge:
   
2. + $form['media_bulk_form'][$key]['#title'] = t('Update media "@label"', [
   Remove the quote marks from the text here, because some screen readers might pronounce them. Narrator and NVDA were fine, but ChromeVox said "Update media quote red parrot quote", which is pretty ugly. If we think some kind of punctuation is necessary here, I'd go for a colon, so reads like "Update media colon Red parrot". Personally though, I don't think any punctuation is necessary here at all (in English).
   We could wrap the entity label in emphasis tags, like we do with on the permissions page for the various content type permissions, but screen readers don't generally treat this as any different from normal text, and this is an invisible label so the typography doesn't matter.
3. + $form['media_bulk_form'][$key]['#title'] = t('Update media "@label"', [
   We can probably loose the word "media" too (to reduce verbosity) because media is the whole purpose of this page, and every item in the view is a media entity.
4. The select-all checkbox is worded "select..." but the checkboxes for individual items are worded "Update...". We have this discrepancy on the node and user listings too, but I never noticed it until now. I have a feeling this might be confusing, because checking the box doesn't actually cause an update to happen - you're just picking some items for now, and nothing gets updated until the bulk-ops button is pressed. Not sure whether to worry about this or not.

5. +.media-library-item__name a {
   +  text-decoration: underline;
   +  border: 2px solid transparent;
   +}
   +
   +.media-library-item__name a:focus {
   +  border-color: #40b6ff;
   +}
   

   This has an interesting severe side effect - it breaks when a Windows' high-contrast theme is being used. Beware of toggling colour to/from transparent to indicate state. In Windows' high-contrast mode, border width and style are respected, but border colour is overridden. CSS "transparent" is treated as a colour in this regard, so it results in ALL of the title links getting a visible border, whether they have focus or not.
   

   I'd say this approach of toggling a border between transparent and blue fails WCAG 2.0 Use of color. The solution is to toggle the presence of the border, rather than the colour (i.e. toggle between 0px and 2px width). If this messes with the box model, you can use the CSS outline-* properties instead. An outline-offset: 0.15em is nice for readability.

   (Aside: core themes have lots of other problems in Windows high-contrast mode. I want to fix them in other issues. But we can avoid introducing new problems here.)

6. +.media-library-item__name a:focus {
   +  border-color: #40b6ff;
   +}
   

   The blue is too pale. It has a contrast of 2.44 against the white background. There's a new WCAG 2.1 success criterion, 1.4.11 Non-text Contrast, which means we want a focus outline with 3:1 contrast. The easy solution would be to make the focus outline the same colour as the link text.

7. The border used for focus also gets broken when it wraps to two lines. Treating the title as a block level element would make it neater.
8. There's some kind of overflow problem with the focus borders, and the containing div. The top and left borders don't appear.

Leaving at needs review, I haven't looked at anything else in the patch.

Something I mentioned in the UX meeting, but it hasn't been written down here yet...

The entity operations (dropbutton) links are have the same problem as the bulk operations checkbox - users need to know which media item they apply to. Just like bulk-ops checkbox, the dropbuttons currently get context from the HTML table row structure. But in the new grid, the dropbutton is outside of the rendered entity's <article> wrapper.

There's already an issue to address this in EntityListBuilder, but the dropbuttons in the new media library actually come from the EntityOperations views field plugin.

Between the bulk-ops checkbox, dropbutton links, and the visible title link, repeating the entity label stacks up to a lot of verbosity for screen readers. I'll have a think about this, and look around for other patterns, but for now I think we'll have to include the entity label in the operations links.

I took a quick look at the CSS and it looks great to me!

Two minor comments:

1. +++ b/core/modules/media_library/css/media_library.css
   @@ -0,0 +1,184 @@
   +  align-content: space-between;
   

   Does this have any effect on the grid of media items? space-between should add an equal amount of spacing between all the items in the grid, and I don't see that being applied. If this isn't needed, this can be removed.

2. +++ b/core/modules/media_library/css/media_library.css
   @@ -0,0 +1,184 @@
   +  margin-right: 0.5em;
   

   I've noticed a mix of em and px for margin and padding values. I'd probably stick with one or the other, though there isn't any particular convention I can find for what units to use within Drupal. So, this is more of a 'keep things consistent' suggestion :)

Accessibility review of patch #44, what's improved since #38

1. The bulk-operations checkboxes for each media item have distinct labels. FIXED. Here's a screenshot of NVDA screen reader's form item's list. We see checkboxes called "Select Rainbow lorrikeet". "Select "Blue and Yellow macaws" as checkbox labels. I use parrot photos for all my test files.
   
2. The windows-high contrast bug with the focus outline is fixed. We're no longer toggling a border from transparent to blue, instead we're swapping a 2px margin for a 2px blue border. The screenshot shows a focus indicator for one media item title only. FIXED, very nice indeed.
   

Continuing accessibility review of patch #44, stuff not fixed since #38...

The Entity Operations drop button has repeated buttons and links which aren't contextually tied to the individual media items. When we had a table view, these dripbuttons were inside the same row TR wrapper, so context could be inferred by exploring the row.

With three media items, we see three indistinguishable "Edit" links. Here's a screenshot of NVDA's list-links tool.

Similarly, there are 3 indistinguishable "list additional actions" buttons, in NVDAs list-buttons tool.

I think we are best of fixing this in EntityOperations itself, providing some flexible display options for the output.

Opened #2973124: Provide better context in accessible names of EntityOperation views plugin drop-button operations to address #39 and #46. And with that, all requested follow-ups have been opened and I believe accessibility review is complete.

Demoed this in the UX call using simplytest.me. There were (only!) three excellent pieces of feedback:

• When thumbnails are broken, as in the attached screenshot, the alt text is just "Thumbnail", which is bad from an accessibility standpoint. Can we fix that? If not, let's open a follow-up to fix it. I'm not sure this is a commit blocker, but think it is a stability blocker.
• When no thumbnail exists for a media item, the generic icon is too big compared to the design in #22 (see the attached screenshot). Can we reduce them in size, in this view only? If not, let's open a follow-up.
• The "select all media" action makes sense as a checkbox when it's in a table, but it doesn't make as much sense in a grid of thumbnails. Although she is on the fence about it, @ckrina suggested making it a button (which is fine from an accessibility standpoint, according to @andrewmcpherson). So we need a touch of design input on this, but for now we should go ahead and make it a button rather than a checkbox, if possible.

Re #48.1, I found #2956368: MediaThumbnailFormatter produces unhelpful text alternative and title attributes for media thumbnails which claims alt="Thumbnail" is by design for MediaThumbnailFormatter. I've reclassed it as a major bug. I guess it could be a stability blocker for Media Library, but the formatter is already in use by the table version of the media listing.

For accessibility, things are going well so far, and we have some follow-ups which are fairly well understood. No concerns from me for committing this as an experimental module.

Yesterday in the ux call we discussed whether to use a button for the “Select all” instead of a select. The main reason is that it’s not a single item to be picked, it's an action. Actually, it’ll override the other selects in the grid, so unless we come up with a better visual way to explain the hierarchy (like happens in the table) a button is a much simpler way to imply it.

But there’s yet another problem to solve once you have all items selected (as @samuel.mortenson said):
- The select all button looses it’s purpose and it’d be confusing to keep it when all items are selected.
- What if you want to “unselect all”?

So I would add a button that toggles between "select all" and "unselect all"(not a copy specialist here :P) as it is clicked/has done its job. The ideal design would be implementing designs from #2721807: Design for filters and bulk operations.

But since it is out of scope from this issue I'd go for something like this:

Sorry, uploaded the wrong file.

Also not a copy specialist but "Unselect all" should probably be "Deselect all".

Or perhaps "Select none"?

+ const $button = $('<a class="button media-library-select-all" aria-pressed="false"></a>')
This isn't keyboard operable. Use a <button>

What's the aria-pressed for? Only use that for a button which behaves like a toggle. Also, it doesn't make sense on a HTML <a> element - links don't toggle.

"Select all" vs "Deselect all" isn't actually a toggle behaviour. They are two separate actions, to override the selections made in the grid of individual items, which could be in a mixed state.

Let's imagine there are 20 items shown. 7 are selected, 13 not selected. In this state, both actions would make sense. So it isn't appropriate to treat the button as a boolean toggle (aria-pressed=true|false).

This is actually more like a tri-state switch, where you cycle though mixed/all/none. Desktop applications use them, but they're not very common on the web. Now aria-pressed and aria-checked both support tri-state widgets - in the ARIA rec, at least. I haven't looked at how well supported that is in real tech. Come to think of it, the select-all checkbox on the table display would benefit from this too. ARIA wasn't around in Drupal 3 or whenever the bulk operations appeared ;-)

"Invert selection" would be a toggle behaviour. Some Linux file managers have that.
MacOS Finder only has "select all", it doesn't have "deselect-all", or "invert selection".

Tagging, I'm a bit concerned about this button.

I've been thinking more about the select-all checkbox, to avoid complicating things...

• Let's not bother with the tri-state button idea for now. I've filed #2974944: Use a tri-state checkbox for tableselect widget. as a nice generic feature we could have eventually.
• Also let's not use aria-pressed=true|false in a boolean way. Unless we fully implement the tri-state behaviour, it could be misleading.
  Edit: by coincidence, I found some relevant advice just a few hours later...
  

  if your actual label text changes when toggling, aria-pressed is likely not necessary (could actually be more confusing for user)

  Source: Patrick Lauke, WAI-ARIA - An introduction to Accessible Rich Internet Applications (slide 104).

• Instead, just update the action text on the button, and convey the outcomes to assistive tech using Drupal.announce() for now. So we'd have announcements which said "All 20 items selected" and "Zero items selected".

With this in mind, that means I prefer a <button> to a checkbox.

1. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,27 @@
   +          $(event.currentTarget).closest('.click-to-select').toggleClass('checked', $(event.currentTarget).prop('checked'));
   

   Personal preference, but this line feels a bit too long. Maybe two or more chained methods we wrap them?

2. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,27 @@
   +        .on('change', (event) => {
   

   We have destructured event elsewhere. I think it makes sense to do it here.

3. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,46 @@
   +        $('.media-library-view .view-content [id*="edit-header"] .form-actions').prepend($button)
   

   Missing a semicolon here.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -0,0 +1,102 @@
   diff --git a/sites/README.txt b/sites/README.txt
   
   diff --git a/sites/README.txt b/sites/README.txt
   old mode 100644
   new mode 100755
   diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
   
   diff --git a/sites/default/default.services.yml b/sites/default/default.services.yml
   old mode 100644
   new mode 100755
   diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
   
   diff --git a/sites/default/default.settings.php b/sites/default/default.settings.php
   old mode 100644
   new mode 100755
   diff --git a/sites/development.services.yml b/sites/development.services.yml
   
   diff --git a/sites/development.services.yml b/sites/development.services.yml
   old mode 100644
   new mode 100755
   diff --git a/sites/example.settings.local.php b/sites/example.settings.local.php
   
   diff --git a/sites/example.settings.local.php b/sites/example.settings.local.php
   old mode 100644
   new mode 100755
   diff --git a/sites/example.sites.php b/sites/example.sites.php
   
   diff --git a/sites/example.sites.php b/sites/example.sites.php
   old mode 100644
   new mode 100755
   

   Are we sure we want to adjust the permissions on these files?

As discussed in the #ux channel, here's the last proposal for the design:

1. Adds new table/grid switch buttons so the content can be adapted to each one needs.
2. Proposes to remove the media type label as @samuel.mortenson mentioned in the previous comment.
3. Proposes to show one line of media name by default, then show the rest on hover. There wasn't an agreement on the last UX, so this decision still needs accessibility review at least.
4. Changes styling of the attribute area.
5. Moves the checkbox to the top right, and the unpublished status to the left.

Patch #67:

+  Drupal.behaviors.MediaLibraryHover = {
+    attach(context) {
+      $('.media-library-item .media-library-item__preview', context).once('media-library-item')
+        .on('mouseover focus', ({ currentTarget }) => {
+          $(currentTarget).closest('.media-library-item').addClass('is-hover');
+        })
+        .on('mouseout blur', ({ currentTarget }) => {
+          $(currentTarget).closest('.media-library-item').removeClass('is-hover');
+        });
+    },
+  };

Focus and blur events will never fire on .media-library-item__preview because it isn't a focusable element - it's just a wrapper DIV. If we want to provide the blue border is-hover style for keyboard users, we'd want something like:

/* Needs a :focus-within polyfill for IE11 and Edge */
.media-library-item:focus-within {}

or

+  Drupal.behaviors.MediaLibraryFocus = {
+    attach(context) {
         // :tabbable selector comes from jQuery UI
+      $('.media-library-item .media-library-item__preview :tabbable', context).once('media-library-item-focus')
+        .on(focus', ({ currentTarget }) => {
+          $(currentTarget).closest('.media-library-item').addClass('has-focus-within');
+        })
+        .on('blur', ({ currentTarget }) => {
+          $(currentTarget).closest('.media-library-item').removeClass('has-focus-within');
+        });
+    },
+  };

More review of patch #67:

I tested the Drupal.announce() messages from the select-all button. They're good.

Re: #66.3

3. Proposes to show one line of media name by default, then show the rest on hover. There wasn't an agreement on the last UX, so this decision still needs accessibility review at least.

It would be good to show the full title to keyboard users as they are tabbing though items. A complication is that the hover area contains TWO focusable items for a keyboard user - the checkbox and the title. When a mouse user sees the whole title on hover, it helps them to decide which checkboxes to select. For a keyboard user to have a comparable experience, they would need to see the full title when the checkbox has focus, not just when the title has focus. This is a bit like a tooltip behaviour.

So we'd want something like:

.media-library-item__preview.is-hover .title,
.media-library-item:focus-witihin .title {
  /* styles to show the full title when focus is on a checkbox OR a title link */
}

.media-libary-item__preview .title:focus
{
  /* Indicate the title has focus.  This needs to be in addition to the full-title display.
}
.media-libary-item__preview [type="checkbox"]:focus
{
  /* Indicate the checkbox has focus
}

I mentioned this in Slack, just saying it again here.

We should remove the image title attributes - they're no use. Only mouse users can perceive them, and it just duplicates visible text nearby:

• On the table view, the thumbnail image column is followed by the title column, which shows the entire title.
• On the grid view, the truncated title is visible, and the full title will be visible on :hover and :focus-within.

I think this might be an issue with the media thumbnail formatter in general, so maybe not in scope here.

Aside: Drupal core makes way too much use of title attributes in all sorts of places where they are useless, so it might be better to address this in a "stop using unncessary title attributes" plan for the whole of core.

The blue is-hover border appears when you hover over a preview image, but it dssappears when you hover over the checkbox itself.

We're just using the user-agent focus styles for the checkboxes. Nothing wrong with that, but we've provided our own focus style for the title link, so it feels a bit half-done to still use the user-agent style for the checkboxes. More consistency would make it easier to follow focus visually.

@samuel.mortenson My point in #68 is that
on(mouseover focus) creates a JS focus event listener which will never fire, so it shouldn"t be there.

The tabbing order itself is correct - checkbox and link are both focusable - and I wasn't proposing to change that.

Rather, I mean that whatever styling is supposed to be activated by on(mouseover focus) will actually only work for mouseover. I think the border around the whole media item could help keyboard users too, but it would need to track focus-within, because the wrapper isn't focussable.

The focus indicators for the checkboxes and title links must remain tough.

Does this make sense? I could make a little demo.

I'm not a big fan of on hover solutions. I think we designed this explicitly to create as much room for longer titles as possible. Then again, grid layouts like this will eventually break if we don't take ultra-super-long titles and/or long words like Eisenbahnenschienenhinundhersteller into account. So maybe we need some kind of mechanism to handle overflow anyway.

Still, I'd like to proceed with what I see in #67

The "Select all" button seems to be in a weird place. Was a "Select all" checkbox on top of the grid itself considered?

For expanding the full title, we can make that work for hover, and focus too.

What about touch screen users? I don't see how they will expand the truncated title.

During Frontend United @yoroy and I discussed and agreed in the following design:
- Change from dark background to white for the item title to play better with Seven general styles.
- Revert the "Select all" to a select again.
- Don't move the "Apply to selected items".
- Title is a link, cropped by default and expanded when hovering it.

Maybe the touch interaction can both select the item and show the full title? My assumption here is that in those cases that the selected item is not what you were looking for *based on reviewing the full title* it will be quicker to just unselect that item.

About keeping same behaviour on desktop... I'm not sure there's a reliable way to differentiate from touch screens so I'd keep it consistent and I'd try to keep the same interaction for now. So it means having the full title displayed for selected items in desktops too.

Soooo... just to remind folks that we're trying to put this feature into core as an Experimental module, at least initially. So while all the attention to these details/edge cases, balancing UX vs. accessibility concerns, etc. is greatly appreciated, I'm concerned that we might be endangering the feasibility of this making it into 8.6 at all if we don't get something committed sooner than later.

Are we close here, or should we start spinning off sub-issues for some of these gnarlier bits?

Also, escalating this to "Major" since it is both one of the main roadmap items for this initiative, and also blocks other aspects of it, e.g. inserting media during content creation.

Only minor nits. None should block commit.

1. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -0,0 +1,56 @@
   +.media-library-view .view-content [id*="edit-header"] > div,
   +.media-library-view .form--inline {
   +  display: flex;
   +  flex-wrap: wrap;
   +}
   

   We can combine this with the .media-library-view .view-content form selector.

2. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -0,0 +1,56 @@
   +.media-library-view .view-content [id*="edit-header"] {
   +  flex-basis: 100%;
   +}
   

   Let's combine this with the .media-library-view .view-content form > [id*="edit-actions"] selector.

3. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,68 @@
   +      $('.media-library-item .media-library-item__preview,.media-library-item .click-to-select-checkbox', context).once('media-library-item-hover')
   

   Nit: We should add a space after the comma separating the selectors. Also, can the .once() call be moved to its own line?

4. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,68 @@
   +      $('.media-library-item .click-to-select-checkbox input', context).once('media-library-item-focus')
   

   Nit: Can .once() be on its own line?

I echo @webchick's sentiment from #85. The bikeshed has been open for a long time on this issue, which is a very important 8.6 target. We need to land this thing. It has undergone a lot of UX and accessibility review, and at this point we don't seem to have any commit-blocking problems on either front, although we've generated several follow-ups (many of which address deficiencies we are inheriting from other parts of core). Everyone seems pretty happy with the patterns we're implementing here. The code looks good to me, and I would certainly consider its new APIs (which is really just the "click to select" stuff) to be beta-quality, although this module is mostly configuration and therefore should not need an update path if we change it later, seeing as how it is an experimental module.

We have beaten around the bush long enough. It's time to go to RTBC.

I reviewed the front-end parts of the latest patch. Mostly it looks good, but there's few things we should still fix. Only the first of the issues should be commit blocking, so if you prefer to open follow-ups for rest of the feedback, feel free to do so.

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -0,0 +1,149 @@
   +* @file media_library.theme.css
   

   These styles are quite opinionated so we should move them to Seven before we mark this module Stable. Let's add a @todo comment so that we don't forget that.

   We also have to make sure that the UI works without these styles.

2. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,31 @@
   +      $('.click-to-select .click-to-select-trigger', context)
   ...
   +            .closest('.click-to-select')
   +            .find('.click-to-select-checkbox input');
   ...
   +      $('.click-to-select .click-to-select-checkbox input', context)
   ...
   +            .closest('.click-to-select')
   
   +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,68 @@
   +      $('.media-library-item .media-library-item__preview,.media-library-item .click-to-select-checkbox', context).once('media-library-item-hover')
   ...
   +          $(currentTarget).closest('.media-library-item').addClass('is-hover');
   ...
   +          $(currentTarget).closest('.media-library-item').removeClass('is-hover');
   ...
   +      $('.media-library-item .click-to-select-checkbox input', context).once('media-library-item-focus')
   ...
   +        $(currentTarget).closest('.media-library-item').addClass('is-focus');
   ...
   +        $(currentTarget).closest('.media-library-item').removeClass('is-focus');
   ...
   +      const $view = $('.media-library-view', context).once('media-library-select-all');
   +      if ($view.length && $view.find('.media-library-item').length) {
   ...
   +              .closest('.media-library-view')
   +              .find('.media-library-item input[type="checkbox"]');
   ...
   +        $view.find('.media-library-item').first().before($label);
   

   We should prefix the classes used in JavaScript with js-. While moving to js- prefixed classes, it would be good idea to revisit the selectors to make sure that we use good specificity, for example .media-library-item .media-library-item__preview selector doesn't need .media-library-item because BEM already guarantees that .media-library-item__preview is inside .media-library-item.

3. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -0,0 +1,56 @@
   +.media-library-view .view-content form > [id*="edit-actions"] {
   ...
   +.media-library-view .view-content [id*="edit-header"] > div,
   ...
   +.media-library-view .view-content [id*="edit-header"] {
   

   Could we add a class instead of using the id for theming?

4. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -0,0 +1,56 @@
   +.media-library-view .view-content form {
   

   Would it be possible to add class to the form element to simplify this selector?

Is there a reason why the media_library view mode of image is using the medium image style and all the other media types are using the thumbnail image style?

Ah, I think I can answer my question by myself :)
Image is the only media type that supports "real" thumbnails. The others are using the default icon, and for that one, the thumbnail image style is enough.

Now that @lauriii has reviewed this, we can remove the front-end framework manager review beacon.

The changes look good to me. I'm moving it back to RTBC, and hopefully we can get a chance to run this past committers soon.

+++ b/core/modules/media_library/css/media_library.theme.css
@@ -0,0 +1,150 @@
+ * @todo Move into the Stable theme when this module is marked as stable.

Sorry, bumping back to NW. We actually want to move this to Seven because we don't want to include this in Stable. If we don't move this to Seven, we cannot make any changes to these styles after marking the module Stable. Since these styles are quite opinionated, the UX team might want to make improvements to these in future.

Let's also open a follow-up for that, and link to it in the comment, so we don't forget.

Looks good. Thanks, Sam.

1. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,31 @@
   +      $('.js-click-to-select__trigger', context)
   

   A number of places we've used $(context).find().

2. +++ b/core/modules/media_library/js/media_library.click_to_select.es6.js
   @@ -0,0 +1,31 @@
   +        .once('click-to-select')
   

   These .once() keys should probably be prefixed with media library or something similar. Click-to-select feels vague.

3. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,68 @@
   + * @file media_library.view.js
   

   This should be .es6.js

4. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,68 @@
   +        .on('mouseover', ({ currentTarget }) => {
   +          $(currentTarget).closest('.media-library-item').addClass('is-hover');
   +        })
   +        .on('mouseout', ({ currentTarget }) => {
   +          $(currentTarget).closest('.media-library-item').removeClass('is-hover');
   +        });
   

   I think we can use the ability to pass multiple events and use .toggleClass() to simplify this.

5. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,68 @@
   +      .on('focus', ({ currentTarget }) => {
   +        $(currentTarget).closest('.media-library-item').addClass('is-focus');
   +      })
   +      .on('blur', ({ currentTarget }) => {
   +        $(currentTarget).closest('.media-library-item').removeClass('is-focus');
   +      });
   

   Looks like all of these lines have some incorrect indentation.

   Additionally,

   I think we can use the ability to pass multiple events and use .toggleClass() to simplify this.

6. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,68 @@
   +        const strings = {
   +          announce_selected: 'All @count items selected',
   +          announce_deselected: 'Zero items selected',
   +        };
   

   Bit curious why these strings are stored in an object when they are only used once.

Discussed #98.2 with @drpal and to clarify -- we don't need to change the CSS class names; just the key we pass to .once().

The changes look good and @drpal's feedback has been addressed. Thanks!

Just reviewed this with @GáborHojtsy and @phenaproxima. This looks frigging AWESOME!!

Played around with various things, and here's a list of what was found:

The biggest thing is that when the module is enabled, you don't actually see the Media tab on the admin/content page until you click on one of the other tabs, or manually clear the cache. This is some sort of views caching issue, because the code in hook_install() is loading the view, replacing some properties, and then re-saving which you would think would be enough to clear the local cache, but sadly no. So we either need a follow-up or to identify the existing issue about this, if applicable. Because this is an upstream problem, however, not a commit blocker for this.

Minor, but the media tab lacks an empty table text the same as the other things under Content. We should add that for consistency.

The URL Alias + Revision/Author info fieldsets still appear which clutters up the form spectacularly. :( However, this is a "pre-existing" condition. There's also a magically shifting around published checkbox that's sometimes above and sometimes below the fieldsets, depending on the media type. Also, "pre-existing condition," though.

"Save" media (under the bulk actions) is a bit goofy; I can't think of any reason you'd want to do that from here. However, that's something that just came over from the existing page in core.

I'm also a little confused... Grid vs. table? Why do we still have both? Seems like a stable blocker to unify this, but not a blocker for beta.

The empty text thing would be great to fix before commit if possible, but not a commit-blocker.

Product manager sign-off: complete!

Because this is a new module for core, adding framework manager review, and because we're hoping it to be beta quality, adding release manager review as well.

1. +++ b/core/modules/media_library/js/media_library.view.es6.js
   @@ -0,0 +1,58 @@
   +            const announcement = $(currentTarget).prop('checked') ? 'Zero items selected' : 'All @count items selected';
   +            Drupal.announce(Drupal.t(announcement, {
   

   l.d.o's parser does not know to look up variables like this, we need to pass the actual string to Drupal.t() otherwise it won't be translatable.

2. +++ b/core/modules/media_library/media_library.install
   @@ -0,0 +1,49 @@
   +      $view->save();
   ...
   +      $view->save();
   

   We should probably call trustData() on the view before saving.

3. +++ b/core/modules/media_library/media_library.module
   @@ -0,0 +1,109 @@
   +function media_library_preprocess_media(&$variables) {
   

   Can't we put this in the media--media-library twig template?

   I thought we were trying to get rid of preprocess hooks.

4. +++ b/core/modules/media_library/media_library.module
   @@ -0,0 +1,109 @@
   + * @todo Remove in https://www.drupal.org/project/drupal/issues/2969660
   

   Minor nit: I think @todos are meant to be placed after @params.

To fix #104.1

const announcement = $(currentTarget).prop('checked') ?
  Drupal.t('Zero items selected') :
  Drupal.t(`All ${$checkboxes.length} items selected`);
Drupal.announce(announcement);

Marking NW for #104. :) Also adding issue credit for those listed in the summary and for other reviewers on the issue.

Re #105: I think the Drupal.t(`All ${$checkboxes.length} items selected`); part from your proposed code should be Drupal.t(`All @count items selected`, { '@count': $checkboxes.length }); instead.

If we want to add empty table text we'd need to open a new issue and add an update hook since the View is already used on all sites using core media.

That's worth looking into. Let's open a follow-up for it.

We could make this configurable and only show the Grid by default on new installs in a follow up, if showing both isn't desirable.

Also a worthwhile discussion to have, but out of scope for this issue, so a follow-up is called for there.

The changes look good; all feedback since #102 has been addressed. Back to RTBC once it's green.

Opened #2981042: Add empty text to Media view and #2981044: Unify the grid/table views of the media library.

We had a conversation today between a few folks, including @xjm, @samuel.mortenson, and @phenaproxima.

Due to the multiple upstream dependencies, as well as the lack of concrete design spec, #2938116: Allow media to be uploaded with the Media Library field widget looks unlikely to land in 8.6. The idea we discussed was shipping Media Library as beta in 8.6 if it includes both this issue and #2962525: Create a field widget for the Media library module. This might need more thinkering, but at a glance, this would give us the ability to ship something of significant value in D8.6 (select from existing media functionality), while giving us the ability to make the "upload new media" experience really slick.

Overall this looks good to me, nothing blocking stood out. I also performed some manual testing and things looked good, nice work!

I have only once concern, but given this is experimental code I think it would be fine to address it in a follow-up:

+++ b/core/modules/media_library/media_library.install
@@ -0,0 +1,49 @@
+  if ($view = View::load('media')) {
+    $display = &$view->getDisplay('media_page_list');
+    if (!empty($display)) {
+      $display['display_options']['path'] = 'admin/content/media-table';
+      unset($display['display_options']['menu']);
...
+    if (!empty($display)) {
+      $display['display_options']['path'] = 'admin/content/media';
+      $display['display_options']['menu'] = [
+        'type' => 'tab',

This logic seems a bit fragile: it's assuming the original media library view was never customized, but that's not safe. I moved the view to another location, installed media_libray, uninstalled it, and my customization was reverted. We should probably add some checks to verify that the path and menu settings are the default ones before altering them.

Opened #2981105: Media Library should not modify the media view in response to the above. Thanks, @plach!

This looks pretty good to me, I don't see a problem with it going in as an experimental module, except unless I'm missing something there's no access control on the view at the moment (see first point):

1. +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   @@ -0,0 +1,447 @@
   +  page:
   +    display_plugin: page
   +    id: page
   +    display_title: Page
   +    position: 1
   +    display_options:
   +      display_extenders: {  }
   +      path: admin/content/media
   +      menu:
   +        type: tab
   +        title: Media
   +        description: 'Allows users to browse and administer media items'
   +        expanded: false
   +        parent: system.admin_content
   +        weight: 5
   +        context: '0'
   +        menu_name: admin
   +    cache_metadata:
   +      max-age: 0
   +      contexts:
   +        - 'languages:language_interface'
   +        - url
   +        - url.query_args
   +        - 'url.query_args:sort_by'
   +        - user.permissions
   +      tags: {  }
   

   Isn't this missing access?

2. +++ b/core/modules/media_library/media_library.install
   @@ -0,0 +1,49 @@
   +
   +/**
   + * Implements hook_install().
   + */
   +function media_library_install() {
   +  // Change the path to the original media view.
   +  /** @var \Drupal\views\Entity\View $view */
   +  if ($view = View::load('media')) {
   +    $display = &$view->getDisplay('media_page_list');
   +    if (!empty($display)) {
   +      $display['display_options']['path'] = 'admin/content/media-table';
   +      unset($display['display_options']['menu']);
   +      $view->trustData()->save();
   +    }
   +  }
   +}
   

   Are we ever going to replace this fully, or is this always going to be necessary? I'm wondering why we couldn't just hide the existing tab, add a new tab, and use a different path. Why do we need to use an identical path here? Could be changed in a follow-up but it feels odd.

I'll leave @samuel.mortenson to respond to #121.1. As far as the second point goes, though -- realism aside, I think the ultimate long-term dream is to replace the table we currently have with the grid, since it's more "visual" and therefore more amenable to displaying a library of media assets. There are a lot of UX questions to sort out there, however, and a follow-up is already open at #2981105: Media Library should not modify the media view (I have copied @catch's feedback into that issue).

For example views.view.media.yml has this:

 display_options:
      access:
        type: perm
        options:
          perm: 'access media overview'

Marking CNW because I'm sure this is missing.

Tested locally with the patch in #115 and confirmed that the Media Library has the "access media overview" permission. Screenshot:

And, for good measure, I confirmed that the modified Media view has the permission as well:

Everything looks to be in order, so I'm restoring RTBC.

OK that's me sorry, it's right up the top of the view, I was looking for it in the page display :(

+++ b/core/modules/media_library/config/install/views.view.media_library.yml
@@ -0,0 +1,447 @@
+langcode: en
+status: true
+dependencies:
+  config:
+    - core.entity_view_mode.media.media_library
+  enforced:
+    module:
+      - media_library
+  module:
+    - media
+    - user
+id: media_library
+label: 'Media library'
+module: views
+description: ''
+tag: ''
+base_table: media_field_data
+base_field: mid
+core: 8.x
+display:
+  default:
+    display_plugin: default
+    id: default
+    display_title: Master
+    position: 0
+    display_options:
+      access:
+        type: perm
+        options:
+          perm: 'access media overview'

#2981105: Media Library should not modify the media view as a follow-up seems fine.

Given this has had product sign-off, both me and plach have reviewed it too, this seems fine to go in as experimental and it's good to see incremental steps towards reusable media widgets.

Fixed these DrupalCS issues before commit:

diff --git a/core/modules/media_library/css/media_library.theme.css b/core/modules/media_library/css/media_library.theme.css
index 783a17b3cc..0427143538 100644
--- a/core/modules/media_library/css/media_library.theme.css
+++ b/core/modules/media_library/css/media_library.theme.css
@@ -20,8 +20,8 @@
   border: 1px solid #ebebeb;
   margin: 16px 16px 2px 2px;
   width: 180px;
-  background: #ffffff;
-  transition: border-color .2s, color .2s, background .2s;
+  background: #fff;
+  transition: border-color 0.2s, color 0.2s, background 0.2s;
 }
 
 .media-library-view .form-actions {
@@ -70,7 +70,7 @@
 .media-library-item__status {
   color: #e4e4e4;
   font-style: italic;
-  background: #666666;
+  background: #666;
   padding: 5px 10px;
   font-size: 12px;

Committed d4350e2 and pushed to 8.6.x. Thanks!

Re: #115 (emphasis mine):

From accessibility/UX review we found that the table is always going to be preferred for users using screen readers that can better parse table markup, and for more technical users that want to show more metadata than the Grid is able to nicely handle. We could make this configurable and only show the Grid by default on new installs in a follow up, if showing both isn't desirable.

It's already configurable: site builders can disable the media table view.

I'm dead set against having the table view disabled on new installs. Of all the accessibility aspects looked at in this issue, keeping the table view is by far the biggest win. It's not just for screen reader users: it provides useful metadata for everyone, and offering multiple ways to manage things is a principle of inclusive design. If the table view was off by default, my hunch is that few site builders would ever think to turn it on for their users. If it's enabled by default, my hope is that few site builders will ever turn it off :-)

Edit: Other benefits of keeping the table view...

• Doesn't have to to deal with truncated titles. Long titles get wrapped in their table cell, and make the row a bit row taller. The table design accommodates this easily.
• Lets you see the full title, without obscuring the thumbnails.
• Lets you see all titles in full, none truncated. The card grid just shows one expanded title at a time.
• Has room for the entity operations links/dropbutton.
• Easier for site builders to extend, allowing any columns known to Views. For example, license/rights, video length, a media tags field. The card grid can't accommodate much more information.

The choice of grid / list is quite common. A quick look at my phone apps show that Google Drive, Dropbox, the Android file manager, and my RSS reader all have a button to swap between a list and a card/grid view.

The choice of grid / list is quite common. A quick look at my phone apps show that Google Drive, Dropbox, the Android file manager, and my RSS reader all have a button to swap between a list and a card/grid view.

Cool, that data point is helpful. My review was less "let's get rid of that" and more "Hm. Why?" since it creates maintenance overhead to keep the two views in sync. But #127 is a pretty good wrap-up of why, so cool. :)

Discussed with @catch. This is something we want to highlight in the frontpage post about the release. :) We don't think there's any specific update instructions we need to add for it though.

Since its marked as an highlight, why removing it from 8.6-dev ?
https://github.com/drupal/drupal/commit/bd6756510075429ef539c5727461323e...

I would really hate to see this not be included in 8.6 as it will really be a step backward in getting media in a real usable form.

I am working on an installation profile in which the Media Library module is a required module via the info.yml.

For some reason, the media_library_install() wasn't called after installing the profile.

Is there anyone have seen an issue like this?

For anyone is facing the issue raised in #134,

The install hook function won't be called during a profile is being installed. If you create your custom installation profile in which the Media Library is enabled, you need to copy the 'views.view.media.yml' from '/core/modules/media/config/optional/' to your profile config/install folder and then make following changes:

      path: admin/content/media-table
      menu:
        type: none

Here is the patch to fix the issue with installation profile raised on #134

Thank you guys for the great work on Media Library!

So far, I'm missing just two features:

1. As a author, I'm in the backend on a node form with a media field. I see the media library preview of the inserted images. Now I want to change something about the inserted images (e.g. its crop settings). I propose a simple button next to the media item preview which links me to the media edit form.

2. Help test in Media Library insert dialogue for image field:
As a admin, I tried to put some help text for my clients under the image field in the insert dialogue. I changed the help text in the settings under
/admin/structure/media/manage/image/fields/media.image.field_media_image
But the entered text doesn't appear in the insert dialogue.

@annerl This issue has been closed 2 years ago. If you have any feature request, it would be better to create a new issue.