Check intersection of lazy images from "update the rendering"

[zcorpan]

Fixes #5236.

• At least two implementers are interested (and none opposed):
  • Chromium: @domfarolino
  • Gecko: @emilio
  • WebKit: asked in Check intersection of lazy images from "update the rendering" #5510 (comment)
• Tests are written and can be reviewed and commented upon at:
  • N/A per Check intersection of lazy images from "update the rendering" #5510 (review)
• Implementation bugs are filed:
  • N/A per Check intersection of lazy images from "update the rendering" #5510 (review)

(See WHATWG Working Mode: Changes for more details.)

---

/images.html ( diff )
/infrastructure.html ( diff )
/urls-and-fetching.html ( diff )

[domfarolino]

I'm confused with this approach. My understanding of #5236 is that we sort of resolved to go with option (1) from the original post, where we use the Intersection Observer spec, instead of adding something extra to the update the rendering steps, and having a per-document list of lazy loaded images.

[zcorpan]

Per #5236 (comment) , my understanding was that we only want to start observing in the update the rendering. But maybe I was overthinking it. Should the "update the image data" add lazy images to an intersection observer directly?

[domfarolino]

Per #5236 (comment) , my understanding was that we only want to start observing in the update the rendering.

I think from updating-the-image-data if loading=lazy, we always want to observe the image, so that the update-the-rendering resumes the updating-the-image-data algorithm.

I'm commenting on how we observe the image, not so much when. My understanding from #5236 is that we don't want to create a per-document image list, and add a new step to update-the-rendering. Instead, from updating-the-image-data, we want to create and attach an Intersection Observer, and not add anything to the updating-the-image-data. That way the next time update-the-rendering runs, we just delegate to the Intersection Observer steps, instead of our own custom logic. Does that make sense? Please see @domenic's comment: #5236 (comment)

I'm happy to take on this approach, as it is pretty complicated and interesting to me. Or if you want to adjust this PR to do so, that's fine too.

[zcorpan]

Thanks for the explanation! It makes sense to me now. I'll update the PR.

[zcorpan]

OK, please take a look.

I set the root to the document, and left rootMargin and threshold UA-defined.

I realized now though that IntersectionObserver can only initialize rootMargin, and then it's read-only. Per #5408 (comment) , I think it needs to be changeable somehow?

[domfarolino]

I still have to take a closer look at some edge cases, but overall looking decent. Regarding some of the checkboxes in the main comment:

• I think we can check the "at least two implementers agree [...]", given that this is mostly a spec refactoring, and and Chrome (me) and Firefox (@emilio) agree with the change, as per discussion in Lazy load intersection-checking should not be done in-parallel #5236.
• There are no implementation bugs to be filed, since we're just aligning the spec to what implementations are already doing, so that can probably be checked

[zcorpan]

I realized now though that IntersectionObserver can only initialize rootMargin, and then it's read-only. Per #5408 (comment) , I think it needs to be changeable somehow?

Filed w3c/IntersectionObserver#428

[zcorpan]

cc @rwlbuis @smfr

[zcorpan]

There are no implementation bugs to be filed, since we're just aligning the spec to what implementations are already doing, so that can probably be checked

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/8073

It seems in both Chromium and Gecko, root is the implicit root. Is that what we want? This PR sets it to the image's node document. Using the implicit root takes away the rootMargin if the origins aren't similar-origin, per IntersectionObserver spec (for images in nested browsing contexts).

[emilio]

Yes, that's what you want since otherwise you load images inside iframes that are out of view.

[domfarolino]

I think I've addressed all major reviews in this with my latest commit, would appreciate if another owner took a look (@annevk ?)

[zcorpan]

Thanks @domfarolino, LGTM

[annevk]

I thought we had discussed this (though maybe only with Dominic), but we should not add new points where we instantiate JavaScript objects or call their methods.

[domenic]

we should not add new points where we instantiate JavaScript objects or call their methods.

Although I agree in general, I'm not sure whether we should block this work on that. It seems like it would require a large refactoring of the IntersectionObserver spec.

A middle ground might be to add awkward disclaimers everywhere, e.g. "new IntersectionObserver instance (using the original value of the IntersectionObserver constructor)", "For each entry in entries (using a method of iteration which does not trigger developer-modifiable array accessors or iteration hooks)", "if entry.isIntersecting is true (using the original value of the isIntersecting getter)", etc.

But, I guess if we could pull off the refactoring, that would be much nicer...

[domfarolino]

I agree with the comments around using the DOM-exposed method names and objects. This was originally brought up in #5510 (comment), and we filed w3c/IntersectionObserver#427 for it.

But yeah, until that gets addressed I agree with making notes in the spec that explain what we're trying to do, and pointing to that issue to track the IO refactoring. For now I've added one around using the IntersectionObserver constructor, and I'll add more around the other parts @domenic mentioned.

[domfarolino]

I've added some more <p class="XXX">'s describing that dom-IntersectionObserver* references should use the original values etc. I think this is ready for review

[domenic]

Looking pretty nice!

[zcorpan]

Thanks, @domenic! I've addressed your comments.

Follow-up cleanup tasks will be:

• Fix Setting loading=eager should be less COMEFROM-ey #5547
• Fix Define referenceable algorithms for IntersectionObserver methods w3c/IntersectionObserver#427 and fix the "XXX" issues introduced in this PR.

[domfarolino]

Thanks, looks good!

[zcorpan]

@domenic can you do the honors? 🙂