Problem/Motivation

Media Library widget has a weight field that allows specifying order of the attached media entities. The weight field doesn't have any effect when only single media can be attached to the field.

Proposed resolution

Remove weight field from the Media Library widget when only single media can be attached.

Remaining tasks

User interface changes

Correct cursor in media library and no weight field when selecting a single media library.

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#69 after_patch.png257.54 KBambuj_gupta
#67 interdiff_64-67.txt1.23 KBhimanshu_sindhwani
#67 3102402-67.patch7.31 KBhimanshu_sindhwani
#64 interdiff_55-64.txt3.71 KBhimanshu_sindhwani
#64 3102402-64.patch6.83 KBhimanshu_sindhwani
#57 Before_Multiple.png394.17 KBpriyanka.sahni
#57 Before_Single.png261.08 KBpriyanka.sahni
#57 After_Multiple.png374.71 KBpriyanka.sahni
#57 After_Single.png257.74 KBpriyanka.sahni
#55 interdiff-3102402-53-55.txt3.53 KBshaktik
#55 3102402-55.patch7.12 KBshaktik
#53 3102402-53.patch5.08 KBshaktik
#51 no_weight_for_single_media.png350.51 KBambuj_gupta
#49 3102402-49.patch6.3 KBmrinalini9
#46 3102402-46.patch5.97 KBkishor_kolekar
#44 interdiff_33-44.txt2.22 KBswatichouhan012
#44 3102402-44.patch6.33 KBswatichouhan012
#39 Screenshot 2020-02-18 at 2.18.10 PM.png297.09 KBmayurgajar
#34 interdiff-29_33.txt2.25 KBhash6
#34 3102402-33.patch5.06 KBhash6
#31 Screenshot 2020-02-13 at 4.01.54 PM.png142.85 KBhash6
#31 interdiff-29_31.txt4.11 KBhash6
#31 3102402-31.patch7.12 KBhash6
#29 3102402-29.patch5.1 KBswatichouhan012
#29 interdiff_25-29.txt4 KBswatichouhan012
#27 Screen Shot 2020-01-21 at 3.26.36 PM.png159.36 KBsnehalgaikwad
#25 interdiff-3102402-23-25.txt1.82 KBsathish.redcrackle
#25 3102402-media-field-weight-issue-25.patch5.24 KBsathish.redcrackle
#23 3102402-media-field-weight-issue-23.patch4.76 KBsathish.redcrackle
#23 interdiff-3102402-21-23.txt653 bytessathish.redcrackle
#22 Screen Shot 2020-01-08 at 23.20.40.png245.84 KBlauriii
#21 interdiff-3102402-14-16.txt750 bytessathish.redcrackle
#21 3102402-media-field-weight-issue.patch4.37 KBsathish.redcrackle
#19 interdiff-3102402-14-15.txt1.73 KBHardik_Patel_12
#19 3102402-15.patch2.76 KBHardik_Patel_12
#16 after apply patch - multivalue value.png146.27 KBsathish.redcrackle
#16 after apply patch - single value.png112.49 KBsathish.redcrackle
#16 before apply patch.png120.05 KBsathish.redcrackle
#14 3102402-14.patch4.27 KBidebr
#14 interdiff-11-14.txt5.11 KBidebr
#11 interdiff-3102402-9-11.txt1.37 KBHardik_Patel_12
#11 3102402-11.patch2.32 KBHardik_Patel_12
#9 interdiff-3102402-7-9.txt825 bytesHardik_Patel_12
#9 3102402-9.patch1.9 KBHardik_Patel_12
#7 3102402-7.patch2.04 KBHardik_Patel_12
#7 interdiff-3102402-4-7.txt1.2 KBHardik_Patel_12
#4 interdiff-3102402-3-4.txt180 bytesHardik_Patel_12
#4 3102402-4.patch1.04 KBHardik_Patel_12
#3 3102402-3.patch1.04 KBHardik_Patel_12
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

Devipriya Rajamanickam’s picture

Kindly provide screenshots for better understanding.

Hardik_Patel_12’s picture

Status: Active » Needs review
FileSize
1.04 KB

Kindly follow a new patch.

Hardik_Patel_12’s picture

FileSize
1.04 KB
180 bytes

Kindly follow a new patch

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

Status: Needs review » Needs work

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

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
2.04 KB

kindly follow a new patch.

Status: Needs review » Needs work

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

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
1.9 KB
825 bytes

kindly review a new patch

lauriii’s picture

It seems like there are some coding standard violations. You can run composer run phpcs core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php to see those.

Hardik_Patel_12’s picture

Coding standard violations are resolved in new patch

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

Status: Needs review » Needs work

The last submitted patch, 11: 3102402-11.patch, failed testing. View results

idebr’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
4.27 KB

Attached patch updates the test coverage.

Status: Needs review » Needs work

The last submitted patch, 14: 3102402-14.patch, failed testing. View results

sathish.redcrackle’s picture

I have reviewed the patch and attached the screenshots before and after patch applied.

Error screenshot
before patch applied

After patch applied
after patch applied - single

after patch applied - multivalue

sathish.redcrackle’s picture

Status: Needs work » Reviewed & tested by the community
idebr’s picture

Status: Reviewed & tested by the community » Needs work

The patch in #14 still has a failing test.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
2.76 KB
1.73 KB

kindly follow a new patch.

Status: Needs review » Needs work

The last submitted patch, 19: 3102402-15.patch, failed testing. View results

sathish.redcrackle’s picture

Status: Needs work » Needs review
FileSize
4.37 KB
750 bytes

Fixed the test issue in Comment #14 and attached the modified patch file.

The test failed because the media popup is not triggered so the second media is not attached to the form. Also attached the interdiff file.

lauriii’s picture

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

The weight field is still visible when JavaScript is disabled.

sathish.redcrackle’s picture

I have fixed the issue mentioned on comment #22 and attached new patch.

phenaproxima’s picture

Issue tags: +Needs tests

Thanks, @sathish.redcrackle! Will you also add test coverage to ensure that the weight field doesn't show up when there is only one item?

sathish.redcrackle’s picture

I added the test to check hidden weight field and attached patch.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
snehalgaikwad’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
159.36 KB

Patch in #25 is working perfectly, now 'Show media item weights' doesn't appear if only one media can be attached to the field.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

This looks very good to me, except for some minor points and requests for simplification/clarification. I'm removing the "Needs tests" tag, since we certainly have adequate coverage now. Thank you!

  1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -354,6 +354,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +        '#access' => count($referenced_entities) > 1,
             '#attributes' => [
    ...
               '#title' => $this->t('Weight'),
    +          '#access' => count($referenced_entities) > 1,
    

    The #access expression is repeated, which increases its chance of breaking due to future refactoring. Can we put it in a variable and re-use that?

  2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
    @@ -191,11 +191,14 @@ public function testWidget() {
    +    // The toggle for weight inputs visibility should not be available when
    +    // the field contains a single item.
    

    Nit: The word "inputs" should have an apostrophe after it: "...the toggle for the weight inputs' visibility..."

  3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
    @@ -191,11 +191,14 @@ public function testWidget() {
    +    $weights = $this
    +      ->xpath('//div[(contains(@style,"display: none"))]//input[(contains(@class,"js-media-library-item-weight"))]');
    +    $this
    +      ->assertEquals(0, count($weights), "Weight field should be shown if there is one item.");
    

    This seems unnecessarily complex; I think the following would suffice instead:

    $assert_session->fieldNotExists('Weight', $wrapper);

  4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
    @@ -213,6 +216,13 @@ public function testWidget() {
    +    $assert_session->fieldExists('Weight', $wrapper)->click();
    

    We should add a comment explaining why we click on the Weight field (it's to ensure that the styling doesn't accidentally render it unclickable).

  5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/WidgetAnonymousTest.php
    @@ -60,12 +61,28 @@ public function testWidgetAnonymous() {
    +    $assert_session->elementNotExists('css', '.js-media-library-widget-toggle-weight');
    +    $weights = $this
    +      ->xpath('//div[(contains(@style,"display: none"))]//input[(contains(@class,"js-media-library-item-weight"))]');
    +    $this
    +      ->assertEquals(0, count($weights), "Weight field should be shown if there is one item.");
    

    This also seems overly complicated. Why not just $assert_session->fieldNotExists('Weight')?

swatichouhan012’s picture

Status: Needs work » Needs review
FileSize
4 KB
5.1 KB

Here is the new patch to fix points according comment #28, Kindly review.

hash6’s picture

Assigned: Unassigned » hash6
hash6’s picture

Assigned: hash6 » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
7.12 KB
4.11 KB
142.85 KB

Thanks @swatichouhan012 for the patch ,we need couple of changes
As mentioned by @phenaproxima
#1 #access expression , I have added the complete $weight_access = count($referenced_entities) > 1;
#2 "weight input's" changed to "weight inputs'"
Rest #3, #4 and #5 seems perfect.

After patch weight field & link disappears

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

#31 contains core/modules/layout_builder/tests/src/FunctionalJavascript/BlockLocatorTrait.php which is unrelated to this patch.

hash6’s picture

Assigned: Unassigned » hash6
hash6’s picture

Status: Needs work » Needs review
FileSize
5.06 KB
2.25 KB

Sorry, @alexpott Updated the patch. Please review it again.

Hardik_Patel_12’s picture

Assigned: hash6 » Unassigned
Rangaswini’s picture

Assigned: Unassigned » Rangaswini
Rangaswini’s picture

Assigned: Rangaswini » Unassigned
mayurgajar’s picture

Assigned: Unassigned » mayurgajar
mayurgajar’s picture

Hi @ hash6,

patch #34 apply cleanly LGTM +1 RTBC .

Thanks..!!!

mayurgajar’s picture

Status: Needs review » Reviewed & tested by the community
lauriii’s picture

Status: Reviewed & tested by the community » Needs work

We should also remove cursor: move from .media-library-item__preview in this scenario because the element isn't draggable.

phenaproxima’s picture

Thanks for this patch! I have a few concerns about the tests, though.

-    // Assert that we can toggle the visibility of the weight inputs.
+    // The toggle for weight inputs' visibility should not be available when
+    // the field contains a single item.
     $wrapper = $assert_session->elementExists('css', '.field--name-field-twin-media');
-    $wrapper->pressButton('Show media item weights');
-    $assert_session->fieldExists('Weight', $wrapper)->click();
-    $wrapper->pressButton('Hide media item weights');
+    $assert_session->fieldNotExists('Weight', $wrapper);

I don't understand this change. We are asserting that the "weight" field does not exist, which is correct...but shouldn't we also assert that the toggle itself is not present? (Something like $assert_session->elementNotExists('named', ['button', 'Show media item weights'], $wrapper) should do it.)

+    // It's to ensure that the styling doesn't accidentally render it
+    // unclickable.
+    $assert_session->fieldExists('Weight', $wrapper)->click();
+    $wrapper->pressButton('Hide media item weights');

The comment here is a little strangely worded; how about "Ensure that the styling doesn't accidentally render the weight field unusuable" instead?

     // Ensure that the selection completed successfully.
     $this->waitForText('Dog');
+    $assert_session->fieldNotExists('Weight');

If possible, maybe we should also get the field wrapper element here and assert that it, specifically, does not contain a "Weight" field. Otherwise, this is going to assert that there is no field by that name on the entire page, which is a side effect we probably don't want. :)

     $this->submitForm([
       'title[0][value]' => 'My page',
       'field_unlimited_media[selection][0][weight]' => '0',
+      'field_unlimited_media[selection][1][weight]' => '1',
     ], 'Save');

I'm not sure this change is strictly necessary, but it does help to ensure that the weight fields are working as intended. So I'm fine with keeping it, just thought I'd point it out here.

mayurgajar’s picture

Assigned: mayurgajar » Unassigned
swatichouhan012’s picture

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

Hii, I have update patch wrt comment #41 and #42, kindly review new patch.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kishor_kolekar’s picture

I've re-rolled patch for 9.1

Status: Needs review » Needs work

The last submitted patch, 46: 3102402-46.patch, failed testing. View results

mrinalini9’s picture

Assigned: Unassigned » mrinalini9
mrinalini9’s picture

Assigned: mrinalini9 » Unassigned
Status: Needs work » Needs review
FileSize
6.3 KB

Rerolled patch to 9.1.x.

ambuj_gupta’s picture

Assigned: Unassigned » ambuj_gupta
ambuj_gupta’s picture

Assigned: ambuj_gupta » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
350.51 KB

Steps to Test
1. Enable Media module (with media library).
2. Apply patch #49.
2. Go to "/admin/structure/types/manage/page/fields"
3. Add a media field to the basic page content type.
- Set the limit to 5.
- Set the media type to "Image"
4. Go to "/node/add/page"
5. Click on the Add Media button. And upload a single image.
6. Insert the Image and check the weight field.
7. Edit the above created node and upload multiple images and check the weight field.

Results:
Verified after applying patch #49. and it's working as expected.

1. When there is a single image, the weight field is hidden.
2. When there is more than one image, the weight field is coming.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/claro/css/theme/media-library.css
@@ -478,7 +478,6 @@
 .media-library-item__preview {
   padding-bottom: 34px;
-  cursor: move;
 }

+++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -440,7 +440,6 @@
 .media-library-item__preview {
   padding-bottom: 34px;
-  cursor: move;
 }

+++ b/core/themes/seven/css/theme/media-library.css
@@ -456,7 +456,6 @@
 .media-library-item__preview {
   padding-bottom: 34px;
-  cursor: move;
 }

Is this always correct? I think we still need this when there are more that 1 .media-library-item__preview on the page.

shaktik’s picture

Status: Needs work » Needs review
FileSize
5.08 KB

Hi @alexpott,

yes, I think still need, updated on this patch kindly check.

diff --git a/core/themes/claro/css/theme/media-library.css b/core/themes/claro/css/theme/media-library.css
index 1e78d16d6d..b41f78eac0 100644
--- a/core/themes/claro/css/theme/media-library.css
+++ b/core/themes/claro/css/theme/media-library.css
@@ -478,6 +478,7 @@

 .media-library-item__preview {
   padding-bottom: 34px;
+  cursor: move;
 }

 .media-library-item__status {
diff --git a/core/themes/claro/css/theme/media-library.pcss.css b/core/themes/claro/css/theme/media-library.pcss.css
index f2d74c38e1..78970b4dcd 100644
--- a/core/themes/claro/css/theme/media-library.pcss.css
+++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -440,6 +440,7 @@
 /* Media library entity view display styles. */
 .media-library-item__preview {
   padding-bottom: 34px;
+  cursor: move;
 }

 .media-library-item__status {
diff --git a/core/themes/seven/css/theme/media-library.css b/core/themes/seven/css/theme/media-library.css
index e6c7400360..80217a64bc 100644
--- a/core/themes/seven/css/theme/media-library.css
+++ b/core/themes/seven/css/theme/media-library.css
@@ -456,6 +456,7 @@
 /* Media library entity view display styles. */
 .media-library-item__preview {
   padding-bottom: 34px;
+  cursor: move;
 }
alexpott’s picture

Status: Needs review » Needs work

@shaktik the issue is that cursor: move; is needed when there is more than one item and not when there is only one item.

shaktik’s picture

Status: Needs work » Needs review
FileSize
7.12 KB
3.53 KB

Hi @alexpott,

updated cursor:move; and weight_access to access variable, kindly check.

priyanka.sahni’s picture

Assigned: Unassigned » priyanka.sahni
priyanka.sahni’s picture

FileSize
257.74 KB
374.71 KB
261.08 KB
394.17 KB

Verified and tested by applying the patch #55.It looks good to me.Can be moved to RTBC.

Steps to test -
1. Go to the admin site.
2. Go to admin/modules.
3. Enable Media module (with media library).
4. Go to "/admin/structure/types/manage/page/fields"
5. Add a media field to the basic page content type.
- Set the limit to 5.
- Set the media type to "Image"
6. Go to "/node/add/page"
7. Click on the Add Media button. And upload a single image.
8. Insert the Image and check the weight field.
9. Edit the above created node and upload multiple images and check the weight field.

Before Patch (Single) -
Before Patch

Before Patch (Multiple) -
Before Patch

After Patch (Single) -
After Patch

After Patch (Multiple) -
After Patch

priyanka.sahni’s picture

Assigned: priyanka.sahni » Unassigned
DeepaliJ’s picture

Assigned: Unassigned » DeepaliJ
DeepaliJ’s picture

Assigned: DeepaliJ » Unassigned
Status: Needs review » Reviewed & tested by the community

Verified and tested on local by applying patch #55. Looks good to me.
Can be moved to RTBC.
Refer #57 for steps and screenshots.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -342,10 +342,12 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      $access = count($referenced_entities) > 1;
    

    $access is not a great name here. $weight_access was better... but $multiple_items would be even more descriptive. Also it should be defined outside of the if.

  2. +++ b/core/themes/claro/css/theme/media-library.css
    @@ -478,9 +478,12 @@
     .media-library-item__preview {
       padding-bottom: 34px;
    -  cursor: move;
     }
     
    +.move-cursor .media-library-item__preview {
    +  cursor: move;
    + }
    

    So the original cursor move causes another problem - which this patch happens to fix. It results in a move cursor on the media library dialog but the items can not be moved. So that's a good thing.

    However it is possible to add the move cursor without adding another class...

    .field--widget-media-library-widget .media-library-item__preview {
      cursor: move;
    }
    
    .field--widget-media-library-widget .js-media-library-item:only-child .media-library-item__preview {
      cursor: inherit;
    }
    

    That CSS will ensure that the move cursor only appears when there are multiple items.

    We can use this in seven too I think.

shaktik’s picture

himanshu_sindhwani’s picture

Assigned: Unassigned » himanshu_sindhwani

will work on the feedbacks mentioned n #61

himanshu_sindhwani’s picture

Status: Needs work » Needs review
FileSize
6.83 KB
3.71 KB

Updated the patch according to #61.

himanshu_sindhwani’s picture

Assigned: himanshu_sindhwani » Unassigned
alexpott’s picture

+++ b/core/themes/claro/css/theme/media-library.css
@@ -481,6 +481,10 @@
   cursor: move;
 }

+++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -443,6 +443,10 @@
   cursor: move;
 }

+++ b/core/themes/seven/css/theme/media-library.css
@@ -459,6 +459,10 @@
   cursor: move;
 }

As per these cursor: move; also need to be more specific as this only applies to the .field--widget-media-library-widget - atm this CSS breaks the cursor on the media item listing when it is in the media library modal. See #61.2

himanshu_sindhwani’s picture

ambuj_gupta’s picture

Assigned: Unassigned » ambuj_gupta
ambuj_gupta’s picture

Assigned: ambuj_gupta » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
257.54 KB

Tested and verified by the apply patch #67. And it's working as expected.

alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev
Category: Task » Bug report
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
index eef4a64d13..17e72a0af2 100644
--- a/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
@@ -216,7 +216,7 @@ public function testWidget() {
     $wrapper = $assert_session->elementExists('css', '.field--name-field-twin-media');
     $wrapper->pressButton('Show media item weights');
     // Ensure that the styling doesn't accidentally render the weight field
-    // unusuable.
+    // unusable.
     $assert_session->fieldExists('Weight', $wrapper)->click();
     $wrapper->pressButton('Hide media item weights');
 

Fixing spelling mistake on commit.

PHPCS: core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php passed

FILE: ...ary/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 194 | ERROR | [x] No space found before comment text; expected "// /
     |       |     The toggle for weight inputs' visibility should
     |       |     not be available when" but found "/// The toggle
     |       |     for weight inputs' visibility should not be
     |       |     available when"
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed coding standard too.

Backported to 9.0.x as a bugfix.

  • alexpott committed da8e7f6 on 9.1.x
    Issue #3102402 by Hardik_Patel_12, sathish.redcrackle, hash6,...

  • alexpott committed ba4ed74 on 9.0.x
    Issue #3102402 by Hardik_Patel_12, sathish.redcrackle, hash6,...

Status: Fixed » Closed (fixed)

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

jrb’s picture

These changes caused the JavaScript bug described in #3327234: Dragging single media thumbnail on edit page causes JavaScript error because the expected weight field was no longer in the markup and available to the drag-and-drop code.