Problem/Motivation
In #2211831: Removal of alt attribute from [picture] tag we removed the alt and title elements from the picture element, because they should not appear on the picture element, as per the current version of the picture specification (http://picture.responsiveimages.org): several browsers are currently working on implementing the current version of the picture specification.
Because browsers are working on implementing picture element support, we are using a polyfill to mimic the support that the browsers will provide. The polyfill we are using is Picturefill (https://github.com/scottjehl/picturefill), which we have forked to make some changes.
Our current markup for picture looks roughly like this:
<picture>
<source media="some media query" src="some-image.jpg" />
<source media="some other media query" src="some-other-image.jpg" />
<noscript><img src="fallback-image-for-no-js.jpg" alt="Awesome image" title="Awesome image" /></noscript>
</picture>
What Picturefill does is to check which source is the right one to use based on the viewport size (and resolution density, when srcset is involved). Once it finds the right source file to use, it checks to see if there is an img element inside picture, but outside noscript. If not, it creates an img element with the correct src file. It's this img element that people see when viewing the page.
If Picturefill does find an img inside of picture but outside noscript, then Picturefill either changes the src file on this img or adds a src file if a src isn't defined. If the source element had the correct file to use was using srcset, then Picturefill replaces the existing img element with a new img element.
What we've learned is that Picturefill uses the previous alt attribute on the picture element to add an alt attribute to the img it creates when the source element it has selected uses srcset or if an img element doesn't exist outside of noscript.
So that means that currently there is no alt attribute available on the img element created by Picturefill outside of noscript.
The current version of the picture specification requires that an img element exist within the picture element (and outside of a noscript). In order to work with coming browser implementations, we need to have an img element exist without needing to have Picturefill insert the img element, so that the picture element will work with browser preloaders. This is crucial to improving performance and having images load much faster.
For an img outside noscript, if we have a src attribute defined, browsers that don't support picture will download that initial src file before Picturefill has a chance to define the correct src. However, testing has shown that if no src attribute is defined on this initial img, there will not be a double download of image resources. See test 4 results on this page: http://wilto.github.io/srcn-polyfills/.
Twig conversion of the picture element (#1898442: responsive_image.module - Convert theme_ functions to Twig) depends upon us resolving this issue first.
Proposed resolution
Change our markup to roughly the following:
<picture>
<source media="some media query" src="some-image.jpg" />
<source media="some other media query" src="some-other-image.jpg" />
<noscript><img src="fallback-image-for-no-js.jpg" alt="Awesome image" title="Awesome image" />
<img alt="Awesome image" title="Awesome image" />
</picture>
Picturefill can then add in the src of the img outside noscript, and browsers that support picture will be able to do the same thing.
We also need to make a small tweak to our fork of Picturefill:
Change this (beginning at line 95):
// Copy width and height from the source tag to the img element.
_copyAttributes(match, newImg);
picImg.parentNode.replaceChild(newImg, picImg);
To this:
// Copy width and height from the source tag to the img element.
_copyAttributes(match, newImg);
// Copy alt and title from the source tag to the img element.
newImg.alt = picImg.getAttribute('alt');
newImg.title = picImg.getAttribute('title');
picImg.parentNode.replaceChild(newImg, picImg);
This is required so that a source element is using srcset that the newImg created has alt and title attributes.
Remaining tasks
- Write patch
- Review patch
- Write or modify test coverage
- Test patch
User interface changes
This does not change the user interface.
API changes
The img element within picture would be created through our markup rather than being created by Picturefill. Theme developers would still style this img element.
Comment | File | Size | Author |
---|---|---|---|
#15 | 2220865.15.patch | 3.04 KB | jayeshanandani |
#14 | diff-11-13.txt | 1.77 KB | jayeshanandani |
#14 | 2220865.14.patch | 2.63 KB | jayeshanandani |
#13 | interdiff-2220865-1-11.txt | 629 bytes | mariancalinro |
#11 | picture-empty-img-2220865-11.patch | 1.59 KB | mariancalinro |
Comments
Comment #1
RainbowArrayThis should add the empty img element and tweak Pictuefill to work as described above. We will likely need to modify existing tests or write new tests for coverage of this change.
Comment #2
RainbowArrayComment #3
jayeshanandani CreditAttribution: jayeshanandani commentedPatch works as per summary.
Comment #4
jayeshanandani CreditAttribution: jayeshanandani commentedUploading images.
Before patch:
After Patch:
Comment #5
jayeshanandani CreditAttribution: jayeshanandani commentedThe img tag after patch has got #alt attribute in it. That needs to be changed to alt. Probably a minor fix that will require removing of # from key in:
Comment #6
Eli-TComment #7
joelpittetThat's exactly right @jayeshanandani re: #5, also just as a style try not to use double quotes with variables: "$var". It adds extra parsing to PHP (minor I know) and trickier to spot the variable in the string, it's not a drupal coding standards thing, but likely should be...
Comment #8
RainbowArrayThe discussion on this Picturefill issue is pretty relevant here: https://github.com/ResponsiveImagesCG/picture-element/issues/131
The short version is that we might want to expand this to remove noscript from the markup as well, either in this issue or a separate issue.
Otherwise for people that have a browser with picture support, but have JavaScript disabled, they will get two copies of the image downloaded and displayed, which is really bad.
It's important to note that research (https://gds.blog.gov.uk/2013/10/21/how-many-people-are-missing-out-on-ja...) shows that noscript only works when JS is disabled in the browser, which is about 0.2% of traffic. If JS is enabled but not working, noscript doesn't do anything: that's a much larger percentage, about 0.9% of traffic.
So right now noscript provides a benefit to a small number of people: the 0.2% of traffic where JS is disabled in the browser. As browsers start to implement picture support, though, noscript will become a real problem for those people, as it will cause them to see two copies of an image. A better solution is to drop noscript from our markup. That means that 0.2% of traffic will see alt text instead of an image, but that's probably better than getting two copies of an images downloaded and displayed, which slows down page rendering time and screws up the layout.
Comment #9
botanic_spark CreditAttribution: botanic_spark commentedComment #10
RainbowArrayFollow-up issue: #2227435: Remove noscript from picture element markup based on comment #8
Comment #11
mariancalinro CreditAttribution: mariancalinro commentedI fixed the patch in #1 and rerolled it.
I also did manual testing, and it does indeed work, as expected.
If someone else can confirm this with another manual test, I think we can RTBC it.
Comment #12
RainbowArrayPatch looks good to me. Thanks mariancalinro!
Comment #13
mariancalinro CreditAttribution: mariancalinro commentedAlso the interdiff.
Comment #14
jayeshanandani CreditAttribution: jayeshanandani commentedAfter having discussions with @mdrummond and @cottser in irc:
Added missing tests for
<img>
element. There are few things that are missing from the tests:1) If alt isn't set, do we still get alt=” on the image?
2) If title isn't set, does title not appear (it shouldn't).
3) If alt is set with a value, does that value appear in the alt?
4) If title is set with a value, does that value appear on the empty img?
There is a confusion on do we need to check for all those cases or shall we just check the
<img>
element is being generated inside<picture></picture>
or not.Comment #15
jayeshanandani CreditAttribution: jayeshanandani commentedRerolled #14. Now will work on remaining tests.
Comment #16
attiks CreditAttribution: attiks commentedComing from #2260061: Responsive image module does not support sizes/picture polyfill 2.2 and since we are going to update the picture polyfill, I think this one has to be postponed
Comment #17
RainbowArrayThis issue has to be completed to meet the new picture spec. The picture element needs to have an img element, always, and that's what this issue tackles.
Comment #18
attiks CreditAttribution: attiks commented#17, this will be added in #2260061: Responsive image module does not support sizes/picture polyfill 2.2 as well as the test to make sure the img tag is present
Adding noscript is not an option anymore and the fallback image has to use scrset attribute instead of src attribute to avoid double HTTP requests.
This leaves us the copying of alt and title attributes, but this has to be handled by the polyfill.
Comment #19
RainbowArrayThis will likely be unnecessary once #2260061: Responsive image module does not support sizes/picture polyfill 2.2 lands, as that replaces the output of the picture element output to match the current spec (which this issue also tries to do).
Comment #20
attiks CreditAttribution: attiks commentedIs part of #2260061: Responsive image module does not support sizes/picture polyfill 2.2