Updated: Comment #44

Problem/Motivation

The W3C published a working draft of the picture element February 26, 2013. Work is ongoing to finalize the specification of the picture element.

Yoav Weiss built a partial implementation of the picture element for the Chromium browser. A number of browser developers are meeting in Paris September 10, 2013 to discuss srcset, picture and client hints to decide how to move forward. Hopefully this will help push towards browser implementations of the picture element.

Until that time, however, the picture element doesn't actually function in most browsers. So, because the picture element might still change, the recommended approach is to use a polyfill that uses span elements rather than directly placing picture and source elements into HTML. If sites use picture and source elements, then potential changes to the specification could break those sites. Span elements, being more generic, are a better option for something to polyfill.

If and when picture is standardized it should be a simple fix to switch picture and source elements back into core. Whether or not that breaks sites depends on how themers/front-end developers use CSS to target those span elements.

There are a couple potential options to polyfill picture when using span elements.

  1. Create our own custom version of picturefill.js to address a few issues specific to Drupal.
  2. Use the weblinc picture polyfill.

    This project has some performance benefits, primarily because it can use a faster matchmedia polyfill called media-match.js. match-media.js is 100x faster matchMedia.js.

    Right now, Drupal uses MatchMedia.js in core: it would be redundant to have two polyfills for matchMedia.

    However, Weblinc removed their dependency on media-match.js (which is where many of the performance improvements are), allowing us to use the custom MatchMedia.js polyfill that is already in Drupal core. That custom polyfill needed some performance improvements with caching. MatchMedia.js has had some recent improvements to performance as well.

    Previously the weblinc approach did not support srcset: the latest version now does. However it still does not support width and height attributes on source elements to prevent reflow.

    What this boils down to is we have options here. We can use the Weblinc version with MatchMedia.js (hopefully with some more important performance improvements). Also, we still have time to switch out the matchmedia polyfill in drupal core to use media-match.js to take advantage of its performance improvements.

Proposed resolution

The current focus is on an adapted version of the span branch of picturefill.js.

There are a number of reasons to use our custom version of picturefill:

  • JS performance improvements suggested by [nod_] as noted in #7 and patched in #10.
  • Title was missing. To stay as close as possible to img, we added it, see https://github.com/ResponsiveImagesCG/picture-element/issues/51. (Please note that Marco Caceres noted that Title is listed as a global attribute, so he didn't feel that needed to be specifically added).
  • Width and height attributes are added to source elements to avoid reflow. See https://github.com/ResponsiveImagesCG/picture-element/issues/50. The RICG has not yet implemented this change, which means that our code output might perform better, but also might not match the developing specification.
  • Use of querySelectorAll, to improve speed. This is a Drupal decision, since we no longer support IE8
  • domready to be inline with other Drupal 8 js. This is Drupal specific and not part of the polyfill

So there are a number of solid reasons why we would want to use this custom polyfill versus some of the other polyfills available from external sources.

Remaining tasks

The patch in #34 appears to be ready to go.

There are a couple of potential improvements that can be made down the road.

#1999312: Add an 'empty image' option for responsive image adds a feature that allows output of empty image sources for certain media queries. Primarily, this would allow removal of extraneous images on small screens as a performance improvement.

#1848500: Use performance optimized matchmedia polyfill would improve the performance of matchmedia through caching and would thus improve the speed of this polyfill.

However neither of those changes should block this from moving forward.

As per the comment in #48, it does look like the picture element will eventually be finalized and implemented in browsers. Once picture and source are natively implemented in browsers, using those elements (rather than polyfill-based spans) will eventually provide faster performance. A polyfill would still be needed to support older browsers without picture/source support. However, it's questionable whether picture/source will be at that point by the time Drupal 8 is ready for release.

Essentially if we don't implement this patch and Drupal 8 sites output picture and source elements in the HTML output, then those sites could break if the definition of picture and source change.

If instead we switch to the span output, as this patch does, then we can still switch back to picture and source once they are finalized. If themes are targeting span elements, however, their CSS might need to be updated to instead target picture and source instead once that change is implemented, either prior to 8.0 or for a future feature release, such as 8.1.

User interface changes

None.

API changes

None.

Original report by [attiks]

