Remove link to media item from media library view.

Can you include a screenshot of the problem? Maybe even of the desired state too?

This has tripped me up many times.

#2: Screenshots added. The desired state is simple: remove the <a>, but keep the text wrapped by the <a>.

I think Wim pretty much nailed it.

Oh, I hadn’t considered #4! That is a very good point. I think in that case the entire thumbnail could be made into a link?

As well as the CSS pointer-events: none, there is a JS keypress listener which tries to "fix" this for by presenting a confirmation dialog. Can we clean that up at the same time?

There's another side to this which is that we expand the size of the link when it has focus, to avoid truncating the media label. I don't recall how we did this in the end, but it needn't require a:focus. We can expand the media label with a focusin/focusout behaviour on the media item wrapper.

There are some other problem links too. The table view has links in the author column which are also unnecessary (they take you away from the dialog). That could be changed to a plain text author name.

Can we clean that up at the same time?

+1 — because those are partial work-arounds for the same problem.

There are some other problem links too.

Let's not to do those in this issue. One problem at a time, so that we can get improvements committed without getting stuck :)

Removed a link from all displays and added an edit link to page admin display.

Thanks, @gease!

Added some tests.

Transferring credit for @mcannon, myself, @HeikkiY, and @lauriii from #3041111: Items selected from the media library cannot be reordered by weight, which I'm closing as a duplicate. We should copy the tests from that issue into this one, since it adds some valuable coverage.

Kicking back to "needs work" for tests to be copied from #3041111: Items selected from the media library cannot be reordered by weight.

Finished restyling "edit" link and hook_update_N() without still considering comment #15.

Let's see how the tests feel...

Added tests from #3041111: Items selected from the media library cannot be reordered by weight (comments #6 & #12).

Added "delete media" icon to media library page. Unless some community review fails, nothing more I can add here.

Comparison of current 8.8.x media library items on content admin page without and with patch:

Thanks @seanB for the thorough review.
I'll go step by step:

The feedback in #7 also does not seem to be addressed.

Comment #7 has three distinct notes.

As well as the CSS pointer-events: none, there is a JS keypress listener which tries to "fix" this for by presenting a confirmation dialog. Can we clean that up at the same time?

I am planning to cleanup js with the issue #3062375: Media library item loses focus when hovering over item's title.

There's another side to this which is that we expand the size of the link when it has focus, to avoid truncating the media label. I don't recall how we did this in the end, but it needn't require a:focus. We can expand the media label with a focusin/focusout behaviour on the media item wrapper.

It's expanded on hover over container.

There are some other problem links too. The table view has links in the author column which are also unnecessary (they take you away from the dialog). That could be changed to a plain text author name.

This should be a separate ticket, if at all.

We should probably override the field classes instead of providing an empty div.

If we just override the class, the icon won't be a link. I think I've taken the most transparent way (that is, with the least amount of code and overrides).

I think we need a textual fallback for the links (for a11y purposes).

You are correct, I thought about this (after posting the last patch). Will do it.

Are we sure this is always 44px?

All the margins, offsets, sizes for those icons are given in pixels. And I see no breakpoints or anything. So yes, we are sure.

Why do we need this?

Good question. I turned it off and see no regression. And for sure don't remember clearly why I've put it there. I can guess that before restyling "edit" and "delete" links it prevented clicking them. I can remove it now.

Do we still need this? It seemed to be specific for the link styles.

No, it's for the title. It's part of "show full title on hover" functionality.

I know it already sucked a bit, but with the new additions I hope we can find a better way to write this :(

Basically, I see no reason for putting all kinds of pseudo-selectors, but I kept it as it was, assuming it was done for some reason.

Can we removed .views-field-edit-media?

No, because we have media widget and media page, and edit button is positioned differently in those cases.

I think we need to be a bit more specific. When a user has changed the media_library view on an existing site this would override their changes. We probably need to make sure we only make additions.

We should also have a test for this.

Agree about the test, will add check that there is an edit and delete links on media page. What concerns the view, I'm overriding just the fields of the display concerned. Of course, we can be more granular and just add "edit" and "delete", but chances are this will mess up users' overrides. Overall I don't think it's a best practice to override third-party views.

Can we have an extra test for the delete link as well?

Btw, the test for "edit" is there. So just the same for "delete" to be added.

I don't think we need this line here.

To make sure we are in grid view. If you think this is superfluous, I'll remove it.

The name should never have an anchor in it if I'm not mistaken.

At least, this sounds quite logical. I'll try.

Can we have explicit tests for the widget that there are no links to edit/delete media from the media library? This could be done in a separate issue, but since the solution we chose here is chosen partly to make sure the edit/delete link are NOT in the media library widget, it might be good to assert that.

Ok, I will add these. There is a "remove" button on a widget, but it's an input element, so no problem.

New patch as per above comments.
pointer-events: none;
comes from #3041111: Items selected from the media library cannot be reordered by weight, items weights are not clickable.

Oops, referenced to wrong issue.

Follow-ups filed:

#3064586: [PP-1] Improve checkbox styling in the administrative media library
#3064587: Improve positioning of publishing status label for media items in the administrative media library
#3064589: [PP-1] Improve focus styles for items in the administrative media library

This is really close to ready, and a huge win.

1. --- a/core/modules/media_library/config/install/views.view.media_library.yml
   +++ b/core/modules/media_library/config/install/views.view.media_library.yml
   

   It looks like we have only added things to this view, and not removed anything. I thought we'd be removing some stuff too?

2. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -85,13 +89,6 @@
   -/* @todo Remove or re-work in https://www.drupal.org/node/2985168 */
   -.media-library-widget .media-library-item__name a,
   -.media-library-view--widget .media-library-item__name a,
   -.media-library-add-form__selected-media .media-library-item__name a {
   -  pointer-events: none;
   -}
   

   Have we properly dealt with the referenced issue? (i.e., is it closed or triaged correctly in relation to this one?)

3. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -106,3 +103,7 @@
   +.views-live-preview .media-library-view div.views-row + div.views-row {
   +  margin-top: 0;
   +}
   

   This could use a comment.

4. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -312,6 +312,7 @@
    .media-library-item--grid:before {
      position: absolute;
   +  pointer-events: none;
   

   Why are we adding pointer-events: none here?

5. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -510,7 +508,21 @@
   + * We have to override the .button styles since for a lot of selectors buttons
   + * make heave use of background and border property changes.
   

   What is meant by "a lot of selectors buttons"? Also, "heave" should be "heavy" :)

6. +++ b/core/modules/media_library/media_library.install
   @@ -218,3 +218,218 @@ function media_library_update_8702() {
   +function media_library_update_8703() {
   

   Should this maybe be a post-update hook, so we can take advantage of the Views and entity APIs?

7. +++ b/core/modules/media_library/media_library.install
   @@ -218,3 +218,218 @@ function media_library_update_8702() {
   +  if ($view && $view->get('display.page')) {
   

   $view will always be truthy, even if the view has been deleted. We should check if (!$view->isNew()).

8. +++ b/core/modules/media_library/media_library.install
   @@ -218,3 +218,218 @@ function media_library_update_8702() {
   +    // Check if the edit link field already exist and add if it doesn't.
   

   s/exist/exists

9. +++ b/core/modules/media_library/media_library.install
   @@ -218,3 +218,218 @@ function media_library_update_8702() {
   +    $view->set('display.page.display_options.fields', $fields);
   +    $view->save(TRUE);
   

   Nit: These calls can be changed.

10. +++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateViewPageDisplayEditDeleteLinkTest.php
    @@ -0,0 +1,52 @@
    +/**
    + * Tests the media library module updates for the view page display links.
    + *
    + * @group media_library
    + * @group legacy
    + */
    +class MediaLibraryUpdateViewPageDisplayEditDeleteLinkTest extends UpdatePathTestBase {
    

    Do we, by any chance, have an existing update path test we could merge this into?

11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -435,6 +442,15 @@ public function testWidget() {
    +    $page->find('css', '.ui-dialog-titlebar-close')->click();
    

    This should be $assert_session->elementExists()->click(), to prevent potential method calls on NULL.

1. I didn't participate in any discussions around this, but this seems to introduce a new design pattern. Did you discuss whether you could use contextual links for this? I'm not against introducing a new design pattern, but it would be nice to at least have it documented why we couldn't use the pre-existing pattern for this.

2. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -51,22 +51,26 @@
   +  left: 5px;
   

   This should document that this is for LTR.

3. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -106,3 +103,7 @@
   +.views-live-preview .media-library-view div.views-row + div.views-row {
   +  margin-top: 0;
   +}
   

   Maybe we should open a follow-up against Views UI for this.

4. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -499,6 +493,10 @@
      right: 5px;
   

   This should be marked as LTR.

5. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -510,7 +508,21 @@
   + * Media library widget edit and delete button styles.
   + *
   + * We have to override the .button styles since for a lot of selectors buttons
   + * make heave use of background and border property changes.
   ...
   +.media-library-item__edit.button,
   +.media-library-item__edit.button:first-child,
   +.media-library-item__edit.button:disabled,
   +.media-library-item__edit.button:disabled:active,
   +.media-library-item__edit.button:hover,
   +.media-library-item__edit.button:focus,
   

   Where can I test this version of the element where the element has .button? Could we remove the .button class from the element to simplify this?

6. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -523,20 +535,61 @@
   +  right: 40px;
   

   This should be marked as LTR.

7. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -523,20 +535,61 @@
   +  right: 10px;
   

   This should be marked as LTR.

This may be somewhat relevant: #2998005-51: [PP-1] Support Drupal core's Media Library.

#35: That screenshot looks great! This is part of the work that #3023807-12: Override media fields from the reference field proposed 👍

I'm also building on top of this in #2998005-51: [PP-1] Support Drupal core's Media Library, which @phenaproxima cross-posted with me 🤓

Patch no longer applies. I think it needs a reroll :)

I gave this a quick manual test. I installed Drupal from HEAD, then enabled Media Library. I then applied the patch and ran the database update, which went flawlessly. Then I logged in as admin and went to /admin/content/media, added a couple of media items, then navigated into them with the keyboard (which worked great!) and the mouse (which also worked great!). I was able to select them, edit them, and delete them without any issue.

It also worked admirably on a clean install. I think this is done.

+++ b/core/modules/media_library/media_library.install
@@ -218,3 +218,218 @@ function media_library_update_8702() {
+function media_library_update_8703() {
+  $view = \Drupal::configFactory()->getEditable('views.view.media_library');
+  if (!$view->isNew() && $view->get('display.page')) {
+    // Fetch the fields from the page display, if the fields are not yet

For updating some config in an update hook, we usually use the ConfigEntityUpdater. For example, see block_content_post_update_add_views_reusable_filter()

I think we should do that here as well.

For updating some config in an update hook, we usually use the ConfigEntityUpdater. For example, see block_content_post_update_add_views_reusable_filter()

I don't know if that's necessary here, since we're only modifying a single config entity, and we're doing it using the low-level config system...

Minor IS update.

#44.1 I think having it in this issue is enough (at least now it's somewhere).
#44.5 Can we open follow-up to figure out if it would be possible to remove the automatically added classes?

It seems like the field widget remove button has some issues with the positioning of the cross:
Chrome:

Firefox:

Other than that the frontend changes look good to me.

I just tried reproducing the cross alignment issues from #51, and I'm not able to reproduce it. I took this screenshot on a Retina Macbook Pro; Chrome 75.0.3770.100 on the left and Firefox 67.0.3 on the right.

So...I am reproducing this problem on a non-Retina MacBook Air. However, the weird thing is that the problem disappears if I zoom in; only at the default zoom level do I see it. Here's a screenshot; Chrome is on the left, Firefox on the right.

This is a weird problem. Fiddling with it in the web inspector, I discovered that, on my machine at least, changing the background-size of the X to 14px solves the problem. Screenshot attached; Chrome on the left, Firefox on the right.

It seems to be some kind of rounding issue. Changing the background size changes the size of the icon, so we probably should increase the size of the button as well to keep the design consistent. If that doesn't work, there are some other, more complex workarounds we could look into.

Edit: another solution would be to just change the size of the button, without changing the background size. For example, increasing the size by 1px seems very minor change to the design, and fixes the rounding issue on 100% zoom.

Increased the button size, and now the X looks good in both Firefox and Chrome.

Looks good to me; thanks @starshaped!!

I filed #3065431: Allow buttons to opt out of getting the 'button' class to handle the button class snafu.

I also look another quick look at the changes from @starshaped. Looks great in both Chrome and Firefox at 100% zoom:

I think we're done here. Green once testbot catches up.

Whew, I didn't expect cross-browser CSS issues to suddenly appear! Now I'm extra glad that I made #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` reuse this patch's CSS rather than duplicating it 😅

This patch introduces some css coding standards regressions. You can find this out by doing:

cd core
yarn run lint:css

And fix this by doing adding --fix to the command.

+++ b/core/modules/media_library/templates/media--media-library.html.twig
--- /dev/null
+++ b/core/modules/media_library/tests/fixtures/update/drupal-8.media_library-update-view-page-display-edit-delete-link-3039829.php

+++ b/core/modules/media_library/tests/fixtures/update/drupal-8.media_library-update-view-page-display-edit-delete-link-3039829.php
+++ b/core/modules/media_library/tests/fixtures/update/drupal-8.media_library-update-view-page-display-edit-delete-link-3039829.php
@@ -0,0 +1,111 @@

@@ -0,0 +1,111 @@
+<?php
+// @codingStandardsIgnoreFile
+/**
+ * @file
+ * Contains database additions to drupal-8.bare.standard.php.gz for testing
+ * the upgrade paths of the media library module widget view.
+ *
+ * @see https://www.drupal.org/project/drupal/issues/3020716
+ */

+++ b/core/modules/media_library/tests/src/Functional/Update/MediaLibraryUpdateViewPageDisplayEditDeleteLinkTest.php
@@ -0,0 +1,52 @@
+  protected function setDatabaseDumpFiles() {
+    $this->databaseDumpFiles = [
+      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz',
+      __DIR__ . '/../../../../../media/tests/fixtures/update/drupal-8.4.0-media_installed.php',
+      __DIR__ . '/../../../fixtures/update/drupal-8.media_library-update-view-page-display-edit-delete-link-3039829.php',
+    ];

The @see is not correct. And the docs aren't really either - it's for adding on top of drupal-8.4.0-media_installed.php.

I think we should call this drupal-8.7.2-media_library_installed.php as this file is probably more generic than just for this specific issue. And remove the @see

Also we should file a follow-up issue to rename and merge core/modules/media_library/tests/fixtures/update/drupal-8.media_library-update-view-table-display-2981044.php and core/modules/media_library/tests/fixtures/update/drupal-8.media_library-update-widget-view-3020716.php to whatever version of media library install they represent.

Leaving rtbc as this is naming things.

Filed #3065718: Merge and rename Media Library update path fixtures.

Crediting @andrewmacpherson for review.

Pushed to 8.8.x.

Yay! Rebased #2994702-15: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`, because it no longer needs to include this patch anymore 🥳

I would like to nominate this patch to be backported to 8.7.x, since it’s a significant accessibility improvement in an experimental module.

While this is indeed a good accessibility improvement, I think it's too disruptive to backport the item for a patch release (given the update hook, UI changes, etc.). I would leave this one as 8.8-only for now unless it's blocking other accessibility bugfixes or unless the issue is more severe than I understand.

Thanks!

This should probably have been a post update rather than hook_update_N(). If we're not going to backport it, it creates a conflict if we did want to backport another update in the future. Also the update is just updating a config entity.

Hm, should we have a followup that moves it to a post_update, and leaves behind an empty media_library_update_8703()? Then we can backport just the empty media_library_update_8703() to 8.7 to prevent future number conflict? Or the entire a11y fix if we decide to?

Apparently this bug is the cause of #3041111: Items selected from the media library cannot be reordered by weight. Can we at least provide a patch that fix this issue? It's unusable in a lot of use case.

Adam will create a followup for the update issue.

Filed #3084043: Convert media_library_update_8703() to a post-update hook.

The follow-up has been created and is now RTBC, so I think this can be marked fixed.