Main refactoring for performance and Spec compilance + many bugfixes and new unit tests

[aFarkas]

New features:

• simple mediaqueries are now also supported in IE8
• Performance (Resize-Performance, "Scales better", Network-Performance see below)
• Does not download a new image,
  • if it has already a better candidate from the same set (maybe needs an option, some people might not understand this and think it's a bug)
  • while window is resized
• Loads new candidates in background for "smooth" Transition between candidates
• addDimensions (option)
• resQuantifier (option)
• the elements parameter can now also handle a DOM node (if an img is passed this is polyfilled, if another element is used this is used as the context)
• Mutationobserver (plugin)
• improved errormessages for bad descriptors, bad sizes and wrong source positioning

Following is the list of fixed bugs in your bug tracker by my changes.

Fixed issues:
#333
#332
#331
#329
#327
#322
#318
#314
#313
#311
#309
#304
#303
#292
#270
#266
#244
#142
#263
#317

[zcorpan]

The reason I care about spec compliance for invalid markup is that the spec's behavior has been carefully designed to enable future extensions. If you have different behavior then you might make it harder to introduce new features in the future.

Another reason is that if you support something that is not supposed to work (e.g. source after img), people might start writing such markup and expect it to work in a native impl, which is bad. (source after img in particular should not work because they might not have been parsed yet when the browser wants to start downloading an image.)

As an example, earlier versions of picturefill evaluated source elements in reverse order despite the spec calling for source order. This might seem harmless but it means pages written against the polyfill would have different behavior compared to a native impl, and worst case browsers would be forced to change to match the polyfill if there is enough legacy content expecting reverse order.

But then again I understand that there are tradeoffs and I don't expect a polyfill to hit perfect compliance.

[aFarkas]

This might seem harmless but it means pages written against the polyfill would have different behavior compared to a native impl, and worst case browsers would be forced to change to match the polyfill if there is enough legacy content expecting reverse order.

I don't think this will happen. People will and should use Chrome 38 and FF 36? to test on the one hand and only use the polyfill to get the same behavior in Safari and IE + other older/odd browsers.

About the descriptor tokenizer, I don't think, that we should care about it today. Until there is no descriptor, which can be used in combination, we don't have to tokenize it.

[scottjehl]

@zcorpan +1 to @aFarkas's sentiments: thanks! It's great you were able to look some parts over and review.

It sounds like @aFarkas and I are in agreement about a practical vs strict implementation. In general, when there's an edge-case situation and addressing it will either increase wire weight or decrease performance, I like to place the incovenience on the developer rather than the user. For example, after a dom mutation of some sort, developers can currently (manually) call picturefill to refresh that portion of the dom. If they're updating the dom in some way already, there's likely a place to add this call. Of course, we can automate that handling with mutation events and behave just like a native implementation, but at the cost of adding quite a bit more code to polyfill that event model. If it's already easy to work around this situation, should we really support that convenience by default in the script? Maybe! And perhaps it's just an event binding in this case anyhow. But in general I like the idea of keeping this script fairly responsible for those downloading and executing it, so these things should be judged case by case.

After a second read, I think I misunderstood a feature or two. The smooth-loading image for example, sounds like a great idea. I thought you meant something different there. Sorry!

Really, most changes here are probably keepers. It's just new and unusual to me to try to evaluate 20 bug fixes in one PR! I'm still not quite sure how to do that, honestly. It seems like this PR touches so many parts of the code that I'm not sure I'm comfortable saying it's all bug-free. And it probably is, I just wish this could be itemized a bit is all.

For example, the height: auto for IE8 is a nice simple fix that should be its own PR. But that's just one line of 16,080 additions and 10,437 deletions to review here (including some build files, of course).

I guess I'm just struggling with how to proceed.

@Wilto @yoavweiss - any ideas?

[aFarkas]

about better errormessages and stricter parsing / devbuild
I have written a proposal for handling errormessages with a better parser in a dev build. This is currently done with uglifys dead code removal. Here are the changes for this: aFarkas@32ed42f and here it is used aFarkas@26997b0. Note that the stricter parsing doesn't change our results, it is only done for the errormessages.

about matchMedia polyfill
As noted above, I have written a basic matchMedia polyfill for min-width and max-width for IE8. Without matchMedia picturefill is quite useless.

I'm open to do this with a plugin (like suggested from @scottjehl), but I would rather go a total different way. Currently picturefill is including matchMedia polyfill directly. I think it is a bad habit to include a third party script directly into another script. Developers should resolve the dependencies themselves. Otherwise a developer doesn't a) not know what's already included and b) ends up with multiple copies of the same script.

Additionally the included matchMedia polyfill is only done for IE9.

My matchMedia polyfill on the other hand could be used by IE8 *and IE9 * and therefore removes any hard dependencies to other scripts. In case a developer needs better matchMedia support in IE9, he/she can still add matchMedia polyfill as a "soft dependency".

You can see my code changes for this here:
aFarkas@109be77

Currently I do this lazy on first call to pf.matchesMedia, I test for native support, if it's supported, I choose the native implementation, then I check for Modernizr's mq check, if it works I use it and if those don't work, I choose the minimal implementation.

What do you think about both proposals?

[aFarkas]

I would like to share some other thoughts, because I'm still improving.

1. polyfill vs. graceul degradation
The poyfill vs. progressive enhancement debate made me think, how the situation can be improved.
I think omitting the src attribute is a reasonable suggestion to avoid doubble requests (most of the time). But the problem is, that picturefill is written in a way, that it handles/treats responsive images with a src attribute very badly and therefore gives a developer no choice other than either use a polyfill and always omit the src attribute or to use the src attribute, but being unable to polyfill specific images inside the project.

Here are three interesting improvements, which I have just added. Consider the following markup:

  <img 
    srcset="http://placehold.it/700x300 700w, 
        http://placehold.it/1400x600 1400w" 
     sizes="350px" 
     src="http://placehold.it/1400x600"
     alt="w with fixed sizes, doesn't make sense ;)" />

A srcset supporting browser will always load the 700w picture, if the devicePixelRatio is 2 or below and so does also picturefill. But the fallback strategy here is using the biggest picture, so the user experience is extremly bad, because the user downloads the big picture, but only sees the small one. This can be simply improved by checking, wether the current source is "good" enough. A simple demo of this can be seen here: http://codepen.io/aFarkas/pen/Dhtfr.

