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

Fix an edge case on IntersectionObserver #3502

Closed
wants to merge 2 commits into from

Conversation

unarist
Copy link
Contributor

@unarist unarist commented Jun 1, 2017

The default value for the threshold option is 0, which means notify when intersectionRatio will be greater than 0. However, it generates the entry which is intersectionRatio === 0 && isIntersected sometimes. This causes some toots still being hidden.

gif

This issue wouldn't occur if we use isIntersecting, but not now for compatibility (#3365). So I've set threshold to clearly greater value than 0 to avoid this edge case.

cc @nolanlawson @sorin-davidoi

Example

https://codepen.io/unarist/pen/rwBxrQ/

In this example, the element with intersectionRatio === 0 will be colored pink background. Elements you are seeing should not be colored because it is intersecting, but I can see sometimes while scrolling faster.

I've confirmed this issue on Chrome 61 on Windows.

The default value for the `threshold` option is `0`, which means notify
when intersectionRatio will be greater than 0. However, it generates
the entry which is `intersectionRatio === 0 && isIntersected` sometimes.
This causes some toots still being hidden.

This issue wouldn't occur if we use `isIntersecting`, but not now for
compatibility. So I've set `threshold` to clearly greater value
than `0` to avoid this edge case.
@ykzts ykzts requested a review from nolanlawson June 1, 2017 13:27
@ykzts ykzts added the bug Something isn't working label Jun 1, 2017
@sorin-davidoi
Copy link
Contributor

Ah, this is the bug I was talking about in #3365 but couldn't remember 😞

@sorin-davidoi
Copy link
Contributor

Wouldn't threshold: 1 cause problems if toots have a lot of content (big height) and the viewport height is small? In this case the toots may never be 100% visible. May not be a big deal with rootMargin: 300%, but if we decide to lower it it might be an issue. What about using something like 0.05?

@unarist
Copy link
Contributor Author

unarist commented Jun 1, 2017

Ah, yes. If the toot is longer than vertical margin, threshold: 1 would cause a problem. Also I should have changed a condition for isHidden, fixed it.

Probably threshold: [0, 0.01] and intersectionRatio > 0 would be perfect for hiding and showing, but it will call the observer twice. So I have decided to use threshold: 0.01 and check intersectionRatio >= 0.01 which should cover most cases.

@nolanlawson
Copy link
Contributor

I've done a bit of research, and the apparently the correct polyfill for isIntersecting is this: w3c/IntersectionObserver#69 (comment)

I could be wrong, but the 0.1 solution seems to me like it would have exactly the same problems as the old system (e.g. an element is intersectionRatio === 0.1 but isIntersecting is true, or maybe you end up with 0.1000001 or something, I dunno). I'd prefer if we just implement the correct polyfill for a threshold of 0.

@unarist
Copy link
Contributor Author

unarist commented Jun 2, 2017

e.g. an element is intersectionRatio === 0.1 but isIntersecting is true

Hence I've implemented as intersectionRatio >= 0.01 instead of intersectionRatio > 0.01. This may cause false-positive, but safer.

But the polyfill you mentioned looks correct and we have previous value already. I will try it.

@nolanlawson
Copy link
Contributor

I've tried to implement the recommended polyfill (https://github.com/nolanlawson/mastodon/commits/3502) but it seems even with this fix I can still reproduce the bug (albeit inconsistently – it's really hard to reproduce). This is hard. 😕

We might have to go with your 0.01 solution instead but I'd be shocked if there really wasn't a working polyfill for this.

@unarist
Copy link
Contributor Author

unarist commented Jun 2, 2017

Oh, really? I cannot reproduce this bug with the polyfill (although I've used prevState instead of create new field). Also can't you reproduce the bug with my 0.01 solution?

nolanlawson added a commit to nolanlawson/mastodon that referenced this pull request Jun 2, 2017
@nolanlawson
Copy link
Contributor

nolanlawson commented Jun 2, 2017

Actually @unarist I can't even seem to reproduce the original bug you filed. 😕 Which version of Chrome are you using, which operating system, and which scrolling method? (Chrome 61 on Windows, okay. That's an unreleased version of Chrome; maybe it has a bug?)

I'm trying Chrome 58 on macOS using both touchpad and sidebar scrolling, and I can't reproduce. I've even set a breakpoint for when entry.intersectionRatio === 0 && !entry.isIntersecting and it's not getting called…

@nolanlawson
Copy link
Contributor

I've also tested your CodePen and it seems it just turns elements pink when !entry.isIntersecting. It's fine if some entries are momentarily !intersecting because that's why we animate them from hidden to not-hidden. It seems unrelated to the bug you're describing.

@unarist
Copy link
Contributor Author

unarist commented Jun 2, 2017

Oh, what bug did you reproduce on below comment...?

I've tried to implement the recommended polyfill (https://github.com/nolanlawson/mastodon/commits/3502) but it seems even with this fix I can still reproduce the bug (albeit inconsistently – it's really hard to reproduce).

I have tested on below environment:

  • Windows 10
  • Chrome 58 (stable) / Chrome 61 (canary)
  • Scrolling with middle click / Track pad (which sends scroll event)

Also this bug seemed to occur on Android (https://mstdn.maud.io/@umezou/2357712)

it seems it just turns elements pink when !entry.isIntersecting

No, it checks entry.intersectingRatio:

    entry.target.className = !!entry.intersectionRatio;

@sorin-davidoi
Copy link
Contributor

sorin-davidoi commented Jun 2, 2017

I have observed it on Chrome 58 on Linux, scrolling with trackpad.

@sorin-davidoi
Copy link
Contributor

If this is only happening in Chrome, couldn't we use isIntersecting? If it is defined (Chrome and Firefox), use it, otherwise use the ratio (Edge).

@nolanlawson
Copy link
Contributor

Oh, what bug did you reproduce on below comment...?

The bug where a toot would occasionally look blank.

If this is only happening in Chrome, couldn't we use isIntersecting? If it is defined (Chrome and Firefox), use it, otherwise use the ratio (Edge).

I'm okay with this; I'm on the Edge team and so arguably it's my job to make sure this gets fixed. 😉 In the meantime checking typeof isIntersecting === 'boolean' and doing one logic for Chrome vs another logic for Edge seems fine. Then I can research and see how to fix Edge.

@nolanlawson
Copy link
Contributor

No, it checks entry.intersectingRatio

My mistake; in any case my point is that the codepen does not reproduce the bug AFAICT. But I also admit this bug is really hard to reproduce.

@akihikodaki akihikodaki added the ui Front-end, design label Jun 3, 2017
@Gargron Gargron closed this in #3525 Jun 3, 2017
@unarist unarist deleted the fix-intersection-observer branch June 5, 2017 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ui Front-end, design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants