Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatibilities between spec and implementations for observer events #222

Closed
nolanlawson opened this Issue Jun 26, 2017 · 15 comments

Comments

Projects
None yet
4 participants
@nolanlawson
Copy link
Member

nolanlawson commented Jun 26, 2017

(Forking discussion from #211)

There are incompatibilities between Chrome, Edge, Firefox, and the spec on when exactly observer events should fire.

According to the spec:

Let thresholdIndex be the index of the first entry in observer.thresholds whose value is greater than intersectionRatio, or the length of observer.thresholds if intersectionRatio is greater than the last entry in observer.thresholds.

And

If thresholdIndex does not equal previousThresholdIndex, queue an IntersectionObserverEntry

So IntersectionObserverEntrys (events) should only be fired when the thresholdIndex changes. However, what we've observed in our testing is that Chrome fires this event when isIntersecting changes, regardless of whether thresholdIndex changes (which we can observe through intersectionRatio).

So for example, here is a test showing Chrome firing an extra event for an element whose intersectionRatio does not change but whose isIntersecting has changed. And here are the cross-browser results:

out

And here is a test showing Chrome not firing an event for an element whose isIntersecting hasn't changed, but whose intersectionRatio has changed. And here are the cross-browser results:

out2

So roughly, here's the results with a ✔️ for event fired, and for event not being fired:

Spec Chrome 59.0.3071.109 Edge 15.15063 Firefox Developer Edition 55.0b3
Demo 1 ✔️ ✔️
Demo 2 ✔️ ✔️ ✔️

Currently we (EdgeHTML) are weighing the options in our own implementation, but I'm interested to hear what Blink/Gecko folks think about this.

@nolanlawson nolanlawson changed the title Chrome fires observer events when isIntersecting changes Incompatibilities between spec and implementations for observer events Jun 26, 2017

@szager-chromium

This comment has been minimized.

Copy link
Collaborator

szager-chromium commented Jun 26, 2017

  1. I'm pretty sure the spec should be updated to say that a notification happens if thresholdIndex OR isIntersecting changes.

In your first example, Chrome exhibits the behavior we would expect were this spec change applied.

  1. I also think the existing spec should be clarified by adding the words "OR EQUAL TO" as below:

"Let thresholdIndex be the index of the first entry in observer.thresholds whose value is greater than intersectionRatio, or the length of observer.thresholds if intersectionRatio is greater than OR EQUAL TO the last entry in observer.thresholds."

In your second example, Chrome exhibits the behavior we would expect were this spec change applied.

Thoughts? I think the first change ought to be non-controversial, but the second one is perhaps more debatable.

@tobytailor

This comment has been minimized.

Copy link

tobytailor commented Jun 28, 2017

One thing Edge's implementation seems to miss is an initial event being fired when starting to observe a non-intersecting element (see missing <isIntersecting: false, intersectionRatio: 0> events). See #165 for discussion on this and https://github.com/WICG/IntersectionObserver/pull/192/files for spec changes.

@tobytailor

This comment has been minimized.

Copy link

tobytailor commented Jun 28, 2017

@szager-chromium I think your first change is what we should push for. But this might also require either an additional property (e.g. isIntersecting) in IntersectionObserverRegistration or another special value for previousThresholdIndex (e.g. -2) indicating a previous non-intersection. The latter is how we currently archive this behavior in Firefox.

@nolanlawson

This comment has been minimized.

Copy link
Member Author

nolanlawson commented Jun 28, 2017

One thing Edge's implementation seems to miss is an initial event being fired when starting to observe a non-intersecting element

Yep, noticed this! 🙂

Just to make sure I understand correctly: it sounds like @szager-chromium you would like the spec to match Chrome's behavior (fire on Demo 1, don't fire on Demo 2), whereas @tobytailor you would like the spec to match Firefox's behavior (fire on both Demo 1 and Demo 2). Did I get that right?

As for Edge, my team is still deciding how we want to tackle this. Edge's current implementation follows an older version of the spec, and we were a bit blindsided by recent changes (e.g. e2a5bfb, 9ce0b35). So I'm first trying to understand the motivating factor behind these changes.

The case for isIntersecting seems pretty clear to me (directly answers the questions webdevs want answered, i.e. "is this element intersecting?"). I can also understand immediately firing events on observe() even for non-intersecting elements. But as for the edge-adjacency change, I'm a bit puzzled as to why we want to make a special case for exactly overlapping bounds. It's confusing to me that the intersectionRatio would be 0 but the isIntersecting would be true only in this one special case. Can you help me understand the motivation behind that change?

@szager-chromium

This comment has been minimized.

Copy link
Collaborator

szager-chromium commented Jun 28, 2017

@nolanlawson I would turn that question back on you: in the case of edge-adjacent intersections, what should the value of intersectionRatio be?

As I understand it, the real question is whether, given a threshold of zero, we should send a notification when transitioning from is-intersecting-zero-intersection-ratio to is-intersecting-non-zero-intersection-ratio. That's the part that I consider debatable, and I can imagine arguments on both sides.

