1611204 - IntersectionObserverEntry's isIntersecting boolean differs from other browsers when thresholds are involved.

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[austin]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

1. Create a page with 1 target element and make sure there is enough room on the page to scroll the target fully into view, and fully out of view.

2. Setup an Intersection observer that simply logs the entry.isIntersecting prop, and set the options.threshold to 1. For example:

const handler = ([entry]) => console.log(entry.isIntersecting)
const observer = new IntersectionObserver(handler, { threshold: 1 });
observer.observe(document.querySelector('.target'))

Here's a more visual demo on Codepen:
https://codepen.io/Stegosource/pen/NWPoObE

Actual results:

If the target begins off the page, the even will work as expected. The observer will log the entry.isIntersecting value as false. When the entire element becomes fully visible, the observer will log the entry.isIntersecting value as true. So far, this is the expected behavior.

From this point on, when the element becomes fully visible, or no longer fully visible, the handler will fire (as expected), but the entry.isIntersecting property will always be logged as true even though it should be false if it's not fully visible.

Expected results:

When the element becomes fully visible, the handler should log the entry.isIntersecting property as true, and when the element is no longer fully visible the handler should log the entry.isIntersecting as false.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Yeah, agree this looks borked.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I'll put it in my queue as I also need to look at other IntersectionObserver bugs, but not sure I'll get to it soon.

Comment 3

[Tim Kadlec]

Ran into this today.

I put another Codepen together to help, but it's pretty similar to the original. https://codepen.io/tkadlec/pen/XWmoOQX

Shot a short video as well. https://www.dropbox.com/s/c0x5rnv0ogp4ryy/iobserver-ff-bug.mov?dl=0

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case from reporter (also logging the intersection ratio).

So I'm not sure this is a Firefox bug after all. The spec seems pretty clear:

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.

In particular note how the isIntersecting value is completely independent of the threshold. So I'd expect isIntersecting to be true, even if the intersection ratio is below the threshold. I think this is a Chrome bug.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

In particular, WebKit and Blink seem to use isIntersecting = thresholdIndex > 0, effectively. But that is not what the spec says.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I think Firefox is right per spec here.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Though in fairness Firefox should then technically dispatch a notification when the element enters partially in view, and same for when it goes fully out of view.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

So I dug a bit more and filed https://github.com/w3c/IntersectionObserver/issues/432.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1611204 - Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange,dholbert

Note that no browser matches the spec (see
https://github.com/w3c/IntersectionObserver/issues/432), but that our
behavior is reasonably close to them. So do this to match them.

Comment 10

[austin]

To be honest, Im not 100% clear from the spec what the behavior should be. As I understand it, isIntersecting should be true while the target is intersecting the root. In the example you shared, isIntersecting starts as false, changes to true when the the box comes fully into view, but then it never goes back to false. That's true whether the box is partially off screen or even fully offscreen. Shouldnt it go back to false if its fullly offscreen?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to austin from comment #10)

To be honest, Im not 100% clear from the spec what the behavior should be. As I understand it, isIntersecting should be true while the target is intersecting the root. In the example you shared, isIntersecting starts as false, changes to true when the the box comes fully into view, but then it never goes back to false. That's true whether the box is partially off screen or even fully offscreen. Shouldnt it go back to false if its fullly offscreen?

Yes it should, and per spec it should get another notification when that happens, but that doesn't match any browser so I think we should change the definition of isIntersecting to match the majority (Blink and WebKit in this case).

Comment 12

[danielkarp]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Yes it should, and per spec it should get another notification when that happens, but that doesn't match any browser so I think we should change the definition of isIntersecting to match the majority (Blink and WebKit in this case).

I’m not sure I understand what is wrong in the definition. To me, it seems clear that, at some point, as the user scrolls, the element will move completely out of view AND Be no longer edge adjacent. At that point, according to the spec, it should trigger a new notification with isIntersecting set to false. This is what seems to be missing in the current behavior, as I read it. Am I missing something?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to danielkarp from comment #12)

I’m not sure I understand what is wrong in the definition. To me, it seems clear that, at some point, as the user scrolls, the element will move completely out of view AND Be no longer edge adjacent. At that point, according to the spec, it should trigger a new notification with isIntersecting set to false. This is what seems to be missing in the current behavior, as I read it. Am I missing something?

Yes, that's right. The definition doesn't have anything inherently wrong, except the fact that it doesn't really match any browser. All browsers right now only dispatch notifications when the thresholds are crossed, rather than also when isIntersecting changes.

That's really what this comment, introduced in bug 1391154, is about.

I had a patch to make us behave as per spec, but and it causes a bunch of test failures (arguably because the tests are testing Chromium behavior, not the spec, sadness). Unless Chromium and WebKit both want to adapt to the spec, I think the best thing to do is just matching them and changing the spec. Our behavior is reasonably close to theirs right now anyway.

Comment 14

[danielkarp]

Ah, now I understand. Perhaps a better behavior would be to have isIntersecting depend on the direction in which the threshold is crossed. So if it goes from not visible to edge adjacent (but still not visisible), it reports as intersecting, and if it goes from visible to edge adjacent and not visible, it could report as NOT intersecting. But obviously, that is something for W3C to consider. As it is, isIntersecting is currently useless for us, and we use intersectionRatio instead.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Sure, makes sense. I'd consider filing an issue in the intersectionobserver repo, if you think that's not sufficiently covered by the issue I posted above.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0dba60796753
Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/23813 for changes under testing/web-platform/tests

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 19

[Razvan Maries]

Backed out for perma failures on isIntersecting-threshold.html.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=Mg-RxzNOQDqyQZUz18buZw-0&resultStatus=testfailed%2Cbusted%2Cexception&revision=6c865ed1f99827c0df85840f6a98a1e2d7587741&searchStr=linux%2C18.04%2Cx64%2Casan%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-linux1804-64-asan%2Fopt-web-platform-tests-e10s-15%2Cw%28wpt15%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304053163&repo=autoland&lineNumber=2799

Backout: https://hg.mozilla.org/integration/autoland/rev/ce8ad8ef1a2cec5a8b9c9dde9fb82c7e7ab2675f

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/867528d1d35b
Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 23

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/867528d1d35b

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot