Unify the grid/table views of the media library

It would be nice if you could link 2 views displays as a grid/table display and handle this through views in a generic way. There are a lot of views besides the media library that could benefit from this. Below are some examples from major shopping sites.

The media library view is now separate from the regular media overview. That might be a problem, but I guess in the future it would make sense to merge the 2 views together.

As per the accessibility feedback received above, it looks like having both options available by default is a good solution (which I personally agree with). In this case, is there anything else this issue is trying to solve?

#4 mentioned the fact that the two views are separate right now, but I don't really see an issue with that. If we want to avoid the tabs as a mechanism to toggle between grid/table view, we can easily add a Text area header to both views, containing the two links, and style them to look like the toggle icons that users are familiar with (such as in the screenshots above). But I'm not sure there is a strong need to remove the tabs, IMHO.

Is anything else needed here?

Just closed #2973162: allow users to toggle between the media library grid and the old table-based list as a duplicate. This is a copy of the IS that contains important information from @andrewmacpherson.
----------------
Problem/Motivation
Follow-up from comments #80 and #97 in #2834729-87: [META] Roadmap to stabilize Media Library.

We might want to allow users to toggle between the media library grid, and the old table-based list.

Misc benefits...

• Offering users a choice of tools is a recognized principle of inclusive design. If you find one interface awkward, try the other.
• Keeping the familiar interface available while they're still leaning the new one, a good strategy to support users with cognitive/learning impairments.
• Reduce frustration for users who might otherwise be annoyed that you took their old tool away, when it worked just fine.
• Many screen readers provide some power tools for navigating table content. Users who are confident with these can save a lot of time with things like "jump to end of row", or quickly going down the title column while ignoring the other fields.
• Sorting the list can easier to understand with a table.
• Reviewing selections (before running bulk-operations) might be easier with a table, too.
• You can fit more items in the viewport with a table than with a card grid. Less scrolling needed for speech control users.

Proposed resolution
Provide a way to swap between the table and card-grid views of the media library.

MVP could just be a pair of views displays configured using the secondary tabs functionality.
A more advanced approach could preserve filters and bulk-operations selections when switching displays.

Just discussed this with phenaproxima and lendude on slack. We came up with the following solution.

We are going to create a new area plugin where you can link view displays together. We only allow a display to be linked if:

• It is a page display
• It has the same filters
• It has the same arguments
• It has the same pager settings

We render links to the other displays in the view area and pass the filters/arguments/pager when a link is clicked. This would mean you get the exact same result, but showed differently.

Re: #11...

It has the same pager settings

To clarify, does this mean:

• The exact same views pager plugin (full, mini, ...)?
• Or, just the same number of results per page?

What problems would arise if they:

• Didn't use the same pager plugin?
• Showed a different number of results per page?

For bulk operations selections, can a user have selections spanning more than one page's worth of results?

I can imagine that a table (with small thumbnail images) may be able to accommodate more results in a page/viewport than a card-grid (with larger images). It depends on the particular dispay settings, but in principle I don't see why a listing couldn't swap between a 4-column grid of 20 results per page, and a table of 50 results per page. That may be the feature that makes switching displays useful to a user.

I can imagine that a table (with small thumbnail images) may be able to accommodate more results in a page/viewport than a card-grid (with larger images). It depends on the particular display settings, but in principle I don't see why a listing couldn't swap between a 4-column grid of 20 results per page, and a table of 50 results per page.

While I agree that a table display is suited to show more results, what should happen when a user navigates to page number 3 and the displays are showing a different number of items? The first/top result for those displays would be different and probably confusing.

The only way we can guarantee both displays are showing the exact same results is when the filters/arguments and pager settings are equal.

For bulk operations selections, can a user have selections spanning more than one page's worth of results?

Core currently doesn't support that.

Created #3025657: Add views area plugin to display a link to another view display within the same view and this is blocked on that issue for now.

This is a first patch to add the views display and the header links based on #3020716: Add vertical tabs style menu to media library and #3025657: Add views area plugin to display a link to another view display within the same view.

+++ b/core/modules/media_library/js/media_library.ui.es6.js
@@ -120,6 +120,45 @@
+   * @todo Remove when the AJAX system adds support for replacing a specific
+   *   selector via a link.
+   *   https://www.drupal.org/project/drupal/issues/3026636

nit: @see https://...

Otherwise, all looks good to a quick skim. I haven't applied locally and tested manually, but the screenshot looks nice. ;) Good to have code for the original use-case in core for #3020716: Add vertical tabs style menu to media library.

Thanks,
-Derek

Rerolled, plus the following changes:

• Fixed AJAX bugs.
• Added nice icons as designed in #3019150: Update/improve mockups and designs for the media library. decided to keep the text instead of just showing an icons for usability and accessibility.
• Added post update hook to add the changes to existing views.
• Added tests.

Interdiff is probably not super helpful but added it anyways.

Updated and added screenshots to the IS.

#25
1. I don't think it is?

.media-library-view.view-display-id-widget .media-library-select-all,
.media-library-view.view-display-id-widget_table .media-library-select-all {
  display: none;
}

2. Fixed. We do :). Screenshots in the IS.
3. We only want this for the media library widget displays, not for the admin/content/media display in the same view.
4. Fixed.
5. See 3.
6. That can only be done via a template. And we can't add templates to seven for the media library yet :(
7. See 3.

Also think I fixed the tests.

Fixed a small RTL issue and webchick noticed I somehow managed to use the grid icon for the table link and vice versa. Should be fixed now. Also updated screenshots in the IS.

+++ b/core/modules/media_library/config/install/views.view.media_library.yml
@@ -662,9 +665,268 @@ display:
+            items_per_page: 25

Minor quibble: since the grid is responsive, I've found that multiples of 12 are best for these sorts of things, since that's evenly divisible by 1, 2, 3, 4 and 6. :) As you view at different widths, you basically always get full rows of items. 25 is only evenly divisible by 5, so there's only 1 magic width where you see a "full" grid. What do y'all think about standardizing on 24 for both of these displays? If you have 24 items, and you happen to be at the 5-wide width, you still get a mostly-full last row, and all the other possible widths always give you full rows.

Thoughts?

Thanks!
-Derek

While working on a demo, I noticed the .media-library-item class used to style the grid was also applied to the table. We shouldn't do that, so added a separate class for the table. Also made sure the JS uses the js-* class.

Regarding #30: I totally agree! Not sure why this hasn't come up before? Let's do this in a follow up though, since this should get UX sign-off, front-end FM sign-off, and an update path + update path tests.

Also found another issue with the views DisplayLink plugins. Created #3033404: Views DisplayLink area plugin tries to use non-existent theme hook for that.

Re: #31: Follow-up at #3033442: Use 24 items per page for better responsive grid behavior

Thanks!
-Derek

Fixed the tests. Also simplified the CSS selectors a bit and made sure the links are close to the view. That makes a lot more sense.

Updated the screenshots in the IS and added a video.

Reroll now #3023802: Show media add form directly on media type tab in media library has landed.

#36
1. Since the view also contains admin/content/media (and maybe the table display for that as well), I guess having the widget prefix is good to differentiate between the displays.
2. Fixed.
3. Afraid not, the 'select all' checkbox should only be hidden in the widget, not on admin/content/media.
4. We need to force this ID on the view to make AJAX work for the grid/table links. We can only use ID as a selector.
5. Fixed.
6. Fixed.

Also removed media_library_views_post_render(). This code breaks the previews in the views UI, and doesn't really do anything anymore since we have the MediaLibraryUiBuilder using MediaLibraryState.

Whoops. Removed just a little too much.

