w3c/IntersectionObserver

isIntersecting definition doesn't match browsers when thresholds are involved.

emilio opened this issue · 7 comments

emilio commented

So the spec says that technically, you're supposed to set isIntersecting to true unconditionally, regardless of threshold:

https://w3c.github.io/IntersectionObserver/#update-intersection-observations-algo:

Let isIntersecting be true if targetRect and rootBounds intersect or are edge-adjacent, even if the intersection has zero area (because rootBounds or targetRect have zero area); otherwise, let isIntersecting be false.

It also says that we should schedule notifications whenever isIntersecting changes, regardless of the threshold change. However that doesn't match browsers, see:

Firefox initially implemented the spec to the letter but had a workaround of sorts to make it similar to other browsers:

Given no browser follows the spec, and that Firefox is pretty close to other browsers, except on the specific value of isIntersecting, I think we should change the spec and Firefox, probably... But it seems kind of an unfortunate situation to be in.

szager-chromium commented

A bit of background...

isIntersecting was added to handle a very specific corner case: namely, for an observer with zero as one of its thresholds, you could not distinguish between a "not intersecting" notification and an "intersecting with zero-area intersection" notification. There are two ways that a zero-area intersection can occur: target and root are edge-adjacent; or target has zero area.

My initial attempt to rationalize this is #316:

Here's the language around when to send a notification:

  1. If:
    * |thresholdIndex| does not equal |previousThresholdIndex| or
    * |isIntersecting| does not equal |previousIsIntersecting| and the first value in |observer|.{{thresholds}} is 0
    queue an IntersectionObserverEntry,
    passing in |observer|, |time|, |rootBounds|,
    |boundingClientRect|, |intersectionRect|, |isIntersecting|, and |target|.

To understand this, it's helpful to consider two possibilities: either observer.thresholds contains a zero, or it does not.

If observer.thresholds does not contain zero, then a change in isIntersecting alone will not trigger a notification; only crossing a threshold will do it. Note that this means that when a notification is sent because the intersection has fallen below the lowest threshold, isIntersecting can be either true or false. Chromium does not currently implement this correctly; it always sets isIntersecting to false when the intersection falls below the lowest non-zero threshold.

If observer.thresholds does contain zero, then the second condition means that the transition from "not intersecting" to "intersecting with zero-area intersection" will trigger a notification to be sent as intended.

Thinking about this further, I would probably prefer that isIntersecting had a reliable value for the case where there is no zero threshold. To that end, I think the above PR could be modified to read:

  1. Let |isIntersectingForNotification| be true if |thresholdIndex| > 0, false otherwise.
  2. If:
    * |thresholdIndex| does not equal |previousThresholdIndex| or
    * |isIntersectingForNotification| does not equal |previousIsIntersecting|
    queue an IntersectionObserverEntry,
    passing in |observer|, |time|, |rootBounds|,
    |boundingClientRect|, |intersectionRect|, |isIntersectingForNotification|, and |target|.
  3. Assign thresholdIndex to intersectionObserverRegistration’s previousThresholdIndex property.
  4. Assign isIntersectingForNotification to intersectionObserverRegistration’s previousIsIntersecting property.

The net effect of this would be that isIntersecting would always be false if the lowest threshold was not met. In the special case of zero threshold, it would be true if there was an intersection (either zero-area or non-zero-area), and false otherwise. This is what chromium currently does, and I think this is in keeping with the intuitive sense of what isIntersecting should mean.

szager-chromium commented

One small modification -- the condition for queueing an entry can be simplified:

  1. If |thresholdIndex| does not equal |previousThresholdIndex|, queue an IntersectionObserverEntry ...

The condition on isIntersectingForNotification becomes redundant, because it can only change value when thresholdIndex also changes.

szager-chromium commented

... and this would actually allow us to get entirely rid of the previousIsIntersecting property on IntersectionObserverRegistration, which would be a nice simplification.

szager-chromium commented

@emilio -- friendly ping, this is an important one to resolve.

ambar commented

A demo for isIntersecting=true, intersectionRatio=0
https://codesandbox.io/s/inview-intersection-ratio-zero-sph9o

thebuilder commented

It would be great if we would could get an agreement on this.
In the https://github.com/thebuilder/react-intersection-observer implementation I'm forced to check both isIntersecting and intersectionRatio to determine if the element is inside the viewport.

If users make their own implementation, and expect that can just rely on the observer behaving logically with isIntersecting, then they are very likely to run into subtle bugs in different browsers - something they most likely wont know until users report a bug.

emilio commented

The net effect of this would be that isIntersecting would always be false if the lowest threshold was not met. In the special case of zero threshold, it would be true if there was an intersection (either zero-area or non-zero-area), and false otherwise. This is what chromium currently does, and I think this is in keeping with the intuitive sense of what isIntersecting should mean.

I think that would be sensible, and it being what Chromium does makes it probably not-problematic to change for Firefox.

Do you know if there are WPTs that test this edge case? Looking at wpt.fyi I don't think I found any. But landing your PR with this tweak and a WPT test would be awesome.