Depending on the progress of http://responsiveimages.org/ and the adaptation by browser builders, we might need to change the picture polyfill and replace it by a div/span based polyfill.

Possible candidates are now implemented in the Drupal 7 version of the picture module:

  1. an adapted version of https://github.com/Wilto/picturefill-proposal/tree/div-markup
  2. an alternative implementation from https://github.com/weblinc/picture, but this depends on an alternative implementation of matchmedia The good thing about this one is it performance, as can be seen at http://jsperf.com/matchmedia/11 and http://jsfiddle.net/wV9HZ/2/ but it will mean we need to change
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

Issue created to rewrite weblinc's implementation so it no longer needs media.is

https://github.com/weblinc/picture/issues/7

attiks’s picture

Status: Needs review » Postponed

Quick update:

For the moment we cannot use the real <picture> tag, because consensus on either syntax or implementation by browsers. So we need to change the syntax the a <span> based solution (div might work as well, but will give problems with inline pictures)

Another decision to make is which polyfill we want to use: picturefill.js or weblinc.js, both have their merits, for details you can have a look at the Drupal 7 picture module

Drupal 7 picture module also has an option to output an empty image for certain breakpoints (see #1950196: Option to render nothing at all (or base64 image data)) which might be a good feature for D8 as well. Issue created for core at #1999312: Add an 'empty image' option for responsive image

attiks’s picture

attiks’s picture

Status: Postponed » Needs review
FileSize
8.94 KB

patch using the span based polyfill

nod_’s picture

Status: Postponed » Needs review

Is the JS to review or is it from a polyfill somewhere? If it's ours I got a few nitpicks but otherwise that's good for me.

attiks’s picture

Nod_ external but a review would be great.

nod_’s picture

Status: Needs review » Needs work

As far as perf goes it's a bit worrying. takes 4ms to initialize for one image. Digging in the code there are muliple places where an attribute is read twice. I got it down to 2ms just by deduping .getAttribute() calls. We can discuss if we're going to be using .dataset for this script and have a 2 line fallback for IE*.

Also the created image is added to the dom then messed around with. it should be added at the end. after all the attributes have been messed around with. Also we can remove some processing by using querySelectorAll('span[data-picture]'). And it is important since getAttribute is kinda expensive.

Also might want to speed things up a little by not checking for an image element, since we're controlling the markup, when does // Find any existing img element in the picture element. happens.

and we could use domready instead of the event things at the end maybe?

attiks’s picture

#7 can you upload a patch?

nod_’s picture

sure, working on it

nod_’s picture

Status: Needs work » Needs review
FileSize
9.33 KB

Got it down to 3ms, can probably do a tiny bit better.

attiks’s picture

FileSize
5.37 KB
attiks’s picture

#10 Thanks, changes are looking good, too bad in cannot RTBC it

Status: Needs review » Needs work
Issue tags: -Usability, -mobile, -revisit before beta, -frontend performance, -media queries, -Responsive Design, -Design Initiative, -d8mux

The last submitted patch, core-js-picture-polyfill-1883526-10.patch, failed testing.

attiks’s picture

nod_’s picture

I'd say it's good enough for now.

JohnAlbin’s picture

Status: Needs review » Needs work

Just to clarify: the patch in #10 uses a forked version of picturefill since we don't use the srcset attribute in Picture.module? I'm fine with that, but it should be clearly documented.

Now that nod_ has had a whack at improving the performance of this script, I'm curious about its performance compared to the weblinc/picture polyfill.

Marking this as "needs work" since it needs a comment about this being a modified version of the vendor version.

attiks’s picture

Status: Needs work » Needs review

#16srcset is supported in the picture module as well, but since there's no UI for it, it can only be configured using code. The D7 version has full support for it.

In the mean time, Scott Jehl updated his version as well to use a span based approach, see https://github.com/scottjehl/picturefill, the biggest difference is that we support srcset and width/height on the "source" element.

The speed difference between this polyfill and weblinc/picture is mostly caused by the polyfill for matchmedia, see #1848500-10: Use performance optimized matchmedia polyfill

JohnAlbin’s picture

weblinc/picture 2.0 was released a few days ago. https://github.com/weblinc/picture

Does that affect this issue or #1848500: Use performance optimized matchmedia polyfill?

attiks’s picture

#18 The problem is they don't support scrset for the moment, https://github.com/weblinc/picture/commit/bddbb1039fab9e88234601b9268a07...

The matchmedia polyfill is a separate discussion, both picture polyfills will work with what we have.

JohnAlbin’s picture

Thanks for answer all my questions, Peter! Reviewing the patch now.

JohnAlbin’s picture

Status: Needs review » Needs work

I applied the patch, reinstalled Drupal 8, configured a new PIcture mapping, edited the article content type’s image to use this new mapping, and created some content.

Everything works the way it should and the HTML source shows that it is now using <span data-picture=""> instead of the picture element. Nice!

Doh! Just noticed a regression. The alt attribute from <span data-picture="" alt="Rock!"> is not copied to the img tag. So that's a bug that didn't exist before (I've confirmed it used to work fine.)

attiks’s picture

Status: Needs work » Needs review
FileSize
9.76 KB
1.89 KB

#21 New patch attached that changes all attributes to data- variant

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

Excellent! width, height, and alt attributes are now copied properly to the img tag for all versions of the picture. :-)

I'm sad that we haven’t been able to get a native <picture> element in any browser yet. It was nice that Drupal 8 was the first CMS to support that proposed HTML element, but with code freeze at the end of this month, we can't let Drupal be ahead of the browsers.

At least, we'll be able to quickly revert this commit when a browser does add picture!

catch’s picture

Status: Reviewed & tested by the community » Needs review

Could you confirm if this is still the forked version? If so I couldn't see the comment in the patch.

attiks’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
10.12 KB
611 bytes

I added the comments

webchick’s picture

I won't knock this back from RTBC, but IMO it'd be great to document *why* we are forking this library. There seems to be a bit of an explanation in #16 but I don't quite grok from that why that's worth having to maintain our own custom version of this code. It'd be nice if some kind of explanation was there so we know in future versions of Drupal/Picture.module if/when we need to update this code / remove the fork / etc.

attiks’s picture

Status: Needs work » Reviewed & tested by the community

@webchick: welcome back ;-)

Why we forked the original picturefill:

  1. Title was missing, to stay as close as possible to img, we added it, see https://github.com/ResponsiveImagesCG/picture-element/issues/51
  2. Width and height are added to avoid reflow, see https://github.com/ResponsiveImagesCG/picture-element/issues/50
  3. Use of querySelectorAll, to improve speed. This is a Drupal decision, since we no longer support IE8
  4. domready to be inline with other Drupal 8 js. This is Drupal specific and not part of the polyfill

Where do you want to add this, in the patch?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I've been hesitating on committing for exactly the same reason.

Lets do this here as we've made further changes and document why in our hacked picturefill.js

catch’s picture

It'd be good to have upstream PRs/issues filed for these changes as well if there's a chance they'll be accepted.

attiks’s picture

FileSize
10.36 KB
997 bytes

I updaated post #27:
1 + 2 now have an upstream issue
3 + 4 are Drupal specific

Comments are updated in picture.module, adding them the picturefill.js will increase download size for visitors, so I left them out.

Since this is only a comment change, back to RTBC.

alexpott’s picture

Nice work re the upstream PRs...

Unsure about not commenting why we've modified the js in the js file

/*! Picturefill - Author: Scott Jehl, 2012 | License: MIT/GPLv2 */

suggests that this is an unadapted version... at least to me.

attiks’s picture

So you want the same comments added to picturefill.js as well?

alexpott’s picture

Personally I think the comments should only be in picturefill.js

attiks’s picture

FileSize
10.29 KB
1.89 KB

Your wish is my command ;-)

Status: Reviewed & tested by the community » Needs work
Issue tags: -Usability, -mobile, -revisit before beta, -frontend performance, -media queries, -Responsive Design, -Design Initiative, -d8mux

The last submitted patch, i1883526-34.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

#34: i1883526-34.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, i1883526-34.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +mobile, +revisit before beta, +frontend performance, +media queries, +Responsive Design, +Design Initiative, +d8mux

#34: i1883526-34.patch queued for re-testing.

Wim Leers’s picture

Is this really critical?

What has happened since mid-June in <picture> land?

attiks’s picture