#40
1. The view from the media module also uses 'author'. The media edit page uses Authoring information and 'Authored by'. I think we should for now copy what we already have. Maybe a followup to discuss if this should be changed?
2. Same as 1.
3. Fixed. Added an extra CSS class to the grid/table displays.
4. Fixed.
5. Fixed.
6. Fixed.
7. Fixed. Added a test to verify the 'Apply filters' button is not added to the button pane of the dialog for the grid/table displays.
8. Fixed. Added a message to inform users they will probably need to reinstall the module of the view is deleted. I think we should have a followup to remove delete access for this view (and probably the displays as well!).
9. Fixed.
10. Not sure what happened there :p
11. Fixed. In a previous version, the update hook assumed the pager and sorts were still using the default, so they were not updated. Users could have manually overridden the grid display pager/sort settings. In this case we need to verify that the widget_table display copies its settings from the widget display (and does not use the settings from the default display). Added some comments to explain this a bit more.

Besides that, added aria-live="polite" to the views container to make sure the new content is announced when clicking a display link.

#42 Fixed 1 and 3. I think throwing an exception is not the best solution here, since we would also stop other updates from running. This could potentially leave the sites in a state where they are not able to fix the problem. A missing view is not worth doing that imho.

This issue just changes the content of the MediaLibraryWIdget, right? Is there another issue to do this to the page view at admin/content/media?

Review of manual testing of patch #41, with and without a screen reader.

1. Besides that, added aria-live="polite" to the views container to make sure the new content is announced when clicking a display link.

   That's way too overblown. This attempts to read the entire content of the view! Confusingly, the first thing it reads is the pair of buttons. one of which was just pressed. Then it reads everything else in the view. The attached screenshot shows the extent of the ARIA live region, using the browser's dev tools layout overlay highlight feature.

   Think about what you actually need to convey:

   • A button was pressed.
   • Then, something happened.
   • So, say what just happened as a result of pressing the button.

   If a blind person asked you "what just happened?", how would you answer in one short sentence? Study the way the toolbar module informs the user of a change in orientation; it doesn't read the entire toolbar.

2. The tabbing order of the view is messed up. The grid and table buttons come before the name filter in the DOM, but visually they come after the apply-filters button This will confound sighted keyboard users. Make the tabbing order match the visual reading order.

   You can fix this in one of two ways. Both involve ditching the CSS which controls the visual position:

   • Swap the order of these two divs in the DOM, to put the buttons second.
   • Let the buttons appear first like the current the DOM order says.

   WCAG technique C27: Making the DOM order match the visual order has further info about this.

3. "Show as grid" and "show as table" are links. Are we intending for MediaLibraryWidget to work without JS? If so, they should probably stay as links. If not, they would be better as buttons. Don't fret about this too much though, it's part of a bigger problem across our entire AJAX system. There are a lot of places where an AJAX dialog works without JS by taking you to an intermediate page (for example adding a block in admin/structure/blocks). So there's a situation where they link is the right role when JS is off, but button should be the role when JS is on. I think there might be an issue elsewhere about that. The new views plugin for a link to another display will presumably have uses outside of a dialog, so this could be an awkward one. However I don't think it's a very major accessibility issue, as it doesn't present an outright barrier to using it. Many screen reader users are accustomed to the fact that buttons and links are used interchangeably by developers, and will shrug it off. However some users will be confused by it (for example, tech support tries to direct them to a button, but it isn't in the screen reader's list of buttons).

#44
1. Fixed. We should probably have a special media-library-item--grid and media-library-item--table class. This would also make the CSS a lot more clear since was a little confusing that the media-library-item class adds only grid specific styles.
2. I don't think we should do that. Same reason we decided the dialog options don't belong in the state.
3. Fixed.
4. Fixed.
5. Fixed.
6. Fixed.

#45
1. Fixed. Removed the aria-live attribute and added Drupal.announce() messages.
2. I've added @todo messages to fix this when the media library is stable, since we can't override the views template from a module. We also can't add a template to seven for a experimental module. It will be either a UX / design issue or an a11y issue. Not sure what to do now?

/* @todo Remove order and reorder the views header and filters via a template
     when styles are moved to the seven theme in
     https://www.drupal.org/project/drupal/issues/2980769 */

3. The media library widget doesn't work without JS, but technically the displays are page views that work outside of it. Leaving this for now.

Re. #46

Focus order:
Hmm, I understand the issue with templates, but that gives us a chicken-or-egg problem. Focus order issue is a WCAG level A problem, and that's a blocker for the accessibility gate. We can't mark it stable.

The other approach is to remove the CSS which hacks the visual order. Do the table/grid buttons really have to appear after the filters in the visual order? I don't think it matters a great deal which one is above the other, but the focus order jumping up and down isn't good.

Announcements:

1. +    if ($options['target_display_id'] === 'widget') {
   +      $options['attributes']['data-display-announcement'] = t('Media library content changed to grid view.');
   +    }
   +    if ($options['target_display_id'] === 'widget_table') {
   +      $options['attributes']['data-display-announcement'] = t('Media library content changed to table view.');

   Getting there, but these are still a bit verbose. The outcome message is over twice as long as the button text. Can you think of a shorter message?
   Also, what exactly does "content" mean here? I thought these buttons just changed the presentation. This message might be misinterpreted, as meaning that different media items are being shown.

2. +            } else {
   +              Drupal.announce(Drupal.t('Media library content changed.'));
   +            }

   This message is useless. In what situations does it get used?

3. + Drupal.announce(Drupal.t('Loading media library content.'));
   This is vague. What situation is it for?

4. +          // Override the AJAX success callback to announce the updated content
   +          // to screen readers.

   This comment makes it sound like an awkward workaround. Can we use the new AnnounceCommand here, and keep the message logic on the server side?

Manual testing of patch #46:

1. After pressing one of the grid/table buttons, focus moves to the start of the dialog. That's not necessary, and is disorientating. Users have to tab past all the vertical tab headers, and the "add files" widget, to recover their position in the dialog.

   TODO: It will be better if focus remains on the button that was just pressed. It seems the view filters and display buttons are all replaced as well as the view results, so it will need some way to remember the button which was pressed, and restore focus to it programatically. Aside, we could do with a generic method to say which element get focus after AJAX responses.

2. I haven't been able to produce the else-case message from #48.2 ("Media library content changed."). I'm still none the wiser what this is for.
3. I've heard the message from #48.3 in context. When using the table button, the screen reader announces:
   • "Media library, dialog." I think this happens because focus has moved to the start of the dialog (point 1 here), but I'm not certain. That's actually useful, because the user needs to know about the undesirable focus shift in point 1 here (however we should still fix that). When we programatically maintain focus on the button, this dialog announcement might go away. If it doesn't, that's fine. No action needed here.
   • "Loading media library content." This is queued up just after the AJAX request has been sent. I think you're saying this because we have no idea how long we're going to wait for a response? The message itself could be more succinct.
   • "Media library content changed to table view." This is queued up after the AJAX response has been received. It's OK but it could be more succinct.

   Overall, these messages are OK, but we should aim to reduce the verbosity. When they add up, it's overkill. We hear the phrase "media library" three times as a result of one button press.

   The "loading" message is the one that bothers me most. If the response comes quickly, say within a second, the "loading" message isn't needed, and possibly annoying. But if the response is slow, the message tells the user something is happening. That sounds great in principle, but we aren't doing this for AJAX requests anywhere else. For example, when pressing "apply filters" we don't have any similar "loading" message for screen reader users.

   For now, leave the loading message in place. It's a bit out of place compared to the rest of our AJAX UI, but we can clean it up after we have a generic approach. (Elsewhere, there is an issue to convey AJAX progress messages to assistive technology. I'll dig that out, it has a patch I think.)

The generic AJAX progress accessibility issue is #2973140: Convey AJAX progress messages to assistive technology..

That's would be a good issue to treat as could-have for Media Library, so I'll add it to the roadmap. By my count there are 3 ajax behaviours in the media library widget that can benefit from this: changing tab, changing filters, changing to grid or table.