There is an obvious workaround if you do, indeed, want that notification: construct your IntersectionObserver like this:

new IntersectionObserver(myCallback, { threshold: [ Number.MIN_VALUE ] })

I don't really like that, though; it's hacky and non-obvious why you would need it. Part of the motivation behind adding the isIntersecting field was to avoid such shenanigans.

@nolanlawson

This comment has been minimized.

Copy link
Member Author

nolanlawson commented Jun 29, 2017

I would turn that question back on you: in the case of edge-adjacent intersections, what should the value of intersectionRatio be?

As a naïve user, I would expect that isIntersecting === false implies intersectionRatio === 0, and that isIntersecting === true implies intersectionRatio > 0. If the edges are perfectly touching, then in what way could they be said to be "intersecting?"

However, looking back on the context for isIntersecting (#69), it seems the whole point is to account for zero-area elements, since their intersectionRatio would always be 0 and thus you couldn't detect any change in their position relative to the root. So isIntersecting solves that by using inclusive intersection rather than exclusive intersection. Is that right?

I'm asking these questions because I'm genuinely trying to understand the motivation behind the spec changes. As you say, there's a real question here about when exactly to fire events, and understanding the context helps. 🙂

@tobytailor

This comment has been minimized.

Copy link

tobytailor commented Jul 5, 2017

In fact, intersectionRatio is 1 and not 0 for zero-area elements. See https://wicg.github.io/IntersectionObserver/#dom-intersectionobserverentry-intersectionratio.

@nolanlawson

This comment has been minimized.

Copy link
Member Author

nolanlawson commented Jul 6, 2017

Thanks for the feedback! I've reviewed this issue with @scottlow and we've come to the following conclusions:

1. We agree with @szager-chromium on this:

I'm pretty sure the spec should be updated to say that a notification happens if thresholdIndex OR isIntersecting changes.

2. As for where the spec says this:

Let thresholdIndex be the index of the first entry in observer.thresholds whose value is greater than intersectionRatio, or the length of observer.thresholds if intersectionRatio is greater than the last entry in observer.thresholds.

This is a bug in the spec, because in e.g. the case where an element's starting intersectionRatio is 0 and observer.thresholds is [0], the thresholdIndex is undefined, because intersectionRatio is 0 and thus doesn't satisfy either condition. Changing the spec to say "if intersectionRatio is greater than OR EQUAL TO the last entry in observer.thresholds" would fix this spec bug, and it would also match Chrome's implementation, but we have an additional concern:

3. In the second demo, where three list elements are intersecting but the third is edge-adjacent, we feel Chrome's behavior would be surprising to web authors, because it fires no events for either the first element or the third one. So we'd prefer to match Firefox's implementation.

However, it's not clear how to amend the spec to achieve this behavior. Most importantly, we want to avoid the situation where an element crossing a root boundary fires two events: one for edge-adjacency (intersectionRatio: 0, isIntersecting: false -> true) and another for the thresholdIndex change (isIntersecting: true, intersectionRatio: 0 -> 0.00001). We can observe that Firefox doesn't double-fire, but we're not sure how y'all did it. 😃

So @tobytailor if you could fill us in on Firefox's current implementation with the -2 fix, that would help clear up our understanding. Spec-wise, though, we'd prefer using an additional boolean to track isIntersecting rather than thresholdIndex: -2, as long as it avoids the case of the double-fire.

@tobytailor

This comment has been minimized.

Copy link

tobytailor commented Jul 17, 2017

We start by initializing an elements previousThreshold value with -2. When testing for intersection. thresholdIndex starts out with -1 and in case of an intersection gets set to the actual threshold index, according to the algorithm described in the spec. That means when running the algorithm on an element for the very first time, the result will always be different to the previousThreshold (being -2), whether its intersecting or not. This guarantees an initial notification when starting to observe an element. From that point on, previousThreshold is always >= -1.

@nolanlawson

This comment has been minimized.

Copy link
Member Author

nolanlawson commented Jul 19, 2017

@tobytailor Hm so it sounds like by adding an extra boolean to track whether we've run the algorithm yet or not, we could avoid the -2. That said, I'm not sure how this fixes the double-fire issue mentioned above? How does Firefox avoid firing the event twice?

@nolanlawson

This comment has been minimized.

Copy link
Member Author

nolanlawson commented Jul 27, 2017

I've looked into this a bit more, and it appears that Firefox's implementation actually doesn't solve the issue of the double-fire. You can see it in this demo:

2017-07-26 16_46_01-microsoft edge

This appears to be a drawback of the "fire when either isIntersecting or previousThreshold changes" approach. Assuming the user scrolls slowly enough (or you set scrollTop manually, as I do in the demo), you can get a double-fire when the element crosses its root boundary.

Based on this, it seems @szager-chromium's original suggestion may be the best one. The Demo 2 case is indeed odd in Chrome given that you can effectively never get an event fired for any element no matter how much you scroll, but that'll only occur in the very unusual case where an element is both at the bottom of a list and perfectly lines up with its root on the top, so maybe it's not worth bending over backwards to accommodate.

@kouhin

This comment has been minimized.

Copy link

kouhin commented Jul 31, 2017

On Chrome: 53.0.2785.135, Android: 7.1.1, there is no isIntersecting in IntersectionObserverEntry, and the intersectionRatio is 0 when it is zero intersection.
So I think in this case, isIntersecting should be recalculated totally by polyfill.

@nolanlawson

This comment has been minimized.

Copy link
Member Author

nolanlawson commented Jul 31, 2017

Chrome 53 is 7 versions old and is at 0.08% global usage according to caniuse.com… is there a strong reason to support it?

@kouhin

This comment has been minimized.

Copy link

kouhin commented Aug 1, 2017

@nolanlawson Sorry for reporting this problem late.
I haven't confirm it on other version of Chrome. However I found that isIntersecting is not available until Chrome 58. And IntersectionObserver is supported from Chrome 51.
Zero intersection problem may also happen on Chrome 51 - 57.

@nolanlawson

This comment has been minimized.

Copy link
Member Author

nolanlawson commented Aug 28, 2017

This is a quick note that we are opting to match Chrome's behavior in our next release (EdgeHTML 16), due to the double-fire issue described above as well as general webcompat. You can preview it in the latest public insider build (16275).

So a PR should probably be opened on the spec to adopt @szager-chromium's suggestion, unless there are objections.

nolanlawson added a commit to nolanlawson/IntersectionObserver that referenced this issue Aug 28, 2017

sideshowbarker added a commit to saschanaz/IntersectionObserver that referenced this issue Mar 17, 2018

asakusuma added a commit to linkedin/spaniel that referenced this issue Jan 5, 2019

WIP: Handle zero-area and display: none cases
Attempts to satsify the spec: https://www.w3.org/TR/intersection-observer/#update-intersection-observations-algo

isIntersecting, non-zero area, and display:nonen are all related, so fixing in one swoop.

Fixes:
#93
#73

Related issues:
w3c/IntersectionObserver#69
w3c/IntersectionObserver#222

asakusuma added a commit to linkedin/spaniel that referenced this issue Jan 5, 2019

WIP: Handle zero-area and display: none cases
Attempts to satsify the spec: https://www.w3.org/TR/intersection-observer/#update-intersection-observations-algo

isIntersecting, non-zero area, and display:nonen are all related, so fixing in one swoop.

Fixes:
#93
#73

Related issues:
w3c/IntersectionObserver#69
w3c/IntersectionObserver#222

asakusuma added a commit to linkedin/spaniel that referenced this issue Jan 6, 2019

Handle zero-area and display: none cases
Attempts to satsify the spec: https://www.w3.org/TR/intersection-observer/#update-intersection-observations-algo

isIntersecting, non-zero area, and display:nonen are all related, so fixing in one swoop.

Fixes:
#93
#73

Related issues:
w3c/IntersectionObserver#69
w3c/IntersectionObserver#222

asakusuma added a commit to linkedin/spaniel that referenced this issue Jan 6, 2019

Handle zero-area and display: none cases
Attempts to satsify the spec: https://www.w3.org/TR/intersection-observer/#update-intersection-observations-algo

isIntersecting, non-zero area, and display:nonen are all related, so fixing in one swoop.

Fixes:
#93
#73

Related issues:
w3c/IntersectionObserver#69
w3c/IntersectionObserver#222

lynchbomb added a commit to linkedin/spaniel that referenced this issue Jan 7, 2019

Handle zero-area and display: none cases
Attempts to satsify the spec: https://www.w3.org/TR/intersection-observer/#update-intersection-observations-algo

isIntersecting, non-zero area, and display:nonen are all related, so fixing in one swoop.

Fixes:
#93
#73

Related issues:
w3c/IntersectionObserver#69
w3c/IntersectionObserver#222

asakusuma added a commit to linkedin/spaniel that referenced this issue Jan 8, 2019

Handle zero-area and display: none cases
Attempts to satsify the spec: https://www.w3.org/TR/intersection-observer/#update-intersection-observations-algo

isIntersecting, non-zero area, and display:nonen are all related, so fixing in one swoop.

Fixes:
#93
#73

Related issues:
w3c/IntersectionObserver#69
w3c/IntersectionObserver#222

lynchbomb added a commit to linkedin/spaniel that referenced this issue Jan 8, 2019

Handle zero-area and display: none cases
Attempts to satsify the spec: https://www.w3.org/TR/intersection-observer/#update-intersection-observations-algo

isIntersecting, non-zero area, and display:nonen are all related, so fixing in one swoop.

Fixes:
#93
#73

Related issues:
w3c/IntersectionObserver#69
w3c/IntersectionObserver#222
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.