#39 It is critical, we can not ship Drupal while using the <picture> tag, there's a meeting in September with all major browser builders to see which way we're going to go.

RainbowArray’s picture

Catching up here. Looks like some performance improvements have been made here. Have those been contributed upstream back to the canonical picturefill? I'd bet the wider community would appreciate those performance improvements too.

attiks’s picture

#41 The patch removes support for IE8 and it relies on domReady, but maybe parts of it can be used upstream.

catch’s picture

Priority: Critical » Major

Downgrading this to major - I don't see how this blocks the release. We might release with broken picture support, but it'd only be an internal change to fix that and it's not going to result in data loss or fatal errors if it's broke - it's exactly what 'major' is for.

RainbowArray’s picture

I'll write up a more detailed issue summary of this.

RainbowArray’s picture

Issue summary: View changes

Update issue summary to help explain decisions that need to be made to move forward with this patch.

RainbowArray’s picture

I've updated the issue summary based on some conversations regarding the current status of the matchmedia polyfill situation.

We're about a week out from a major summit of browser makers regarding responsive images. Hopefully that conversation will help us to move forward on this. Conversely, if we have opinions we'd like to share with how responsive images should be handled by browsers, now is the time to make our voices heard!

Kendall Totten’s picture

I hope this is helpful for anyone following this thread. Here are some updates based on by W3C Responsive Images Community Group (RICG) meetup a few days ago.
Guy Podjarny's summary mentions that the likely outcome is srcset. And this summary by Flo Preynat implies that both srcset and picture could emerge winners, srcset for its simplicity, and picture for its art direction friendliness, a popular aspect in the developers community.

RainbowArray’s picture

My understanding is that the next step is that the RICG is developing picture as a web component, which is good as it would allow media query selected images to get picked up in the preloader, which is part of the goal of picture.

Srcset is great for resolution switching, not so great for switching based on viewport size. If srcset is going to be used for viewport switching, more improvements need to happen to make it viable.

However, as there is momentum for srcset for resolution switching getting built into browsers, we should look at how to build in srcset support on img elements. Does that need a separate module? Or can that be handled within Picture?

If the web component version of picture moves forward, that too is something we should take a look at. I'm not yet clear on what that would mean for browser support and how that would be polyfilled.

The other thing to keep an eye on is client hints. Google is big on pushing those (and they do have some advantages), but unclear how long it will take for that to become a reality. Srcset for res switching and picture as web component are much more near term.

RainbowArray’s picture

Issue summary: View changes

Clarified the issues with the matchmedia polyfill.

RainbowArray’s picture

Quick update on where we're at with the proposed picture element: the short version is that it looks like the actual picture element is what will become standardized.

For a time, using a web component was considered, but that was dropped because it might not work well for performance. In some cases, a web component could be just as fast as a preloaded image, but not when there are blocking scripts in head, which I'd guess will be true a fair amount of the time.

After that, Tab Atkins proposed a new solution called src-N, which was a set of attributes on the img element. Src-N had a nice component to it called sizes that works really well for providing different width images based on the viewport size. However a number of web browsers didn't like the possibility of multiple enumerated attributes on the img element.

That led, in a rather surprising turn of events, to getting picture back on the table. A different method of speccing out the picture element was proposed. Short version: an img element is now a required child of the picture element, and behind the scenes browsers will replace the src of the img element once source elements are evaluated.

After a lot of work, the objections from browser implementors have been addressed, and implementations are going to be developed.

One outcome of this is that srcset is being implemented first, solely for providing retina versions of images when there is no art direction or viewport size switching involved.

Full implementation of the picture element may take some time. Blink has some subsystems that need to be updated first (the preloader may need to be able to evaluate media queries, for example). However, it does look like this will happen.

I think what this means for Drupal is that unless 8.0 is delayed far longer than expected, picture is not going to be widely available natively in browsers prior to an 8.0 launch. However, whether or not this was the case, we'd probably still need to ship with a polyfill, as there will be browsers around for some time that don't understand picture.

So it probably will make sense to ship with a span-based polyfill for picture, rather than the picture element with a polyfill.

There's still a chance that picture will be standardized enough, and that some non-Blink browsers will ship picture implementations, and that will be enough to get picture standardized prior to the 8.0 launch. I think it's probably likelier that the span-based polyfill will still be the recommended option at the time of the 8.0 launch (towards the end of 2014?).

Assuming all goes well, and picture does get implemented and standardized at some point soon after the 8.0 release, we might want to switch to a picture-element-first solution for an 8.1 release.

Whenever we do ship a picture-element-first approach, the main impact will likely be CSS that would be targeting picture and source elements rather than spans. That could very well impact both core themes and contributed themes.

The advantage of eventually switching to a picture-element-first approach will be speed. Once preloading is built into browsers for picture and source elements, then responsive images will load much quickly than what is possible with a javascript-based polyfill.

While CSS and JS is being parsed, the preloader looks ahead for DOM resources that might need to be loaded: additional CSS, JS and image files. So once the preloader can understand picture and source elements, and the media queries on those elements can be evaluated, then those resources can be added to the queue and downloaded whenever possible, rather than waiting for the DOM elements to be loaded so that the polyfill can process them.

So, using the picture and source elements will eventually be faster than the polyfill, but we don't want to start using the picture and source elements until they are standardized. Otherwise we could have HTML that doesn't fit the standards and won't work quite right. Using proposed HTML elements before they are finalized is a bad practice because it makes it harder for the W3C and WHATWG to standardize, when there are legacy elements on the web that may be written differently than the proposed standard.

Another thing we might eventually want to look at is that the source element looks like it will have a new attribute, sizes, that will allow for much each easier selection of image resources based on viewport size. Sizes will provide a much, much better experience for web authors. However, this may be something for contrib to handle, as sizes works considerably different from the media query based approach that picture is based on.

Eventually we will want a way to support sizes, either in core or contrib, as it will greatly reduce the amount of markup necessary to create picture elements. Less markup equals smaller file sizes equals good things.

RainbowArray’s picture

Issue summary: View changes

Updated the issue summary with a shorter version of the recap in comment #48.

RainbowArray’s picture

Update: Mozilla plans to ship support for picture the first quarter of 2014. Yoav Weiss is working on an implementation for Blink as well. Two implementations is generally the requirement in order to solidify a web standard.

So it's *possible* that picture/source will be usable natively in conjunction with a polyfill prior to the 8.0 launch. However, until those implementations actually ship, the safer bet is still a span-based polyfill.

Easier to change spans to picture/source than to make fixes if the picture syntax changes once developers see how browser implementations work.

catch’s picture

Issue tags: -revisit before beta +revisit before release

Doesn't look like this will be settled prior to beta, I'd make this change at any point really, but if it's possible to update before 8.0 it'd be nice to.

RainbowArray’s picture

As another note, I got the majority of the patch changes working in the D7 picture module with the picturefill branch, using weblinc's media-match. Noticeable speed improvements in IE9: a page with 40 picturefill images on our site was unusable before, but works fine now.

We'll need some sort of polyfill whether we're using picture/source elements or span elements: something with weblinc's speed would be good.

jamestombs’s picture

Just to flag up https://github.com/verlok/picturePolyfill which is apparently faster than picturefill and better supports IE8 although that obviously isn't a requirement.

attiks’s picture

My biggest concern is that it uses a non-standard syntax (JSON instead of text), so not really sure it this is the best way forward, even if it is faster. I will create problems once the picture element is part of the standard.

RainbowArray’s picture

I think we'll want to keep an eye on what's happening with Picturefill (https://github.com/scottjehl/picturefill), as work's being done to make it compatible with the new picture spec (http://picture.responsiveimages.org/).

The revamp is being planned here: https://github.com/scottjehl/picturefill/issues. If we have specific changes we'd like to see that can improve performance, we may want to contribute them upstream. There's nothing that says we have to use Picturefill, of course, but they're some pretty smart folks, and the work here is most closely connected with the revised spec.

Implementation of picture is going into Firefox as soon as this month, and work is being done to get into Chrome, Opera and IE as well. The revamped Picturefill will switch back to using picture/source vs. span elements. So we might be able to do the same. We'll see how things are looking.

attiks’s picture

Issue tags: +Needs reroll

FYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed

sun’s picture

Note that the divergence of this library from upstream came up in:

#2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0

Also note that the library has been moved into /core/assets/vendor/picturefill in #2203435: Move all external libraries from core modules into core.libraries.yml

It would be great if we could contribute our changes to the original upstream library, so that we can replace our fork with the upstream version. I already fear that this fork will cause us bigger maintenance pains in the future.

RainbowArray’s picture

Picturefill is about to get a pretty significant update to to the new syntax for the picture element. Here's the pull request that should get merged soon: https://github.com/scottjehl/picturefill/pull/126.

I checked with Scott Jehl, the creator and maintainer of Picturefill, and he suggested we wait to file our upstream changes once 126 is merged in. I'll keep an eye on it, and once it goes in, I'll check it against our version to see if there are some changes I can put into a pull request.

I think getting as close to the official Picturefill syntax as possible is going to be a lot more maintainable.

Eli-T’s picture

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

Picturefill has now been refactored to use the revised picture element rather than the span-based approach.

https://github.com/scottjehl/picturefill/pull/126

I'm going to check if our performance improvement suggestions are still relevant and if so contribute them upstream.

RainbowArray’s picture

It looks like the refactored version of picturefill matches up pretty well with the changes we had made in the patch above. The upstream version of Picturefill has a few differences. It doesn't use domready. And it doesn't use width/height on source elements or the resulting img. Personally I think the latter is a good thing. I'm still not convinced that width/height do anything useful in a fluid layout, and when I've discussed this with the RICG folks, they seem to agree. That's probably a discussion for another issue though.

I found one minor thing which I did a pull request on: https://github.com/scottjehl/picturefill/pull/137

We should probably also look at the matchmedia polyfill they're using. I need to look at the comments above again to refamilarize myself with what we're looking for there. I know I've tested weblinc's matchmedia, and it's much faster, but my memory is that we were considering using our own matchmedia polyfill too.

RainbowArray’s picture

It looks like Paul Irish's mediamatch.js (which is what Picturefill uses) has been updated by David Knight to include the performance improvements in his media-match.js (which he authored for weblinc). https://github.com/paulirish/matchMedia.js/releases

Drupal's mediamatch is being updated here: https://drupal.org/node/2207629.

If mediamatch now has the performance that weblinc's media-match does, that could help settle that question.

sun’s picture

Thanks a ton for studying and researching the situation, @mdrummond!

+1,000 to replacing our fork with the latest upstream library.

I just bumped #2207629: Update matchMedia library to latest release to major, as it seems to be a dependency here.

With both issues combined, we should end up with two literal copies of upstream libraries in Drupal core instead of two nasty forks that we have to maintain ourselves. That is absolutely preferred, at all costs, even in case of a "regression" compared to HEAD, doesn't matter.

RainbowArray’s picture

I confirmed with David Knight that matchmedia.js should now be at least as fast as weblinc's media-match.js, as the latter supports IE8 and down (which isn't needed for D8). He's going to look into doing an updated performance check, but we should be good on that front.

Other things on our to-do list:

1) If we want to use the upstream library, then we need to pull support for putting height/width on picture source elements and the resulting img element. The goal of these were that by telling the browser support how much space an image will take up, it will hold that space and avoid reflow once the image is downloaded. That makes sense in a fixed layout, but not in a fluid layout, where the width and height on the page will very rarely match the width and height of the source image: if they're not the same, there will be reflow. Maybe less reflow, but still reflow. As the upstream library is very unlikely to support width/height, we either can drop support for width/height or fork the upstream library. I don't think the mostly non-existent benefit outweighs the cost of forking. Removing width/height also allows us to simplify some other aspects of the Responsive Image module.

2) Our version of Picturefill uses domready, which is apparently a standard practice with other JS usage in core? I'm not an expert on that, but again, if we want to use domready, we may need to fork Picturefill in order to do so.

3) If we use the refactored version of Picturefill, which supports the actual Picture element, we'll need our markup to match the pattern of the revised Picture spec. The biggest thing we need to do is add an empty img element to the markup. We may also want to drop noscript. Both are discussed here: https://drupal.org/node/2220865.

