Check intersection of lazy images from "update the rendering" by zcorpan · Pull Request #5510 · whatwg/html (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation59 Commits14 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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:
- Implementation bugs are filed:
(See WHATWG Working Mode: Changes for more details.)
/images.html ( diff )
/infrastructure.html ( diff )
/urls-and-fetching.html ( diff )
source Outdated Show resolved Hide resolved
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
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?
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.
Thanks for the explanation! It makes sense to me now. I'll update the PR.
…pdate the image data section
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
This was referenced
May 8, 2020
source Show resolved Hide resolved
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).
Yes, that's what you want since otherwise you load images inside iframes that are out of view.
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
I think I've addressed all major reviews in this with my latest commit, would appreciate if another owner took a look (@annevk ?)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
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...
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.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty nice!
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
