Make picture polyfill optional

First, think about:

Is the web really so bad now? Even if something is called *fallback* it's what everyone is currently using today. So should it not be good enough for some more years (even applicable to only some browsers)?

Some other reasons for doing plain:

* That is how the specification and all work around it has been planned. We expect people to implement it without polyfill, so that's the cowpath.
* Fallback is not bad: it's just the same as what almost every picture on the web currently is.
* It's purely progressive enhancement.
* If a large part of users coming in to the site gets the fallback, that only makes for (even) better incentive to make that work well.
* Having sites actually use the @srcset+@sizes and those only working on user agents that has implemented it make for a better incentive to prioritize the feature for web browsers that hasn't yet (having worked on Presto, I know the mechanism well ;))
* It's a better pattern for the full web; Drupal is an important part of the web and with you guys being so early with this, it would be nice to have others copy a good pattern instead of a less than optimal one.
* I think it would be smart being a bit conservative here in something than will be downloaded and used by a lot of people without a lot knowledge about responsive images.

About the picturefill, as I see it:

* It's not good enough for wide usage; there's a reason we needed to do responsive images in the actual browser engines
* It's completely separate from responsive images, even though respimg *community group* worked on both
* It's been a good place to prototype and test things
* It /can/ work for tightly controlled websites (software such as Drupal is not that :) )
* For drupal specifically, it can even be added as a simple script-tag and indeed incur the double-download, but some people (those doing hard-core art direction, these will be able to figure out how to do that) really want the functionality so they can *add* it (but then in less bad-pattern ways)

So I hope you would go with removing the polyfill, but indeed ship the responsive images code which looks very interesting. I would really like to see what we have in the specification adopted (srcset + sizes specifically for CMS'es, I'm as always a bit afraid of misuse of the media-queries so I'd prefer that to be hidden away).

All in all, Drupal shipping something like this spec-perfect would probably do the web a lot of good.

I'd have pretty big concerns about stripping out the Polyfill. More later.

Here are a few thoughts that jump out at me right away:

Once we are using the current version of Picturefill, we will be using 99% of the current responsive images spec. Yes, we'd be leaving off the src attribute on the img inside picture, but that's what the RICG folks who are behind the picture element are recommending for now to avoid double downloads.

Right now the responsive image elements and attributes are just making their way into browsers. By the time Drupal 8 is released there will be more, for sure, but there is likely to still be a large chunk of the web that could use the polyfill.

Admittedly, that percentage that needs the polyfill will decrease over time. One data point I think we'd need is whether Picturefill can be dropped in a feature release of Drupal 8. That would make a big difference. If Picturefill would need to be supported for the entire lifespan of Drupal, that's a very much different thing than including Picturefill for the next year or two.

The biggest concern I have about dropping Picturefill is what people would likely do with the fallback image.

Responsive images helps to deliver sharp-looking images at different screen sizes and resolutions. A really large image can be shrunk down to fit on a small screen, and it will look good, but it will take up far more bandwidth than necessary. The converse is not true. If the fallback image is a small image file appropriate for a small screen, and that image is in a responsive layout where the image would appear larger on a larger screen, then the image file would possibly get scaled up, in which case it would be blurry, or if max-width is used, then there's just a tiny image that looks out of place in a large layout.

Either way is going to look broken for a lot of people, so I suspect you'd get some pretty large images stuffed in the fallback image, and that defeats the entire point of the responsive images effort.

Making images look great on a wide variety of screen sizes and resolutions, while loading only as much image file as necessary, is way more important than whether or not it is "correct" to leave off the src attribute of the img element inside picture.

I'm not sure where the notion that Picturefill isn't good enough for wide usage is coming from: I haven't had that experience myself.

We're days away from the beta, and we've been working for the last nine months to get Drupal 8 using the latest version of Picturefill to make use of the current version of the spec. Dropping Picturefill at this late date is not what I'd choose. Open to discussion on that though.

Getting some clarification on whether or not Picturefill can be pulled in a feature release would be very helpful. It's certainly possible that at some point we don't need Picturefill, but I don't think we're anywhere near that point yet.

Checked with xjm. We probably wouldn't pull Picturefill after a release candidate is out or in a minor release. Breaking sites probably isn't worth a small reduction in JS download size.

One thing I'd like to get clarification on is how big a footprint Picturefill has for browsers that do support picture. Obviously the JS file would be downloaded, but it's not huge, and it would likely get aggregated with other JS. (Possible a site only has Picturefill for JS, but not likely.) My question is how quickly Picturefill exits once browser support is (hopefully detected).

In any case, I'd strongly suggest that while discussing this is important, I really wouldn't want it to hold us up from closing important responsive image issues like the update to the current version of Picturefill.

We can tweak markup and change this out prior to the release candidate. That likely gives us at least six months to evaluate how well supported this in browsers. If there is substantial support by then, I'd be more inclined to support pulling Picturefill at that time.

I think this issue is at least major and perhaps even critical. For now, marking as major.

We are indeed only days away from beta 1. However, this issue does not need to be finished by then. We still have until the RC 1 to get this done.

It's better to focus our attention on the issues that need to be resolved before beta 1. Let's keep this in the back of our heads and revisit this in 1–2 weeks.

I absolutely agree that we should be using img+srcset+sizes if possible. That's probably going to be the default approach for most people, and our markup should reflect that. Picturefill does polyfill that markup pattern as well.

Making Picturefill optional is an interesting idea. Having a setting for that might be a good idea.

I chatted with the RICG folks on IRC, asking about this. A number thought that dropping Picturefill is worth considering: there are tradeoffs on both sides.

If we do use src on the img for fallback, that could result in double downloads when using Picturefill, so we'd want to advise people to use a small image for that rather than a large one.

That is fine if you or others would strip out core js anyhow. What I do want to make sure of is that for site builders trying this out, it works fairly well out of the box. I think having this work in more than just the cutting edge browsers is a part of that.

An option to disable Picturefill might help to get the best of both worlds. If we get to right before release candidate, though, and browser support is pretty solid for the new responsive image specs, then yes, maybe pulling Picturefill would be worth doing.

After release candidate we could presumably still do the following:

1. Add a configuration option on admin/config/development/performance and default that to on for the standard install

2. Add an optional module that removes it (and also enable that in the standard install)

+ a change notice.

I think we should probably bump this to critical, but I'm not sure to what extent we'd still need to make many of the changes in #2260061: Responsive image module does not support sizes/picture polyfill 2.2.

Also, not really a meta. Either we commit a patch to remove this, or we bump it to a minor release or 9.x, but only one issue to deal with.

There's #1393358: [META-12] Review 'revisit before release candidate' tag and the 'revisit before release candidate' tag. That'd keep #2260061: Responsive image module does not support sizes/picture polyfill 2.2 release blocking, then this issue goes back to major and we check it before a release candidate to see if we want to ditch the polyfill at that point or not.

At the moment srcset is getting love from all browsers, picture is not. Srcset is really only a thing in edge and latest safari although it's a subset of the spec.

I've updated the issue summary with the latest CanIUse data.

@attiks It's a pretty safe bet that we should keep the polyfill and push this to 8.1.x, don't you think?

Oh and wanted to share:
https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/sugge...

Was in my serendipitous email today

I don't think we should remove Picturefill at this time. Browser support is definitely growing, but it is not ubiquitous enough for us to remove Picturefill without responsive images being broken for a good number of users.

Great! Thanks, Marc!

Fixing tag.

I don't think we could actually do this in a minor version since it would be a BC break?

By now everything except IE and Opera mini support the element without polyfill. We should revisit.

Well, IE not supporting it means we can't remove it yet I think?

This is the current status, copy/pasted from http://caniuse.com/#search=picture:

Pardon my ignorance, but can we not do something like:

if (!picture.wahwah) { // something that indicates that the browser supports pictures
  <script>polyfill.js</script>
}

I agree that we can't ignore IE. OTOH I guess I don't know the impact (esp. the end user-facing impact) of not loading the polyfill in IE when picture elements are present.

IE10 dropped support for conditional comments: https://msdn.microsoft.com/en-us/library/hh801214(v=vs.85).aspx

So just loading the polyfill for IE10+ won't be possible, that makes this fixable for IE9 and not IE10 in that sense...

I'd also like to see what actually happens on IE without the polyfill. If it's just that they don't get a responsive image (but they still get the fallback), then I'd personally be in favour of dropping the polyfill - IE shouldn't singlehandedly slow things down for every other browser, as long as it's not properly broken.

I was tried not to comment on it. I want it to be killed, hehe.

I'm worried about older iOS more than IEs. In most reports I've seen recently (From Popular apps, Unity, Apple Official data), there're about 10% iOS still running iOS8 and older.

I can't believe people download the games & apps but didn't browse the websites nowadays. Personally, I preferred to trust these data than the StatCounts (CanIUse.com)

Nowadays, most developers design websites for particular devices (sometimes, we totally drop Android in our projects). I think we can add a $setting['picture-poly'] in settings.yml and turn it off by default. Or creating a new module section: "Depreciated Modules & Features"

@attiks++

+++ b/core/modules/responsive_image/responsive_image.install
@@ -0,0 +1,15 @@
+ * Set suppress_polyfill to true for backward compatibility.

s/true/false/

This typo could be resolved fixed on commit if this passes testbot, otherwise this seems like the correct approach to resolve this.

I'm also concerned about mobile devices mentioned by @droplet in #37 but we also provide a UI checkbox to enable/disable this config?

Other than that RTBC from me.

"Everything except IE11" leaves a pretty big hole. It's not just the picture element, it's the sizes attribute as well. I see no movement in the larger front-end world to move away from using the polyfill at this time. It's not a big performance hit to use this and removing it may cause problems in some browsers. I don't see a lot of good reasons to move forward with removal at this time. Doing so harms Drupal's support for responsive images.

What is it postponed on Marc?

IE 11 is a desktop browser. Windows mobile uses edge, which supports responsive images properly.

I don't think the number of desktop IE11 users on slow connection is significant compared to mobile users.

More accurate.

I think this should be postponed because we are not yet at the point where this polyfill is no longer necessary. I don't see a strongly articulated reason why now is the time for us to make this choice, particularly when doing so has a good chance of making existing sites look broken in some browsers.

@mdrummond: attiks' patch in #41 will not break existing sites, as it sets responsive_image.settings.suppress_polyfill to FALSE.

However on new sites, the default value for this setting is TRUE. To address mdrummond's concerns we could document this in responsive_image_help() and maybe even set a warning message when the module is enabled? This way users / site builders are aware of the setting.

[EDIT]
I also noticed the patch does not provide a UI to change the setting, which might be worth discussing?

+++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
@@ -1,4 +1,11 @@
+  mapping:
+    use_admin_theme:
+      type: suppress_polyfill
+      label: 'Do not use the polyfill'

Should be

 mapping:
    suppress_polyfill:
      type: boolean
      label: 'Do not use the polyfill'

Also, seems to be the wrong interdiff :-)

+++ b/core/modules/responsive_image/config/schema/responsive_image.schema.yml
@@ -1,4 +1,11 @@
+      type: suppress_polyfill

Still a small error:
type: boolean

1. +++ b/core/modules/responsive_image/responsive_image.install
   @@ -0,0 +1,15 @@
   + * @file
   + * Responsive image display formatter for image fields.
   

   Shouldn't this be something like "Install and update hooks for the Resposive Image module." ?

2. +++ b/core/modules/responsive_image/responsive_image.links.task.yml
   @@ -3,3 +3,7 @@ entity.responsive_image_style.edit_form:
   +responsive_image_style.admin_settings:
   +  title: Settings
   +  route_name: responsive_image_style.admin_settings
   +  weight: -10
   

   I think the base_route is required too (could be mistaken)?

3. +++ b/core/modules/responsive_image/src/Form/SettingsForm.php
   @@ -0,0 +1,62 @@
   +/**
   + * @file
   + * Contains \Drupal\responsive_image\Form\SettingsForm.
   + */
   +
   

   The @file docblock should not be present when the file contains a namespaced class. From https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
   "The @file doc block MUST be present for all PHP files, with one exception: files that contain a namespaced class/interface/trait, whose file name is the class name with a .php extension, and whose file path is closely related to the namespace (under PSR-4 or a similar standard), SHOULD NOT have a @file documentation block."

4. +++ b/core/modules/responsive_image/src/Form/SettingsForm.php
   @@ -0,0 +1,62 @@
   +      '#title' => t('Do not use the polyfill'),
   

   $this->t('Do not use the polyfill')

5. +++ b/core/modules/responsive_image/src/Form/SettingsForm.php
   @@ -0,0 +1,62 @@
   +      '#description' => t('Suppress the output of the polyfill and rely on the native behavior of the picture element.'),
   

   $this->t('Suppress the output of the polyfill and rely on the native behavior of the picture element.')

I'll see what I can do.

The current stats on support for picture is 74% globally. Having responsive images not work for 1 in 4 visitors is a huge gap. The polyfill closes that gap. Why would we remove that fix at this time?

If you want to add a UI to allow people to turn off Picturefill inclusion, I have no problem with that. Then people can make this choice as they see fit. But turning Picturefill off by default? I still don't see how that's a good idea.

If this change is made, the first thing I'm going to do is tell people to turn on Picturefill support if they want to use responsive images, as we're not yet at the point where we can afford to skip using the polyfill.

I think this should do the trick.

The current stats on support for picture is 74% globally. Having responsive images not work for 1 in 4 visitors is a huge gap. The polyfill closes that gap. Why would we remove that fix at this time?

It's a (small) performance hit for 74% of requests in return for a (larger) performance improvement for 26% of requests.

As @nod_ points out IE11 is a desktop browser, it won't usually benefit from responsive images, so that could arguably be subtracted from the 26%.

It's not true at all that IE11 won't benefit from the responsive images polyfill as a desktop browser. The standard recommendation is to use a small image as the fallback image. So that means that IE11 will likely see either super small images or image stretched up that are super fuzzy. That's going to make sites look broken in IE11.