Also the situation can be improved, if the fallback source is using the smallest image and the algorithm is calculating a different source. In this case, we know that the small image is loaded anyway and will serve as "better" first impression. Downloading another source instantly will only decrease the loading time of the page, so a polyfill can become a little bit lazy and wait untill the onload event happens (A demo can be seen here: http://codepen.io/aFarkas/pen/xJeKr). But this one should be really tested inside of a real website with real images, because it's really nice.

A tird possibility for a developer could be to explitly mark an image, so that is not polyfilled: http://codepen.io/aFarkas/pen/ldyoj

it should be clear, that all those improvement strategies also work, if the fallback strategy is using the medium img source of the fallback image.

Last but not least, I think, that lazyLoading in combination with responsive images can become very handy because at the point where an image is loaded the size of the image can be computed by script, so this helps with doubble request, performance and adding the right value to the sizes attribute at once. (will create a demo for this one).

intelligent resource selection
Additionally I have also added a configurable way to manipulate the resource selection. Normally you should do those things only on low bandwith, but I'm doing it in a very smooth way generally (but it can be simply turned off).

"lazyPadding":
Consider you have 1dpr device and the calculated sizes changed from 700px to 980px due to an orientation change:

  <img 
    srcset="http://placehold.it/960x300 960w, 
        http://placehold.it/1200x420 1200w" 
     sizes="dyn was 700 and no is 980"  />

Normally the polyfill would now load a new candidate (i.e.: 1200w). My change is now checking, if there is already an a loaded source from the same set and if yes checks, wether this also satisfies the needed pixel density. But before this check, I give the current image a little advantage by adding some little pixel density. Therefore the new resource selection doesn't download a new image.

"lowRes greedy":
Another adjustment can be seen here, which is similiar to the first one but calculated differently. Let's assume the calcluates current size is 1000px:

  <img 
    srcset="http://placehold.it/960x300 970w, 
        http://placehold.it/1900x420 1900w" 
     sizes="1000px"  />

Due to the fact, that the next right resource is a lot higher, the algorithm starts to greed for a smaller download. The intersting part here is that there is something like a "greeding" factor, the more the browser has to download the more it will greed for a lower resultion image.

Well there is more...

[yoavweiss]

Why is this PR closed? Are we not merging this?

[yoavweiss]

I think we should merge this work into a branch, test against the test suite, test manually for obvious regressions, and if there are none, merge it into master.

A lot of known bugs (including performance issues) are fixed by this work. I have not reviewed it thoroughly, and it's fairly possible it's not perfect (e.g. it might be better to trigger fillImgs using mutation observers where available, and fallback to a timer only when they aren't), but it seems to me that incorporating a bunch of bug fixes (even if the they'd be better as multiple PRs) and adding a maintainer to the project is better than having a needless fork on our hands.

[jansepar]

+1 to getting this merged. The best chance of getting this into the hands of developers is by merging it into picturefill. I can provide assistance with PR review and QA if needed :)

[aFarkas]

This would be wonderful. In the branch of this PR, there is one known bug (thanks to @zcorpan):
https://github.com/aFarkas/picturefill/blob/master/src/picturefill.js#L640

Which should be changed to something like this:
https://github.com/aFarkas/respimage/blob/stable/respimage.dev.js#L705

Since I offered you this PR, I made a lot of changes and improvements based on this one, but on another namespace in the following repository: https://github.com/aFarkas/respimage

And well, it would be great if some of those additional things would be also added. The thing here is, that this PR includes mainly bugfixes and some runtime perf improvements, while the new repo has also included my ideas above (some hugh network perf improvements) and generates a little concpetual shift, which is a little bit explained here:
https://github.com/aFarkas/respimage/blob/stable/how-respimg-works.md

In case, there is a yes for at least some parts, I can work on this. Maybe this is a second PR, after the main bugfixes have landed?

Most of the additional work are improvements for srcset and src attributes. While those things do also apply to the picture element. The picture element "fragments" those improvements. In case of my src attribute change (LQIP) in conjunction with the picture element the situation needs to be further improved and about this special thing, I really would to talk with someone with a strong opinion on how to use the picture markup to discuss the possibilites. ( I also tryed to reach out to you guys through the respimg mailinglist yesterday.). But before that I can/would/should/must write down the problem and my ideas including my personal opinion/interpretation about the picture element. (so let me know if there is time and interest).

@yoavweiss and all
About the native implementation: Above I wrote some parts about "intelligent resource selection" and I really think, something similiar must be also included in native implementations. Here is a demo of a worst case szenario: http://codepen.io/aFarkas/full/tplJE/

I will create another demo with "real world images/examples" soon. To show what is possible.

[yoavweiss]

In case, there is a yes for at least some parts, I can work on this. Maybe this is a second PR, after the main bugfixes have landed?

Great to hear you can work on this (and thanks for all the work you've done so far!).
It is not my call, but I think it'd be best to take in this PR wholesale, and then working on follow-up improvements in separate PRs.

Most of the additional work are improvements for srcset and src attributes. While those things do also apply to the picture element. The picture element "fragments" those improvements. In case of my src attribute change (LQIP) in conjunction with the picture element the situation needs to be further improved and about this special thing, I really would to talk with someone with a strong opinion on how to use the picture markup to discuss the possibilites. ( I also tryed to reach out to you guys through the respimg mailinglist yesterday.). But before that I can/would/should/must write down the problem and my ideas including my personal opinion/interpretation about the picture element. (so let me know if there is time and interest).

The "opinion" parts are probably best as some sort of optional plugins to the mainstream "bare bones" polyfill. Not sure how easy to do that would be though.

@yoavweiss and all
About the native implementation: Above I wrote some parts about "intelligent resource selection" and I really think, something similiar must be also included in native implementations

I have that on my todo list :) I looked into your implementation and would love to see some comments on the rational that led to the implementation and the constants. We were thinking of adding a geometric mean as the "breakpoint" between resources.

[scottjehl]

Okay. Let’s plan to do what we can to get these improvements into the project. I very much would prefer that to a separate project, and I really appreciate the work @aFarkas has done here. I certainly agree that the bug fixes are needed and much appreciated. As for the nice-to-have features, I still think that it’d be great to keep those as external files, optional to include in a build, but not there by default.

In the future, I do think it would be good to try to avoid these type of PRs. I closed it not because any of it was bad, but because I’d hoped it could be resubmitted in smaller, reviewable chunks, like every PR we’ve accepted before (with the exception of @jansepar's API rewrite for 2.0, but even that didn't go into master until a lot of branch work was done). As it is, this PR is mixed in purpose. It changes 36 files with 16,080 additions and 10,437 deletions. It closes 20 separate issues.  I just want to be able to feel confident about the code we accept and itemized PRs make that much easier.

I’d love to get other committers’ help testing and merging this work, since it seems both @Wilto and I are pretty busy lately. Starting in a branch and working it all out sounds like a good plan. @jansepar, @yoavweiss, @zcorpan thanks so much for your help in kicking this off.

Thanks everyone.

[Wilto]

Hey, good news—I’m off my frantic project as of today, so I’ll be digging back into Picturefill during the week.

[aFarkas]

@yoavweiss
I'm currently at work so need some time. But wanted to put something together. About the algo. This is subject to change.

The basic idea is to balance performance against quality and the main part is the so called greed factor. This describes how much the browser greeds for lower res candidate. Let's assume we are on a 2x device. We first multiply the greed factor with the dpr to get the calculated greed factor:

var GREED = 0.15 * 2;

Then let's asume, we have our matched best candidate with 2.5x and the next lower candidate with 1.8x. The calculation looks like this:

// calculate the useless pixel by substracting the density of the device from the density of the picture
var uselessX = 2.5 - 2; // 0.5
// calculate the actual greed for the lower res by multiplying the uselessX with the GREED
var curGreed = 0.5  * 0.3; // 0.15
// then we add the greed to the low res candidate
var lowRes = 1.8 + 0.15; // 1.95
and then we check wether the calculated lowRes wins
lowRes > DPR;

So in this szenario the 2.5 source still wins. If the low res would had 1.86x or the best candidate would had 2.67x the lowres source wins.

So this is the basic idea. Not really intelligent or smart, but quite simple, just needs a little bit of refinement ;)

[Wilto]

So, a couple of things, now that I have a chance to sit down and review this:

But I can't deliver small atomic PR, this would simply destroy the good effort/result I made.

I’m not sure why this is the case exactly, but it sounds like you’d prefer us to look at this massive PR as pass/fail rather than grabbing individual features and fixes—we can do that if you’d prefer, for certain, but where not every feature included here aligns with what the maintainers want from Picturefill this seems like a much less satisfying approach for everyone involved.

Right up front, I want to say that we hugely appreciate the work you’ve put into this, and we’re happy to pull in a number of the features and fixes you’ve contributed with the PR as-is. At the same time, though, I want to point out that this is a rocky approach to making an open source contribution. A huge number of open source projects, even high-profile ones, are labors of love—worked on nights, weekends, or whenever the maintainers can. It stops feeling that way when one is already swamped with work and batting away notifications at the same time. I’ll also say that declining PR feedback from a maintainer—feedback on their own project—is usually grounds for insta-closing.

In the meantime, this PR has been open for less than two weeks. During that time you’ve emailed Scott and I a couple of times about it. We are not full-time open source developers; we have client commitments that have to come first. This is clearly something you care about a great deal and that’s something we love to see, both as people so heavily involved in responsive images efforts and as huge advocates of getting more people involved in open source, but we can’t always drop everything to work on Picturefill. This also contains eighty seven commits—if we were working on open source projects full time, this would still take a very long time to fully test and review. Scott’s initial rejection was based on a few features we don’t plan to add—at least at this time—and called for more granular per-feature PRs. Absent those we now have to cherry-pick commits, which is slow-going and not terribly reliable when a feature or fix spans multiple commits.

You’ve put a lot of work into this, and the fixes you’ve included here stand to help a huge number of developers, and countless users across the web. I want you to know that I am willing to put in the time to pull your code to Picturefill, if you’re still amenable to it, but I also wanted to take the time to walk you through a few of the difficulties we’ve encountered with this PR. This is strong work, and we’d love to continue working on Picturefill with you going forward. I just want to remove any potential barriers to contribution like the ones we’ve run afoul of here, so that we can all better focus on writing code.

[aFarkas]

I’m not sure why this is the case exactly, but it sounds like you’d prefer us to look at this massive PR as pass/fail rather than grabbing individual features and fixes.

This is simply not true. Sorry, as you said this PR fixed about 20 bugs + includes main refactoring. What would happen, if I would have created for each "issue" a new branch based on this repo and then fixed these things issuewise. We would have the problem, that you need to review 20-30 different PRs with the same amount of code and after merging about 2 or 3 of them, you would have merge conflicts with the rest 17-28 PRs.

And to end this with a presumption about my bad intends is really not nice.

And again, because I knew that this will be one huge PR, I mailed Scott and you about my plans 6 days before I made this PR plus added you and Scott as collaborator to my repo, so that you could see what and how I'm doing things from the beginning.

After this was closed, I also mailed Scott and you, that I really understand, why you don't wanted to take this PR for several reasons. It's simply a invidious situation for both of us, not just you.

Fortunatley you are the maintainer of this project. You are the only one who controls everything. If you don't like this PR you can re-close it again. But please don't make me responsible for the fact, that fixing bugs is too much work for you.

[yoavweiss]

@aFarkas - do you feel like maybe hopping on the respimg irc channel (on irc.w3.org:6665 #respimg) ?

I think we can talk things through over there, see how we can merge that PR (or parts of it), and continue working on the project together.

[mike-engel]

@yoavweiss , @Wilto , @scottjehl , @aFarkas , Do we have a status on this PR? It would probably be best to get this closed/merged in sooner than later to prevent this code from getting too far out of sync.

[mike-engel]

This PR seems like it will also fix #370.

[albell]

Can I ask what the status of this is? I think it's better for everyone if we can all find a way to hang together here. Togetherness! @scottjehl @Wilto what about the option of mass-merging these commits into a 3.0 branch and then progressively cherry-picking out the stuff you really just can't stand? If that's a dealbreaker there can always be a separate project down the road, not necessarily a bad thing.

Currently picturefill is including matchMedia polyfill directly. I think it is a bad habit to include a third party script directly into another script. Developers should resolve the dependencies themselves. Otherwise a developer doesn't a) not know what's already included and b) ends up with multiple copies of the same script.

Agreed with @aFarkas . I don't want users to download this polyfill twice if it's needed elsewhere. Explain the dependency in the docs. It's not too complicated. Like to see this change in 3.0.

[mike-engel]

@baloneysandwiches As far as I know, this PR is dead, and @aFarkas is working on breaking these up into smaller chunks/a version 3.0 branch.

[mike-engel]

Closing due to stale-ness.