Follow-up to #2260061: Responsive image module does not support sizes/picture polyfill 2.2

We have to decide what we want

  1. Use the polyfill and add support for responsive images to as many browsers as possible
  2. Use the picture tag and img + srcset to be as compliant as possible

1/ Polyfill

  • Most visitors will see a responsive image
  • We load extra javascript
  • We abuse the srcset attribute on img for the fallback

2/ Plain

  • People using an older browser, will only see the fallback.
  • We can reduce the amount of javascript
  • We output the fallback using the src attribute as it is meant to be

For reference:

Current browser status (July 31st 2015):

Picture

srcset

Srcset

  • IE and Safari is starting to implement srcset

Comments

attiks’s picture

Issue summary: View changes
odinho’s picture

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.

attiks’s picture

If anybody wants to test this, patch attached, assuming the parent issue gets committed first

mdrummond’s picture

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

attiks’s picture

To make it clear, the patch is only here so it can be tested, we need to decide first

mdrummond’s picture

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.

mdrummond’s picture

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.

attiks’s picture

This issue is not to block all the others, we continue adding picture as planned, but the concern was raised on the mailing list that sites will be over using the picture tag.

Drupal 8 in the current state (even after the update to the 2.1 polyfill) will only output the picture tag. We added an extra formatter to the picture module for img + sizes + srcset, but in it's current state it is also outputting the picture tag (to avoid the double download problem). Giving this we are not really promoting the use of sizes and srcset, so other framework might look at Drupal and copy paste our approach.

Since Drupal 8 will be used for a lot of different types of sites, we need to find a sensible default, one that follows the specs as close as possible without making everybody's life difficult.

We should at least consider the following (keeping in mind that we cannot change during point releases)
- How, where can we output an img tag with sizes/srcset?
- How do we handle the non supporting browsers?
- Do we have to include the polyfill by default, make it optional or don't include it at all?
- DO we use src or srcset on the fallback image?

PS: This is not about the extra payload of the polyfill, it isn't going to make a big difference, although the less requests, the better.
PSS: Our company disables all javascript and css coming from core/contrib and we only add the pieces we really need, so personally I'm fine with ditching the polyfill, but this doesn't mean that we don't use it, we include it in the theme layer when needed.

Wim Leers’s picture

Priority: Normal » Major

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.

mdrummond’s picture

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.

catch’s picture

Issue tags: -Usability, -revisit before release candidate +minor verion target

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.

attiks’s picture

attiks’s picture

attiks’s picture

catch’s picture

Priority: Major » Critical
Issue tags: +Needs D8 critical triage

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.

catch’s picture

Title: [Meta] Remove Picture polyfill » Remove Picture polyfill

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.

attiks’s picture

This will be a hard decision, so I rather wait as long as possible so we know what browser builders are going to do, especially IE and Safari. If we release D8 today I would postpone it till 8.1, if we release in 6 months (and Win10/IExx is released for instance) we might remove the polyfill.

Is there a tag so we can review this before creating a RC

catch’s picture

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.

attiks’s picture

Priority: Critical » Major
Issue tags: +revisit before release candidate

Deal

xjm’s picture

joelpittet’s picture

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?

joelpittet’s picture

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

Was in my serendipitous email today

mdrummond’s picture

Version: 8.0.x-dev » 8.1.x-dev

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.

webchick’s picture

Great! Thanks, Marc!

mdrummond’s picture

Status: Needs review » Postponed
webchick’s picture

Issue tags: -minor verion target +minor version target

Fixing tag.

xjm’s picture

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

attiks’s picture

#27 It's not really a BC breaks, it only influence what the browser will render, if we do this in 8.1 (or 8.2) we can always make it configurable, so site owners can decide if they want to enable it or not. We already deployed several sites without the picture polyfill, and so far we got no complains, but it depends on the site / target audience.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nod_’s picture

Status: Postponed » Active

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

attiks’s picture

All in favor, as said in #28 we can make it optional and by default no polyfill, for existing installs we can keep using the polyfill. Should be possible to add an update hook that enables the option

Wim Leers’s picture

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:

attiks’s picture

We can remove it, the site will still work, but we save 95% of the people from downloading a polyfill they don't need

webchick’s 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.

catch’s picture

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.

droplet’s picture

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’s picture

#35 as said in #36 people will get the fallback, so it might be they download an image that is too big for their screen.

I create a patch that will add a setting to suppress the polyfill, for existing sites nothing will change, the site admin/developer can disable it using config.

attiks’s picture

Assigned: Unassigned » attiks
Wim Leers’s picture

@attiks++

attiks’s picture

Status: Active » Needs review
FileSize
2.65 KB

First pass

joelpittet’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 41: i2343351-40.patch, failed testing.

mdrummond’s picture

Status: Needs work » Postponed

"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.

joelpittet’s picture

What is it postponed on Marc?

nod_’s picture

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.

nod_’s picture

Title: Remove Picture polyfill » Make picture polyfill optional

More accurate.

nod_’s picture

Status: Postponed » Needs review

The last submitted patch, 3: i2343351-3-do-not-test.patch, failed testing.

mdrummond’s picture

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.

Jelle_S’s picture

@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?

attiks’s picture

#51, the UI was mentioned in #42 and you're both right, we need a UI so people can easily alter it.

#51 I think expanding the help and outputting a message will make this more clear to people. ++

@mdrummond we're not removing it, we're just changing the default behavior, I understand your concerns, but the negative impact will be very minimal. If we as system builders don't pursue new technologies and 'force' people to adopt, it will even take longer, picture was designed to be backwards compatible from the beginning.

attiks’s picture

#42 I'll fix it in the next patch, need to fix the schema as well and add a UI

attiks’s picture

New patch, without UI

Jelle_S’s picture

Status: Needs review » Needs work
+++ 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 :-)

The last submitted patch, 54: i2343351-54.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

#56 Thanks

Jelle_S’s picture

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

Still a small error:
type: boolean

Status: Needs review » Needs work

The last submitted patch, 57: i2343351-57.patch, failed testing.

attiks’s picture

Status: Needs work » Needs review
FileSize
7.07 KB

Patch with a UI, tab is still missing, but battery is dying on me

Jelle_S’s picture

Status: Needs review » Needs work
  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.')

attiks’s picture

#61 Care to upload a patch, might be easier/faster?

Jelle_S’s picture

Assigned: attiks » Jelle_S

I'll see what I can do.

mdrummond’s picture

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.

Jelle_S’s picture

Assigned: Jelle_S » Unassigned
Status: Needs work » Needs review
FileSize
6.58 KB
3.73 KB

I think this should do the trick.

attiks’s picture

+++ b/core/modules/responsive_image/src/Form/SettingsForm.php
@@ -0,0 +1,57 @@
+ * Defines a form that configures devel settings.

devel => responsive image

Small copy/paste error (from me), will fix it later, thanks for the patch @Jelle_S

catch’s picture

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%.

attiks’s picture

I think it's better to stop juggling with numbers, the important part is to make this optional, I don't really care if it's on by default or not, but there should be an easy way to disable it. For reference, we don't use the polyfill anymore since more then a year, the same goes for the use of flexbox, we're using it for almost 2 years without any fallback, and never got any complaints.

I'll leave the patch as it is, until there's a decision if the default should be on or off, happy to continue working on it after that decision is made.

mdrummond’s picture

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.

Jelle_S’s picture

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.

joelpittet’s picture

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

attiks’s picture

Assigned: Unassigned » attiks

will change it today

attiks’s picture

Assigned: attiks » Unassigned
Issue tags: +Dublin2016
FileSize
6.01 KB

Switched the default to false, fixed the typo in the comment

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Wim Leers’s picture

  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.

mdrummond’s picture

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.

droplet’s picture

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

Jelle_S’s picture

Jelle_S’s picture

#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...).
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

alexpott’s picture

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.

mdrummond’s picture

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?

attiks’s picture

There is a UI

Edit: missed Alexs last comment, I think we can leave the UI, will add a test for it

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.