Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#17 | i2481637-24.patch | 3.23 KB | Jelle_S |
Comments
Comment #1
attiks CreditAttribution: attiks commentedTypo fixed
Comment #2
Jelle_SPatch.
Comment #3
RainbowArrayJust 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.
Comment #4
attiks CreditAttribution: attiks at Attiks commented#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.
Comment #5
attiks CreditAttribution: attiks at Attiks commentedWe 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.
Comment #6
alexpottThe 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?
Comment #7
RainbowArrayJust 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.
Comment #8
RainbowArrayI 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.
Comment #9
Wim LeersIE 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.
Comment #10
Wim LeersI wonder if #2580695: Prevent responsive image without picture element from causing double downloads of images is the better solution here?
Comment #12
Jelle_S@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?
Comment #13
attiks CreditAttribution: attiks at Attiks commented#12 You're right, now is a good time to fix this and follow the standards
Comment #15
Jelle_SRE #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.
Comment #16
Jelle_SRerolled patch.
Comment #17
Jelle_SForgot to update some comments. New patch.
Comment #18
attiks CreditAttribution: attiks at Attiks commentedComment #19
attiks CreditAttribution: attiks at Attiks commentedNice one, we should definitely do this, so are mark-up is valid and we'll follow the standard
Comment #20
attiks CreditAttribution: attiks at Attiks commentedComment #21
RainbowArrayI 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.
Comment #22
alexpottReading 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.
Comment #24
attiks CreditAttribution: attiks at Attiks commented#22 It's not really necessary to add to 8.2, impact is minimal