Media library thumbnails are blurry/skewed in IE11

🍿This is amazingly obscure. I love the throwback to a decade ago here! 🤩🤓🤦‍♂️

1. +++ b/core/modules/media_library/media_library.module
   @@ -65,6 +66,24 @@ function media_library_theme() {
   +  // background image that seen only in IE11.
   

   🤓 Übernit: that seen only in IE11. → that is seen only in IE11.

2. WHY_SO_BLORY.png

   😂👏👏

   Thanks for this thoroughly entertaining issue, @bnjmnm 🙏

I was asked to chime in here about MoSCoW.

From what I can see through perusing a few links, IE 11 is an older version sitting at ~3% of usage overall.

And the nature of this bug is that it doesn't prevent you from using the media library, though it does look like shit.

So I'd rate this a "should have," personally. It would be really nice to fix it if we can, since if you're stuck on that browser version because of your organization/company/whatever you would have a far more pleasant experience. But not worth holding up the entire feature as a whole, IMO.

I would make one request: can we perhaps have a comment or todo or something to delete this extra code when IE11 support is dropped? Otherwise we'll forget for sure.

So I'd rate this a "should have," personally. It would be really nice to fix it if we can, since if you're stuck on that browser version because of your organization/company/whatever you would have a far more pleasant experience. But not worth holding up the entire feature as a whole, IMO.

Yup, we'll definitely fix this. The question is mostly whether or not we need to fix this in Media Library's bundled CSS before it is moved into Seven (which will happen when we mark the module stable), or if we consider this a bug that can be fixed in Seven after the module is stable (i.e., potentially during the 8.8.x alpha or, at latest, beta phase).

Since it's a should-have and clearly a bug, it seems reasonable that we can land this after the module is stable. So...+1, I guess!

Given #7 + #9, demoting priority.

This is definitely still a bug that needs to be fixed -- we only use minor for typos in docblocks and whatnot.

I agree that it can be a should-have since IE does still work and there's no serious a11y regression or anything.

Module CSS (and internal theme CSS) can be changed in any minor and up to RC to fix bugs. However, is this a rule that would need to be copied into classy as part of the final steps of #2834729: [META] Roadmap to stabilize Media Library? If it would, then the restrictions about changing it after we mark Media stable go up a lot, so we'd potentially be stuck with the broken CSS in Classy until Classy exits core stage left. And that we'd probably want to have done before beta. Still a should-have, I guess, just sucky to be stuck with it. :P

@bnjmnm confirmed that this CSS only needs to go in Seven, not Classy, so we are good to fix it in any minor up to RC. Thanks!

Agreed re: #8. Created follow-up issue for that: #3089196: Remove IE11 work-arounds from Media when core drops IE11 support.
Re-rolled to apply cleanly to the 8.9.x and 8.8.x branches.
Added @todo / @see comments pointing to #3089196.

Not sure what else we're waiting for in here.

I don't have IE11 nor Virtualbox, so I'm not in a position to test this directly. Maybe one more round of manual testing to officially confirm we're cool? We obviously can't write automated tests for this.

RTBC?

Cheers,
-Derek

p.s. Interdiff was confused on this, so I'm attaching a raw diff of the two patch files.

Just applied the patch without problems on a 8.7.7 build.

Built and deployed it to two of our QA environments and the results are perfect!

before:

after:

Thanks a lot everyone!

Great, thanks! Manual testing complete. Removing tag.

Awesome work, all!

Committed and pushed to 9.0.x, 8.9.x, and 8.8.x. Thanks!

I'm sorry... but it fails when we use SVG images...
the element for SVG image has $element[0]['#image_style'] as NULL
so it fails with error message:
Error: Call to a member function buildUrl() on null in media_library_preprocess_field() (line 112 of core/modules/media_library/media_library.module).

what we really need is additional handling for the case when we don't have an #image_style.
the code in the nested conditions should look like

        $uri = File::load($fid)->getFileUri();
        if ($element[0]['#image_style']) {
          $image_url = ImageStyle::load($element[0]['#image_style'])->buildUrl($uri);
        }
        else {
          $image_url = file_create_url($uri);
        }
        $variables['attributes']['style'] = "background-image: url($image_url); background-repeat: no-repeat; background-position: center;";

or.. do we need to open a separate issue on that?

or.. do we need to open a separate issue on that?

It'd be lovely if you could do that :)

99% of Drupal sites don't allow SVGs to be uploaded because of the inherent security concerns. So this issue mitigated it for 99% of Drupal sites. (Not to mention only a tiny percentage of people use IE11.)

Sorry, all, I had to roll this one back. The fix here was causing double thumbnails to appear in non-Seven themes, e.g.

Tagging this bug fix to be worked on at DrupalCon Amsterdam 2019, and adding some testing instructions.

Also postponing on #3082690: Mark Media Library as a stable core module, since changes made in that issue will make it possible to know if the proposed fix in this issue is adequate (we only discovered the flaw in #14 after applying that patch, which is why this took so long to get reverted).

Media Library is stable, and this is therefore unblocked.

With the EOL of IE11 wonder if this is still relevant?