Cancel old invocations after lazy load resumption

[domfarolino]

The #updating-the-image-data algorithm queues a resumption microtask part-way through. Before the resumption microtask is run, multiple invocations of the algorithm can be started. Therefore, it has step (8) to ensure that after the algorithm is resumed, only the latest invocation continues to completion.

With the introduction of the lazy load semantics, there is now another place where this algorithm can be "paused" and later resumed. #3752 should have included a step identical to step (8) after the algorithm can be resumed as a result of the #will-lazy-load-image-steps. This PR adds it and closes #5314.

Technically without this step there could be "N" running update-the-image-data algorithms associated with a given <img>, each fetching an image. But no browser actually implements this mistake, so this change is nearly-editorial. I am not 100% sure how to test this (would a browser that implemented this mistake fire N load/error events for each running instance? If so I guess that is testable?)

---

/images.html ( diff )

[annevk]

This looks reasonable, but maybe @rwlbuis could review?

[domfarolino]

That sounds good. @rwlbuis PTAL (It won't let me assign you as a reviewer though). Also requesting from Simon too.

[zcorpan]

The next step is the fetch. It is possible to detect the fetch itself, like this:

• Start the test with a lazy image somewhere that doesn't load. (I don't know how to ensure this without standardizing on something for When is a lazy-loaded image "about to intersect the viewport" #5408, though.) The URL should point to an endpoint that stores a stash, with a new UUID.
• After window.onload (at which point "update the image data" is paused at the lazy step), do a "relevant mutation" like changing src. The new URL should probably be loading slowly (using trickle), so the first fetch can complete setting the stash before this fetch is done.
• Scroll the image into view.
• When the image gets a load event, perform a fetch() to take the stash.

If the stash existed, fail the test.

[domfarolino]

It is possible to detect the fetch itself, like this

Right, and even more-locally, I think we can detect the Fetch with the performance timing API right? This could be more simple anyways.

I don't know how to ensure this without standardizing on something for #5408, though

Technically true, however we've been writing WPTs for this anyways with practical known-large-enough gaps to guarantee that any implementation WHATWG is interested in behaves as intended by the test, so I don't think that matters too much.

In general, I'd be OK doing something like this, but for completeness I think we'd have to entirely refactor the relevant mutations tests to track "number of fetches" for each relevant mutation, as opposed to just the expected effect on load/error events for the element. Given that, I slightly lean toward keeping the test limited to detecting unnecessary load/error event handler invocations. What do you think?

[zcorpan]

Right, and even more-locally, I think we can detect the Fetch with the performance timing API right? This could be more simple anyways.

I'm not familiar enough with that API to know how to do it, but if it can and it would show a "double download" for this case, then sure.

Technically true, however we've been writing WPTs for this anyways

OK.

In general, I'd be OK doing something like this, but for completeness I think we'd have to entirely refactor the relevant mutations tests

I don't think we have to, per se. If it's likely to uncover new bugs, then it might be worth doing. But we're not going to cover all possible cases with tests, and we should balance with other concerns like how long it takes to run the tests.

[rwlbuis]

LGTM, but I think it is better if somebody with more html spec experience reviews, I am not even sure if I have that power. Also it would be nice to have a test, but only if it is easy to reproduce.

[annevk]

@rwlbuis your input is certainly always appreciated.

Looking at this again I think my main quibble is that it adds COMEFROM to an already convoluted algorithm, but I'm somewhat inclined to accept it given that it's better than the status quo. @domenic?

[domenic]

Hmm, yeah, it would certainly be nice if this was rephrased in terms of an explicit boolean attached to each img element, which got reset at the appropriate times. The "even if it aborted and is no longer running" makes this extra confusing, I think...

[domfarolino]

The original intention of these lines is to make sure we don't invoke the rest of the algorithm more than once, in case we enter the #updating-the-image-data algorithm multiple times while an image is deferred. However with the refactoring Simon and I are doing in #5510, I think that we get this for free.

If an image is deferred, and we re-enter the #updating-the-image-data algorithm, we'll eventually invoke IntersectionObserver#observe, a second time, however per the IO spec, if an IO is already observing a target element, then invoking observe() again is a no-op, so I think after that PR lands, we won't need this change at all. Do you agree?

I do agree with @domenic in that the wording here is confusing, and since the spec already uses it here, we should maybe figure out a way to remove that and make it more clear.

Edit: On second thought, I'm not sure how much more clear we can make it without basically introducing weak pointer semantics in the spec