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

Safari desktop triggers endless loop #308

Closed
gabrieltomescu opened this issue Feb 24, 2020 · 17 comments
Closed

Safari desktop triggers endless loop #308

gabrieltomescu opened this issue Feb 24, 2020 · 17 comments

Comments

@gabrieltomescu
Copy link

gabrieltomescu commented Feb 24, 2020

Hi!

I'm using inView to detect if a container is in view, then setting this property to a secondary container that should be visible when the first one is out of view.

In Chrome this works fine, but in Safari I'm getting a endless loop of inView being toggled between true and false when my scroll position is at the edge of the intersection point.

navbar

Any help with this?

const [primaryContainerRef, inView] = useInView();

<div className="primary-container" ref={primaryContainerRef}>
       This is the primary container
</div>

<SecondaryContainer isVisible={!inView} />

const SecondaryContainer = ({ isVisible }) => {
   return (
      {isVisible && (
          <div className="secondary-container">This is the secondary container</div>
     )}
   )
};
@thebuilder
Copy link
Owner

I would try setting rootMargin and/or threshold to modify when inView triggers.
The bug you are seeing is most likely related to how safari handles intersection observers natively.

@gabrieltomescu
Copy link
Author

Thanks, I tried both of those, but no luck.

@thebuilder
Copy link
Owner

Can you replicate it in a Code Sandbox?

@giannif
Copy link

giannif commented Feb 28, 2020

I'm seeing an infinite loop as well, I tracked it down to the upgrade from 8.25.3 to 8.26.0. I see it on Chrome desktop.

@designbyadrian
Copy link

designbyadrian commented Mar 18, 2020

I'm experiencing performance degration on Chrome 80.0.3987.132 with react-intersection-observer 8.26.1

Same with render prop and useInView

@designbyadrian
Copy link

designbyadrian commented Mar 18, 2020

@gabrieltomescu I'm currenly using NextJS. I attached the ref to an anchor tag inside a Next/Link component:

import Link from 'next/link';

...

<Link to="/">
<a ref={ref}>Some component</a>
</Link>

Got rid of the issue when applying to a child component to <a>. Will try to mimic in CodeSandbox.


Edit: CodeSandbox example here: https://codesandbox.io/s/nextjs-o0783

Move the ref to the child div to stop the infinite loop.

@thebuilder
Copy link
Owner

@gabrieltomescu I'm currenly using NextJS. I attached the ref to an anchor tag inside a Next/Link component:

import Link from 'next/link';

...

<Link to="/">
<a ref={ref}>Some component</a>
</Link>

Got rid of the issue when applying to a child component to <a>. Will try to mimic in CodeSandbox.

Edit: CodeSandbox example here: https://codesandbox.io/s/nextjs-o0783

Move the ref to the child div to stop the infinite loop.

The Codesandbox example goes into an infinite loop because of a missing next/router

@macrozone
Copy link

I also see a massive performance degeneration with chrome 80.

useInView seems to rerender every frame

@thebuilder
Copy link
Owner

Is it possible to get a Codesandbox that showcases this issue? I'm not seeing it, so i need to know how you are configuring it to trigger the issue.

Edit react-intersection-observer

thebuilder added a commit that referenced this issue Apr 24, 2020
* fix: don't clear the inView state if we get a node #308

* fix: use a useEffect to trigger the cleanup
@macrozone
Copy link

can confirm that 8.26.2 fixes the issue in chrome 80 (and 81)

@thebuilder
Copy link
Owner

Can you share the code that caused the issue? Would like to know if it's related to switch ref target.

@macrozone
Copy link

Can you share the code that caused the issue? Would like to know if it's related to switch ref target.

its nothing special, i did not change the ref target. If it helps: i assigend the ref usually to a component that was created with styled-components

@macrozone
Copy link

the error seem to have reappeared in chrome 81. :-(

@valerie-roske
Copy link

@thebuilder I'm on version 8.26.2 and I'm experiencing the same issue as @gabrieltomescu in Safari only. The code is set up the same way.

This issue seems to occur for me only when the observing element is sticky to the top (using position:sticky), and overlapping the observed element. (The gif demonstrates this well.) When the observer is sticky at the bottom, this flickering does not occur. The flickering also doesn't occur when using position:fixed.

@thebuilder
Copy link
Owner

@valerie-roske Can you make a Codesandbox demo of the issue?

@valerie-roske
Copy link

@thebuilder here you go: https://codesandbox.io/s/react-intersection-observer-p9ide

So another observation as I was making this demo: the order on the DOM is relevant. In my example, the flickering occurs only when the observer element is inserted above the element it's observing. So for anyone coming across this issue, potential solutions:

  • Make sure the observer element is loaded in an appropriate place in the DOM
  • If you are conditionally rendering the observer anyway, perhaps position: fixed is a better choice than position: sticky
  • If you aren't conditionally rendering, then you may not even need an IntersectionObserver in the first place and can go ahead with position: sticky all by itself

Not sure if this is exactly what's going on in the original post, but I hope this helps. :)

@thebuilder
Copy link
Owner

Thanks for the sandbox @valerie-roske - It seems to be an issue in all browsers, but I think the library is actually acting correctly. When you attach the new element with position sticky it causes a layout shift. This in turn causes the IntersectionObserver to trigger again. In Firefox and Chrome it only seems to have once per scroll event, so it causes flickering while scrolling, with Safari going into an infinite loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants