Deferred progress method not firing for all images

[benfoster]

Deferred progress method not firing for all images

[jamcoupe]

Am having the same issue! I thought I wasn't doing it right...

[hydnhntr]

Yeah unfortunately I'm seeing inconsistent results here too.

http://jsfiddle.net/BtHZu/6/
Only the last image is getting the border in Firefox and Safari.

http://jsfiddle.net/BtHZu/6/embedded/result/
Here Safari applies all the borders but again Firefox only applies it to the last image.

The imagesLoaded demo works perfectly but I'm curious as to if it's because the images are being appended to the DOM rather than being there from the start.

if ( hasNotify ) {
    deferred.notifyWith( $(img), [ isBroken, $images, $(proper), $(broken) ] );
}

The deferred stuff is complete witchcraft to me, but I can see that the plugin gets inside the conditional but doesn't reliably fire the notifyWith() method.

[darsain]

I've looked into it, and it actually works properly, it's just that .imagesLoaded() in FF is so fast, that all images has been marked as loaded before you've even managed to bind .progress handler :) And because .progress doesn't work with past events as .done, .fail and .always do, nothing happens... Oh you, asynchronicity, you... :)

So currently, there is no way how to make this work properly for you guys. This will require a change in .imagesLoaded() structure that will support binding of .progress method before the determination process starts.

But as I've mentioned, .done, .fail and .always methods will work properly.

[darsain]

So I've created a new branch defer that contains my solution to this problem. It introduces a new argument, called defer, that postpones determination process, and gives a user the '.start()' method to start it manually. Branch has updated demo page and documentation. It would be nice if @desandro could review the changes and perhaps raise some objections :)

[desandro]

See SHA: 7eb2210

I think if we add another argument, we might as well add a proper options argument. In this case I changed defer to options.isDeferringStart. There's some munging that has to go on to ensure that callback is still a proper function and this branch doesn't break backwards compatibility.

With this in mind - your thoughts?

[darsain]

But but but, now it's so simple :( With only 2 arguments, both being optional, people who are interested only in callback can do:

$(selector).imagesLoaded( fn );

While people interested in deferred+progress can do:

var dfd = $(selector).imagesLoaded( true );
// ...
dfd.start();

I really don't like an options object with only one option :/ Especially when it unsimplifies things.

I understand that this is a "thinking of future", where we might be in a situation to implement more options, but that really doesn't seem likely for this plugin, so adding options object would be just annoying. And even in that highly unlikely situation, we would just bump up the mayor version (i.e. non-backwards-compatible update) and be done :)

[darsain]

I guess a highlight? :) @desandro

[desandro]

@darsain Sorry, this is on the my back-burner for a couple days. You bring up some good points. My issues are:

var dfd = $(selector).imagesLoaded( true );

• Wouldn't this break jQuery chainability? If this was how it was previously working, we should have a separate conversation about that
• This bit of code doesn't read to me as "oh, this is a deferred start thingie." If I were someone else looking at imagesLoaded for the first time, I would assume the true flag would be for something simpler, like triggering immediately or something.

[darsain]

@desandro nope, the full jQuery object is always returned, just extended with deferred promise methods, and a .start() method if postponed determination is requested (defer=true argument), so you can still chain like:

var dfd = $(selector).imagesLoaded( true ).progress( fn ).css()...;
dsd.start();

But this example pointed out to me that checkImages(), i.e. .start() method should also return the full jQuery+deferred object, so we could chain even after that, like so:

 $(selector).imagesLoaded( true ).progress( fn ).start().css()...;

I'll add it to my todo for the final 2.1.0 version.

---

The second point: actually, the call with defer=true has nothing to do with jQuery Deferred object. It's just a similar word used for the second argument, and to avoid confusion, we could probably change it to postpone or something. The deferred object is being returned as an extension of the original jQuery object whenever the imagesLoaded is working with jQuery 1.5+ (i.e. when jQuery.Deferred() is present).

And what happens when you call .imagesLoaded( true ) is dictated by documentation, not feelings :) but we can play with semantics so it would make more sense. Like negating the meaning of the second argument from postpone, to immediate, where the default value would be true, and if people would want to postpone the determination, they would call $(selector).imagesLoaded( false ), which makes more sense?

[johan]

@darsain There is absolutely nothing intuitive about the whole .start() magic going on in this discussion – that's an implementation-level detail I'd be happier not needing to know anything about as an API user.

Could we get away from that need by supporting an options argument that sets all the handlers we care about before you kick things off?

$(selector).imagesLoaded({ progress: fadeInMyImage
                         , done: cueTheAngelChoir
                         , fail: tootTheAngryHorn
                         });

Having it treat a function parameter as the done callback sounds like a friendly shorthand, but for the love of $deity avoid the boolean trap; it's almost invariably bad API design, and this is not an exception to that rule.

I'm sure I'm neither the first nor last to end up writing code like this, as it was easier to spot what broke and kludge past the breakage than figuring out that you can't trust the progress callback:

function sizeAvatars($imgs) {
  function sizeOnceDimensionsKnown() { /* ... */ }
  var $seen = $([]);
  // see https://github.com/desandro/imagesloaded
  $imgs.imagesLoaded().progress(
    function (isBroken, $images, $proper, $broken) {
      if (!isBroken) sizeOnceDimensionsKnown(this);
      $seen = $seen.add(this); // don't redo below!
    })
    // Ugh: we're not guaranteed a .progress() callback for cached images?
    .done(function($all) {
      $all.not($seen).each(sizeOnceDimensionsKnown);
    });
}

[darsain]

@johan true. Your proposition is better :) So the new API would be:

Simple call with "on images done loading" callback

$images.imagesLoaded( function () {} );

Call registering deferred methods before the determination process starts

$images.imagesLoaded({
    done: function () {},
    fail: function () {},
    progress: function () {}
});

Using the returned deferred object

var dfd = $images.imagesLoaded();

dfd.done( function () {} );
dfd.fail( function () {} );
dfd.progress( function () {} ); // note in documentation that this is unreliable

---

If @desandro has no objections, I'll implement that as soon as there would be some free time on my hand :)

[desandro]

No objections. I like the options syntax.

👍