Closed Bug 1335644 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: 446240525, Assigned: tschneider)

References

Details

Attachments

(1 file, 4 obsolete files)

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)
    }
}
Flags: needinfo?(tschneider)
Flags: needinfo?(tschneider)
Much easier solution.
Attachment #8858466 - Attachment is obsolete: true
Blocks: 1358666
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).
Added comment to explain different intersection states.
Attachment #8860561 - Attachment is obsolete: true
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
BTW, I'm not convinced this is a good change to make, see:
https://github.com/WICG/IntersectionObserver/issues/165#issuecomment-301974363
Addressed first review comments + test-migration approach as discussed on IRC.
Attachment #8862907 - Attachment is obsolete: true
Attachment #8868716 - Flags: review?(bugs)
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).
Keywords: dev-doc-needed
Attachment #8868716 - Flags: review?(bugs) → review?(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?
Attachment #8868716 - Flags: review?(mstange) → review+
(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.
Rebased patch.
Attachment #8868716 - Attachment is obsolete: true
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
Depends on: 1367612
Assignee: nobody → tschneider
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f8c279b922
(intersection-observer) Always send an initial notification after .observe() is called. r=mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b7f8c279b922
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Since this is indeed fixed in time for 55, no doc work is required. Huzzah!
Keywords: dev-doc-needed
See Also: → 1400971
You need to log in before you can comment on or make changes to this bug.