It seems like the easy way to resolve this is to provide the UI option to disable Picturefill, but leave it enabled by default for now. I'm not opposed to turning off Picturefill by default at some point, but I don't think we're there yet.

I'm not opposed to the default being that picturefill is still included and can be turned off through the UI. Maybe we should (either in hook_help or in the description of the checkbox) inform the user that turning picturefill off will only really break (=present users with the fallback) things in IE11 (and Opera Mini if that's worth mentioning)? Saves the end user some lookup work.

Let's make it optional and leave it on by default for now.

Looks good to me.

1. +++ b/core/modules/responsive_image/src/Element/ResponsiveImage.php
   @@ -15,12 +15,19 @@ class ResponsiveImage extends RenderElement {
   +    if (\Drupal::config('responsive_image.settings')->get('suppress_polyfill')) {
   +      return [
   +        '#theme' => 'responsive_image',
   +      ];
   +    }
   +    else {
   +      return [
   +        '#theme' => 'responsive_image',
   +        '#attached' => [
   +          'library' => ['core/picturefill'],
   +        ],
   +      ];
   +    }
   

   I'd do:

   $build = ['#theme' => 'responsive_image'];
   
   if (!$suppress…) {
     $build['#attached']['library'][] = 'core/picturefill';
   }
   

   But that's just personal preference. I don't like the same thing to be repeated twice.

2. +++ b/core/modules/responsive_image/src/Form/SettingsForm.php
   @@ -0,0 +1,57 @@
   +      '#description' => $this->t('Suppress the output of the polyfill and rely on the native behavior of the picture element.'),
   

   s/picture/<picture>/

   More importantly, this should this not mention the consequences? Like #70 suggests?

This is not enough to un-RTBC.

It's nice that we give people the choice, but OTOH it's yet another setting.

I'd be somewhat reluctant to put browser-specific information in help text. That's something that in theory could change over time, so unless we're able to update that text in a patch version, that could be problematic.

The IE assumption is wrong or am I missing config or BUG?

1.
Here's the fallback image I got:

 <img property="schema:image" srcset="/drupal8x/sites/default/files/styles/max_325x325/public/2016-09/image.jpeg?itok=CdAwH5OO" alt="test" typeof="foaf:Image" />

missing src ?

(seems no UnitTests covered)

2.
I find no way set it to use `srcset` without picture. It's good default to `srcset` to max iOS supports

#77:

1. This is tested here: http://cgit.drupalcode.org/drupal/tree/core/modules/responsive_image/src... lines 313 to 328.
2. This was added in #2348255: Provide option to use srcset and/or sizes attributes on img tag instead of the picture element. The formatter automatically converts to an <img> tag with the srcset attribute without <picture> when there is only one <source> tag and it has an empty media attribute (see http://cgit.drupalcode.org/drupal/tree/core/modules/responsive_image/res...).

+++ b/core/modules/responsive_image/config/install/responsive_image.settings.yml
@@ -0,0 +1 @@
+suppress_polyfill: false

I think double negatives make life trickier. This should be 'use_polyfill' and the default value should be 'true'.

Also for sites that already have the responsive_image module installed we need to have a update path. There should be a hook_update_N in the .install file that creates the config with the default value. The update path will need a test.

Do we need a UI for this. I'm not convinced. Also if we do the UI added by this patch is untested.

+++ b/core/modules/responsive_image/src/Element/ResponsiveImage.php
@@ -15,12 +15,19 @@ class ResponsiveImage extends RenderElement {
   public function getInfo() {
-    return [
-      '#theme' => 'responsive_image',
-      '#attached' => [
-        'library' => ['core/picturefill'],
-      ],
-    ];
+    if (\Drupal::config('responsive_image.settings')->get('suppress_polyfill')) {
+      return [
+        '#theme' => 'responsive_image',
+      ];
+    }
+    else {
+      return [
+        '#theme' => 'responsive_image',
+        '#attached' => [
+          'library' => ['core/picturefill'],
+        ],
+      ];
+    }
   }

Is this really the right place to have logic? Also I wonder what caches need to be cleared if this setting changes.

Do we need a UI for this. I'm not convinced.

If there's no UI, then how does somebody select whether or not to use the polyfill?

With the advent of dropping IE support in Drupal 10, can't this be deprecated in 9.x for removal in 10.x?

We can't deprecate and still use it in core. Since D9 still supports IE we'll only be able to deprecate in D10, in the meantime we can do #3181050: Add "nomodule" attribute on polyfills for the very minor issue of loading a polyfill for modern browsers.

Marking duplicate of #3263823: Empty out and deprecate drupal libraries which are related to Internet Explorer 11 polyfills in 10.0.x for removal in 11.0.0.