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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RainbowArray’s picture

Issue summary: View changes
FileSize
1.6 KB

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

RainbowArray’s picture

Issue summary: View changes
jayeshanandani’s picture

Patch works as per summary.

jayeshanandani’s picture

FileSize
189.17 KB
187.62 KB

Uploading images.

Before patch:

After Patch:

jayeshanandani’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

The 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:

+    foreach (array('alt', 'title') as $key) {
+      if (isset($variables[$key])) {
+        $controlling_img_attributes["#$key"] = $variables[$key];
+      }
+    }
+    $output[] = '<img' . new Attribute($controlling_img_attributes) . ' />';
Eli-T’s picture

Component: picture.module » responsive_image.module
joelpittet’s picture

That'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...

RainbowArray’s picture

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

botanic_spark’s picture

Assigned: Unassigned » botanic_spark
RainbowArray’s picture

Assigned: botanic_spark » Unassigned

Follow-up issue: #2227435: Remove noscript from picture element markup based on comment #8

mariancalinro’s picture

Status: Needs work » Needs review
FileSize
1.59 KB

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

RainbowArray’s picture

Patch looks good to me. Thanks mariancalinro!

mariancalinro’s picture

FileSize
629 bytes

Also the interdiff.

jayeshanandani’s picture

FileSize
2.63 KB
1.77 KB

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

jayeshanandani’s picture

FileSize
3.04 KB

Rerolled #14. Now will work on remaining tests.

attiks’s picture

Coming 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

RainbowArray’s picture

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

attiks’s picture

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

RainbowArray’s picture

Status: Needs review » Postponed

This 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).

attiks’s picture

Status: Postponed » Closed (duplicate)