Closed Bug 1611204 Opened 11 months ago Closed 6 months ago

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

Categories

(Core :: Layout, defect, P3)

72 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: austin, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Component: DOM: Core & HTML → Layout
Priority: -- → P3

Yeah, agree this looks borked.

Status: UNCONFIRMED → NEW
Ever confirmed: true

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.

Flags: needinfo?(emilio)

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

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.

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

I think Firefox is right per spec here.

Flags: needinfo?(emilio)
Summary: IntersectionObserver with threshold 1 only updates the Entry.isIntersecting boolean once as expected. → IntersectionObserverEntry's isIntersecting boolean differs from other browsers when thresholds are involved.

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.

Assignee: nobody → emilio

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.

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?

(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).

(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?

(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.

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.

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.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0dba60796753
Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/23813 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/867528d1d35b
Fix IntersectionObserverEntry.isIntersecting to match other browsers. r=mstange
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.