In the picture module a problem was discovered with using srcset in the fallback image, so we need to change this and use src in the fallback.

The disadvantage is that there might be a double download when using the polyfill.

Related contrib issue #2449033: Images in Chrome always stretched to 100% width

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

Jelle_S’s picture

Status: Active » Needs review
FileSize
3.08 KB

Patch.

RainbowArray’s picture

Just to be clear, this will cause double downloads in browsers that do not support the responsive images spec, correct?

Support is increasing, but just want to be clear on the tradeoff we're making. Particularly since another issue would make the sizes attribute required, which would also help address the bug in the related issue.

attiks’s picture

#3 Yes it might cause a double download, but once picturefill kicks in, it will get cancelled, so it's not going to be that bad.

Most browser have native support already, only old IE and safari might have this problem.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

We should do this, since outputting the image without an src attribute is actually violating the HTML standard, since - at this moment - the minority of the browsers have no support for picture or srcset, we should start following the standard.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

The double download feels really wrong - especially since we're trying to be mobile first. Exactly which old versions of IE and Safari have the problem and do we even support them?

RainbowArray’s picture

Just chatted with some of the RICG folks in IRC. This article was mentioned, which does a really good job of explaining why leaving src off is still probably the right thing to do: https://www.filamentgroup.com/lab/to-picturefill.html

Yes, it's invalid, but it prevents double downloads from browsers that don't support picture and friends. Whereas there are few to no consequences to being technically invalid as per the spec. There are a lot more browsers that support the new standards, but overall traffic from browsers that support vs don't support picture? Maybe fifty-fifty. Certainly not an overwhelming majority.

So to me, this seems like not the right tradeoff to make at this time.

Especially since the original issue that was reported was due to sizes not being present alongside srcset. However we already have another issue that would solve that: #2481635: sizes is now mandatory in the spec. I think this issue should probably be marked either duplicate or won't fix, based on this. But I'd like to hear from attiks before doing so.

RainbowArray’s picture

Status: Needs review » Postponed

I think this is worth reconsidering if it gets to the point where browser support for responsive images is overwhelming, but for now this does not seem like the right tradeoff to make.

Wim Leers’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Postponed » Active

Exactly which old versions of IE and Safari have the problem and do we even support them?

IE 10 & 11 don't support it: http://caniuse.com/srcset. IE Edge does support it, but that's 1% vs 5–6%.

Though I defer to the Responsive Image wizards @mdrummond, @attiks and @Jelle_S to make a decision here. Let's see if the time is right, now that we're ~6 months further.

Wim Leers’s picture

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.

Jelle_S’s picture

@Wim I think #2580695: Prevent responsive image without picture element from causing double downloads of images is an issue on it's own. It's for when the <img> tag is outputted without the <picture> tag. This issue is about the <img> tag inside the <picture> tag that is used as a fallback image.

We have come to a point where all browsers that don't support the <picture> tag also don't support srcset (http://caniuse.com/#search=srcset and http://caniuse.com/#search=picture).

Which means that if #2343351: Make picture polyfill optional gets in, people can choose what to do with older browsers:
Leave picturefill on, might cause double downloads in browsers without support (might, because when picturefill kicks in it cancels the download).
Turn picturefill off, which will not cause double downloads.

Any thoughts on this? Maybe this is a good point to get this issue going again?

attiks’s picture

Status: Active » Needs review

#12 You're right, now is a good time to fix this and follow the standards

Status: Needs review » Needs work

The last submitted patch, 2: i2481637-2.patch, failed testing.

Jelle_S’s picture

RE #12: After re-reading #2580695: Prevent responsive image without picture element from causing double downloads of images I have to reconsider it being an issue on it's own, they are very much related. As I said there, we can not fix both these issues, it's either one or the other. Requeued patch from #2 for testing against the 8.3.x-dev branch.

Jelle_S’s picture

FileSize
2.29 KB

Rerolled patch.

Jelle_S’s picture

FileSize
3.23 KB

Forgot to update some comments. New patch.

attiks’s picture

Status: Needs work » Needs review
attiks’s picture

Status: Needs review » Reviewed & tested by the community

Nice one, we should definitely do this, so are mark-up is valid and we'll follow the standard

attiks’s picture

Issue tags: +Dublin2016
RainbowArray’s picture

I closed #2580695: Prevent responsive image without picture element from causing double downloads of images in favor of proceeding with this issue. Yes, I raised concerns about the double downloads in the past. Right now that would only be a potential issue in IE11 and Opera Mini. That's certainly important, but since we are looking at the very least providing an option to disable Picturefill, I think it makes sense to move forward with going with src instead of srcset for the fallback at this time.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Reading this issue and the related issue convinces me this is the best choice. Committed 40b2b07 and pushed to 8.3.x. Thanks!

I wonder if we should commit this to 8.2.x - however the fact that #2343351: Make picture polyfill optional won't make it in to 8.2.x gives me pause.

  • alexpott committed 40b2b07 on 8.3.x
    Issue #2481637 by Jelle_S, attiks, mdrummond, Wim Leers: Use src in...
attiks’s picture

#22 It's not really necessary to add to 8.2, impact is minimal

Status: Fixed » Closed (fixed)

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