The short version is that noscript only helps the 0.2% of users who have JS disabled but don't have browser support for picture. As those users' browsers begin to support picture, they'll start seeing two copies of every image that uses picture. That's really bad. Dropping noscript means those 0.2% of users will see alt text rather than an image, but that's better than seeing two images. noscript does nothing for the 0.9% of users whose browser's might process JS incorrectly: they already see alt text instead of an image. So dropping noscript affects only a very small number of users that will be decreasing but helps a subset of users that will be increasing.

I'd love to see us eventually support the sizes attribute for the revised picture spec, but I think we need to take care of these issues, and all the others out there for the Responsive Image and Breakpoint modules, before we can even think of tackling that.

RainbowArray’s picture

Discussed point 2 with sun and nod on IRC. We're using domready since we're already using it elsewhere in Drupal, for example with drupal.js.

However, the bandwidth savings for using domready is only 427 bytes, so not huge. nod is going to file an upstream issue with Picturefill to allow for using domready with Picturefill, possibly as a separate file/build. For the moment, though, we can just using Picturefill's DOM-watching code, and swap in domready support if that becomes an option. Dropping domready for now allows us to defork this.

RainbowArray’s picture

Here's the upstream issue filed by nod. https://github.com/scottjehl/picturefill/issues/138.

This would also allow us to use this in a js behavior rather than calling domready directly.

klonos’s picture

The 4th point under the "Proposed solution" section in the issue summary says:

  • ...
  • Use of querySelectorAll, to improve speed. This is a Drupal decision, since we no longer support IE8
  • ...

I was under the impression that we do not support IE8 when it comes to the admin UI and that we (sort of) DO support IE8 when it comes to the end-user facing part of Drupal (#1993334-15: Add HTML5shiv to Stable and Classy only ). Please clarify and update this part of the issue summary so to avoid confusion/misunderstandings. Thanx.

nod_’s picture

We're already using qSA for scripts that end up on the frontend, ajax.js and active-link.js so no, no IE8 support whatsoever in scripts. The other issue has 100% to do with theming so it's a bit different. For scripts they'll need to use https://drupal.org/project/ie8

RainbowArray’s picture

A few updates:

1) We now have some performance testing results for the updated version of matchmedia.js used in the refactored version of Picturefill: http://jsperf.com/matchmedia/29

The short version is that while matchmedia.js is still slower than media-match.js, it's now only as much as 50% as slow, whereas before it was 100x as slow in some cases. That's a huge improvement and probably good enough that it doesn't provide a good enough reason for us to use a custom solution rather than using the upstream version.

2) The issue filed by nod regarding domready has been updated: https://github.com/scottjehl/picturefill/issues/138

The short version is that the refactored version of Picturefill is now loaded with async and checks the DOM as it is downloaded for picture elements. That should mean they're loaded faster. That time savings is probably worth more than the few bytes that would be saved by using the domready already in use in D8. Again, probably good enough for us to use the upstream version.

3) As for #67, if we use the refactored version of Picturefill, that won't be relevant, as the upstream Picturefill doesn't use querySelectAll. It uses getElementsByTagName, which has more backwards compatibility, although it's a bit slower. Again, probably good enough to use the upstream version.

4) One of the last things we'll need to do to switch to the upstream version is to kill off our usage of width/height on our src and img elements inside picture. I'll work on getting an issue filed on that this week.

Wim Leers’s picture

Issue tags: +JavaScript

The <picture> element is officially coming to both Blink (Chrome + Opera) and WebKit (Safari & others). See https://www.indiegogo.com/projects/picture-element-implementation-in-blink. <picture> is also being implemented for Firefox.

To fill the gap in the mean time, Picturefill 2.0 has been released.

I wasn't involved in this issue, so I'm not sure if this is accurate, but it sounds to me like we should close this issue, create a new one to upgrade to Picturefill 2.0, which seems to incorporate fixes for most if not all of the issues we filed? Together with #2207629: Update matchMedia library to latest release, that should address all our concerns?

attiks’s picture

Status: Needs review » Fixed

I agree, we need a new issue, mainly because it changes a lot, see #2260061: Responsive image module does not support sizes/picture polyfill 2.2

Status: Fixed » Closed (fixed)

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

alexpott’s picture

Issue tags: -revisit before release candidate

Removing "revisit before release candidate" because as @webchick said "we did it".

googletorp’s picture

Issue tags: -Needs reroll