1335644 - (intersection-observer) Always send an initial notification after .observe() is called

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[ziyunfei]

even if the target doesn't intersects with the root at that time. This is a breaking change, will make some lazy loading load immediately.

http://crrev.com/c9092d27fc367898890811f6bb75893bc2200755

I have to update my code before Chrome 58 shipped:

function lazyload(callback) {
    if (window.IntersectionObserver) {
        var target = $("#J_Sale")[0]
        var observer = new IntersectionObserver(function() {
            observer.unobserve(target)
            callback()
        }, {rootMargin: "600px"})
        observer.observe(target)
    } else {
        var DataLazyload = require("kg/datalazyload/2.0.0/")
        new DataLazyload({
            diff: 600,
        }).addCallback("#J_Sale", callback)
    }
}

Comment 1

[Tobias Schneider [:tobytailor]]

Attachment: Always send an initial notification

Comment 2

[ziyunfei]

https://bugs.chromium.org/p/chromium/issues/detail?id=713819

Comment 3

[Tobias Schneider [:tobytailor]]

Attachment: Always send an initial notification

Much easier solution.

Comment 4

[Tobias Schneider [:tobytailor]]

A bit more explenation why this patch is solving the issue:

For every observed element we keep track of the current intersection ratio, as a value repesenting an index into the observer's thresholds array. A non intersecting state (which is different to an intersection ratio of 0, which can still be a valid intersection) is represented by a index value of -1. Every time we test for an intersection we re-calculate that index and compare it against the current value. If the two values differ, we schedule a notification to the observer instance. By starting out with an initial value of -2, we make sure that the very first intersection tests always leads to a notification being scheduled, whether the intersection ratio index is a result of an intersection (>= 0) or not (== -1).

Comment 5

[Tobias Schneider [:tobytailor]]

Attachment: Always send an initial notification

Added comment to explain different intersection states.

Comment 6

[Jet Villegas (inactive)]

Comment on attachment 8862907 [details] [diff] [review]
Always send an initial notification


>-  RegisteredIntersectionObservers()->Put(aObserver, -1);
>+
>+  // Value can be:
>+  //   -2:   Makes sure next calculated threshold always differs, leading to a
>+  //         notification task being scheduled.
>+  //   -1:   Non-intersecting.
>+  //   >= 0: Intersecting, valid index of aObserver->mThresholds.
>+  RegisteredIntersectionObservers()->Put(aObserver, -2);

Make this an enum. Also needs a test to make sure we guard against negative indices in the thresholds array

Comment 7

[Jet Villegas (inactive)]

BTW, I'm not convinced this is a good change to make, see:
https://github.com/WICG/IntersectionObserver/issues/165#issuecomment-301974363

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: Always send an initial notification

Addressed first review comments + test-migration approach as discussed on IRC.

Comment 9

[Eric Shepherd [:sheppy]]

Noting dev-doc-needed so that once this issue is decided, we can update docs appropriately -- the docs need to say one way or the other whether to expect this initial notification. Currently we don't specify, which is unhelpfully vague (I had assumed it would be sent when I wrote my first code using this API).

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8868716 [details] [diff] [review]
Always send an initial notification

Review of attachment 8868716 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/test/test_intersectionobservers.html
@@ +884,5 @@
>  
>          targetEl1.style.top = '220px';
>          targetEl1.style.left = '220px';
>  
> +        var callCount = 0;

Could this be a boolean, e.g. hasBeenCalledBefore? I think that's what it's trying to do, not sure I understand this completely.

What's the origin of these test changes? Did you write them, or is this just syncing up with the tests from the spec repo?

Comment 11

[Tobias Schneider [:tobytailor]]

(In reply to Markus Stange [:mstange] from comment #10)
> Comment on attachment 8868716 [details] [diff] [review]
> Always send an initial notification
> 
> Review of attachment 8868716 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/test/test_intersectionobservers.html
> @@ +884,5 @@
> >  
> >          targetEl1.style.top = '220px';
> >          targetEl1.style.left = '220px';
> >  
> > +        var callCount = 0;
> 
> Could this be a boolean, e.g. hasBeenCalledBefore? I think that's what it's
> trying to do, not sure I understand this completely.

I just applied same wording as in surrounding code. Purpose of this is to skip the very first call. A count seams to be more appropriate for this IMO.

> What's the origin of these test changes? Did you write them, or is this just
> syncing up with the tests from the spec repo?

The original tests where initially imported from Google's former test suite. With changes queuing up to get the web platform tests to work (tracked in Bug 1358666), some of them being breaking changes, the current tests need to be updated.

Comment 12

[Tobias Schneider [:tobytailor]]

Attachment: Always send an initial notification. r=mstange

Rebased patch.

Comment 13

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50a31bd3fb3f894898672019a327e5b62a66930c

Comment 14

[Tobias Schneider [:tobytailor]]

Looks like inbound was in a weird state on last try push, here is a better looking one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac88c419e32750d01671b674be68e8906b7ea69c

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f8c279b922
(intersection-observer) Always send an initial notification after .observe() is called. r=mstange

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7f8c279b922

Comment 17

[Eric Shepherd [:sheppy]]

Since this is indeed fixed in time for 55, no doc work is required. Huzzah!