In any case, we can update the patch here with a @todo before the "Media library content is loading" message, to change it after the generic approach is ready.

Just a comment to the possible followup:

"Author" is a strange term for a media item. How about "Creator" or "Created by"?

I know that for now we settled with Author, which is ok for the moment. However, from the fact that someone uploaded a media item we can‘t infer the user is also the author or creator of the media.

Now you might say that the same argument holds for text content as well. True, however less so. Finally, we‘re providing tools to create new textual content, while our tools don‘t support actually creating AV media. So the chances someone else is the actual author/creator are infinitely higher. Furthermore, the level of originality tends to be higher for AV media.

Therefore, the correct term for media might be „uploader.“

#48 / #49 / #50

Hmm, I understand the issue with templates, but that gives us a chicken-or-egg problem. Focus order issue is a WCAG level A problem, and that's a blocker for the accessibility gate. We can't mark it stable.

I'll leave it up to product managers to make a decision on what to do. We already have stable blockers that can only be fixed after/when the module is marked stable. If we need to fix it now, we need to have a new design for the placement of the links until we mark the module stable.

1. Changed the messages to 'Changed to X view'. Hope this helps.
2. Showing a default message if the data attributes are missing might not be helpful at all, so removed it. Site builders can technically add more display links and not add a data attribute with a message, but that might be their responsibiity.
3. Changed the messages a bit and added the todo's.
4. We have #3026636: Allow AJAX links to replace a specific selector to add support for more advanced use cases to AJAX links #3033150: Add feature parity for consistency between AJAX links and the Form AJAX API. Until then we have to do our own thing, since improving the AJAX system in general is not a direct priority. Implementing a custom controller for this to do everything server side adds a lot more complexity, and should ideally not be nessecary.

I know that for now we settled with Author, which is ok for the moment. However, from the fact that someone uploaded a media item we can‘t infer the user is also the author or creator of the media.

Now you might say that the same argument holds for text content as well. True, however less so. Finally, we‘re providing tools to create new textual content, while our tools don‘t support actually creating AV media. So the chances someone else is the actual author/creator are infinitely higher. Furthermore, the level of originality tends to be higher for AV media.

Therefore, the correct term for media might be „uploader.“

I think this strongly depends on the context, but I can see how this can be confusing to users. What we actually mean is "user that created the media entity". This is not necessarily an upload (for example, Youtube videos are added via URL). I think the generic term for users that create something in Drupal is currently 'Author'. Introducing different term for different entity types is probably not the best idea. If we want to change this, I think we should look at generic terminology for "user that created an entity" and change it everywhere to avoid confusion.

#52...

+          // @todo Replace custom announcement when
+          //   https://www.drupal.org/project/drupal/issues/2973140 is in.
+          if (displayAnnouncement) {

The @todo for #2973140: Convey AJAX progress messages to assistive technology. only relates to the loadingAnnouncement I think?

"Loading grid view. Changed to grid view." and "Loading table view. Changed to table view."

Those are much better messages, call that part done :-)

The @todo for #2973140: Convey AJAX progress messages to assistive technology. only relates to the loadingAnnouncement I think?

You are right! New patch attached.

Forgot to pull into the branch when making #56. Here is a reroll of that exact patch. Interdiff of #56 is the only change since #51.

Mentioned in #45.2 and #48.

The tabbing order of the view is messed up. The grid and table buttons come before the name filter in the DOM, but visually they come after the apply-filters button This will confound sighted keyboard users. Make the tabbing order match the visual reading order.
You can fix this in one of two ways. Both involve ditching the CSS which controls the visual position:

Swap the order of these two divs in the DOM, to put the buttons second.
Let the buttons appear first like the current the DOM order says.
WCAG technique C27: Making the DOM order match the visual order has further info about this.

The response in #46:

I've added @todo messages to fix this when the media library is stable, since we can't override the views template from a module. We also can't add a template to seven for a experimental module. It will be either a UX / design issue or an a11y issue. Not sure what to do now?

Either we fix the DOM order when the library is stable, or we fix the design when the library is stable and implement a different design until then. I think we need a product manager to make a final decision on that one. We unfortunately can't have both.

Fixed #60 and some code style things. Also added extra asserts for the announcements.

Rerolled #61.

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -161,40 +152,137 @@
   +  padding-left: 24px;
   ...
   +  background: url(../../../misc/icons/333333/grid.svg) 0 0 no-repeat;
   ...
   +  background: url(../../../misc/icons/333333/table.svg) 0 0 no-repeat;
   

   Let's add a comment that these properties are for LTR.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -161,40 +152,137 @@
   +  background: url(../../../misc/icons/333333/grid.svg) right 0 no-repeat;
   ...
   +  background: url(../../../misc/icons/333333/table.svg) right 0 no-repeat;
   

   We can override the background-position without overriding the image.

3. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -161,40 +152,137 @@
   +  margin: 2px 16px 16px 2px;
   

   This needs RTL styles.

4. +++ b/core/modules/media_library/media_library.module
   @@ -63,29 +61,60 @@ function media_library_theme() {
   +    $variables['attributes']['id'] = 'media-library-view';
   ...
   +    $options['attributes']['id'] = 'views-media-library-display-link-' . $options['target_display_id'];
   

   We should ensure these IDs are unique.

5. 
   There's a regression in the spacing of the field widget grid.
6. 
   The remove button positioning is not correct in the field widget.
7. 
   There's sideways scroll on the dialog.

Removing the tag but I'll keep my eyes on this 🧐

Yep, the follow-up template plan from #64 has accessibility maintainer sign-off, but I'd like to clarify that we first need to know that the follow-up is actually feasible.

So it would be a good idea to develop the patch in the follow-up issue, while this issue is still being worked on. That way there could be something ready to go as soon as the module is stable, and minimize the chance of encountering a gotcha after marking media library as stable.

I'll add a note in the roadmap issue, about the special handling of the accessibility gate for this issue.

Updated screenshots and video in the IS.

#63
1. Fixed.
2. Fixed.
3. Fixed.
4. If we do that, the IDs will not be the same after the view has loaded and the focus will not be set back on the link. So I guess that is going to be a problem.
5. Fixed.
6. Fixed.
7. Fixed. This was caused by the label for the checkbox having a table display and visually hidden class.

#64 / #65 Changed the link labels and created #3035994: Add a Media Library views template to Seven theme to change to order of the views header and exposed filters

Also added back code in media_library_views_post_render() that I removed in #37. Turns out I was wrong and we need that code when you apply filters and/or sorts. We apparently had no tests that filter a view, select some media from the filtered results and inserts that to the widget. So I added this to make sure this doesn't happen again.

#67

1. Fixed, removed it. I honestly don't know why it was there. There is no primary button in the view anywhere, could be a copy/paste thing from #3023797: Let users choose what to do after selecting and/or adding items in the media library (where we introduce primary buttons).
2. Fixed.
3. Fixed.
4. Fixed.
5. Fixed.
6. Fixed.
7. Fixed.
8. Closing the dialog does not start an AJAX request.
9. We want to now if the page contains the table display, isn’t checking for it’s existance clearer than checking of the number of elements with that class is a certain number? elementNotExists is also slighly more optimized.
10. Fixed.
11. Fixed.
12. Fixed.
13. Fixed.
14. Fixed.
15. See 9.
16. Fixed.
17. I guess in this case we can, but we use $assert_session->elementExists('css', '.ui-dialog-buttonpane')->pressButton('Select media'); 9 more times in the test. Maybe consistency is a little bit more important here?

Whoops! Found 1 minor code style thing (somehow CSS Comb adds this newline constantly).

This is looking good, really nice UX...raises the bar for Drupal admin experience.

1. +++ b/core/modules/media_library/media_library.module
   @@ -63,14 +63,63 @@ function media_library_theme() {
   +function media_library_preprocess_container(&$variables) {
   

   is there a performance cost here...not sure its as bad as on a link (story for another day) - but we should profile this - I think a lot of form elements will end up going through this hook.

   looking at \Drupal\views\ViewExecutable::buildThemeFunctions it adds theme suggestions based on tags on the view - can we explore that?

   And yep, I know \Drupal\views\Element\View::preRenderViewElement is a pita in so far as it adds the 'container' theme wrapper. (our current starter theme at work includes alter stuff to get rid of that by default, because it naffs flexbox and other things)

   I guess first step would be to profile the impact this has before we go too far

2. +++ b/core/modules/media_library/media_library.module
   @@ -63,14 +63,63 @@ function media_library_theme() {
   +function media_library_link_alter(&$variables) {
   

   oh..speaking of links and performance. This is almost certainly a performance hit - that we need to profile - this runs for every menu link...and on big sites...is a huge drain in my experience

   Options I think we have here -
   1) we could add 'class' and 'id' support to the existing plugin, but it wouldn't help with the announcement data attributes
   2) we could add custom area plugins just for media (and extend from the existing display link)
   3) we could add a new alter hook in display link - ie. more specific than 'link_alter'

   Thoughts?

Even though custom area plugins for media would probably be the quickest way to solve this, I'm not sure if users configuring views would understand why they have 2 plugins doing basically the same thing.

I think a custom alter hook for display links would be a better solution. Added #3036694: Add hook_views_display_link_alter() to allow modules to alter view display links. for that. It would also make it easier to target the display links.

The important question now is. Is it really a performance issue at the moment an are we blocked on #3036694: Add hook_views_display_link_alter() to allow modules to alter view display links.?
We are doing 1 isset() call for each link with 2 variables, we could optimize and split them up so we check 1 variable at a time and return faster for most links? That would already make it a bit more efficient.

According to https://phpbench.com/ an isset() call takes 0.01 milliseconds. If a hook_link_alter() that for 99% of the links only does an isset() call is a performance issue, I guess we should definitely document this better on the hook and discourage its use.

#76
1. I tried, but we can't do that because we only control the rendering the first time. After changing filters, sorts etc the view is not rendered by the media library. Also added the ID to the views container in JS now, since the only reason we need that in the first place is because #2821793: Replace #ajax['wrapper'] with #ajax['wrapper_selector'] has not yet landed.
2. Done. We are using classes as selector in a way we normally don't, but I think for now this is fine until we can alter the links properly via #3036694: Add hook_views_display_link_alter() to allow modules to alter view display links..

Let's reroll again now #3035434: Improve name of MediaLibraryWidget dialog landed.

Fixing tags

The JS workaround is acceptable to me until such time as we resolve the follow-ups (#3036694: Add hook_views_display_link_alter() to allow modules to alter view display links. and #2821793: Replace #ajax['wrapper'] with #ajax['wrapper_selector'])

But I think we need the front end framework manager to sign off on the new JS, so adding that tag - I'll ping @lauriii for another look.

The JavaScript workaround is acceptable to me as well. Hopefully, we will be able to make this nicer after resolving the follow-ups.

+++ b/core/modules/media_library/config/install/views.view.media_library.yml
@@ -662,9 +666,271 @@ display:
+          display_id: widget
...
+          display_id: widget

Nitpick: could this be changed into widget_grid? It would make the class names more verbose.

Nitpick: could this be changed into widget_grid? It would make the class names more verbose.

The widget display was already in the view for existing sites (added in 8.6). Renaming that could break customisations in existing sites (css, alter hooks etc). I don't think there are a lot of sites that did that, but I'm not sure if this is worth it.

Superb, thanks all. I believe the overall visual coherence of this window needs work (relative placement, visual weights, etc.) but we can refine that later on as well, especially once the dropzone for uploads lands as well.