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

[BUG] On React without concurrent rendering, render prop produces ResizeObserver errors on Firefox #1136

Closed
johannkor opened this issue Dec 14, 2023 · 6 comments · Fixed by #1137
Labels

Comments

@johannkor
Copy link
Contributor

Bug description

When using React <18, or on React 18 and not using concurrent rendering (using ReactDOM.render() instead of createRoot().render(), use of the render prop produces errors on Firefox. This error is easily and consistently reproduced by rendering a list of elements which all use the same data-tooltip-id and data-tooltip-content. The error produced is:

ResizeObserver loop completed with undelivered notifications.

without a stack trace, which is caught by window.onerror handlers (and thus ends up in Sentry and other monitoring platforms).

Firefox also issues the following warning in the console:

Layout was forced before the page was fully loaded. If stylesheets are not yet loaded this may cause a flash of unstyled content.

The app doesn't crash, but the error happens on almost every tooltip hover change (move cursor from element A to element B quickly), and thus produces thousands of errors per session per user on a tooltip-heavy site.

The error can of course be ignored with a regex as it does not crash the app or the tooltips, but clearly there is something wrong.

Version of Package

I have tested the following versions:

  • v5.25.0 - Reproduces
  • v5.24.0 - Reproduces
  • v5.19.0 - Reproduces
  • v5.15.0 - Reproduces
  • v5.12.0 - Reproduces
  • v5.10.1 - Cannot reproduce, or library is just so slow to change tooltips that it doesn't happen

To Reproduce

  1. Run the example

    import React from "react";
    import ReactDOM from "react-dom";
    import { Tooltip } from "react-tooltip";
    // using `disableStyleInjection="core"` to ensure it's not related to the recent
    // style injection features
    import "react-tooltip/dist/react-tooltip.css";
    
    const TOOLTIP_ID = "tooltip";
    
    function App() {
      return (
        <div className="App">
          <Tooltip
            disableStyleInjection="core"
            id={TOOLTIP_ID}
            render={({ content }) => <span>{content}</span>}
          />
    
          <div>
            {Array(50)
              .fill(0)
              .map((_, i) => (
                <div
                  key={i}
                  data-tooltip-id={TOOLTIP_ID}
                  data-tooltip-content={TOOLTIP_ID}
                >
                  row {i}
                </div>
              ))}
          </div>
        </div>
      );
    }
    
    ReactDOM.render(<App />, document.getElementById("root"));
  2. When the app is running, quickly swipe your cursor up and down the middle of the page

    • Swipe so that the cursor is over the tooltips, and swipe quickly enough that the tooltips don't have enough time to fully appear
    • Swipe from the top of the page to the bottom of the page quickly for maximum effectiveness
  3. Notice the error in the webpack dev server's error overlay which pops up

This bug also appears with normal usage (non-furious mouse swiping), but I found this to be the most effective way to reproduce it.

Expected behavior

I prefer this error not to occur, as it forces us to ignore the ResizeObserver error which hides other, unrelated ResizeObserver errors from us.

Desktop (please complete the following information if possible or delete this section):

  • OS: macOS
  • Browser: Firefox
  • Version: 102.0.1
  • Frameworks: React 16, React 17, React 18 (when not using concurrent mode)

Additional context
I have managed to reproduce this with Chrome in a larger application, but I have not been able to reproduce the error in the MVP above with Chrome.

@gabrieljablonski
Copy link
Member

After some looking around, I found this comment suggesting using setTimeout() on the observer callback, which seems to have fixed it. See #1137 and react-tooltip@5.25.1-beta.1137.0

I don't think we should merge this just yet, in case this introduces some other issues. We'll do some testing later on.

The beta version should be fine to use though.

@johannkor
Copy link
Contributor Author

I think that might fix the error, but I'm concerned whether that is because it fixes the root cause, or if it only makes the browser unable to recognize and stop a resize loop. I believe that the "loop completed with undelivered notifications" error is caused by the updateTooltipPosition() function causing a resize, a layout change, which the ResizeObserver calls. I suspect that the this doesn't reproduce when concurrent mode is on, because React can cancel and batch those updates that happen very quickly.

I'm also slightly concerned about these 10ms setTimeouts in the codebase - I suspect that when throwing the cursor around, it's reasonable to believe that the cursor jumps over two different tooltipped elements within this time.

setTimeout(() => {
if (!mounted.current) {
return
}
setIsOpen?.(value)
if (isOpen === undefined) {
setShow(value)
}
}, 10)
}

@johannkor
Copy link
Contributor Author

johannkor commented Dec 15, 2023

I used git bisect to find that the first commit where this occurs is 8e6c79c, introduced in #1009 (v5.10.6). Prior to this commit, I'm unable to reproduce it, but it's possible that the bug fix just revealed this existing condition.

@gabrieljablonski
Copy link
Member

I genuinely appreciate your effort in figuring this out, but I'm 95% sure this isn't a real cause for concern. I have some tests in mind to demonstrate that, but I'll only be able to get to my computer later on today. In summary, if you have problematic code causing an issue with ResizeObserver, you will get the error thrown, but having the error thrown doesn't mean you have problematic code.

I'm also slightly concerned about these 10ms setTimeouts in the codebase

If I recall correctly, this has been tested and there were no issues with it as far as we could tell. I'll try to demonstrate later as well.


Again, I really appreciate your effort on this, and I welcome you to keep making suggestions on things we could improve. So thanks!

@gabrieljablonski
Copy link
Member

First regarding your concern about the 10ms timeout. Since that's the only place the open state gets changed (at least when it's being fully handled internally), all open/close actions are queued on the event loop due to using setTimeout(), so there's no worry about them getting "desynced".

Here's what happens if we change that 10ms to a full second.

tooltip.mp4

Now about the ResizeObserver loop issue, unfortunately I was unable to quickly create an example that "proves" the tooltip is not doing anything too crazy on the callback, so I'll just defer to the ResizeObserver documentation.

Relevant sections (emphasis mine):

Resize events that don't meet that condition are deferred to the next paint, and an error event is fired on the Window object [...].

As long as the error event does not fire indefinitely, resize observer will settle and produce a stable, likely correct, layout. However, visitors may see a flash of broken layout, as a sequence of changes expected to happen in a single frame is instead happening over multiple frames.

According to its own specs, the observer pushes any further changes to the next render cycle, which I'd argue is similar to what we're doing by wrapping the callback with a setTimeout(). Since, at least as far as I can tell, the layout is not getting broken, and there are no other visible issues with the tooltip because of this, I'd call it a non-issue.

Feel free to object to any of these conclusions, since I haven't demonstrated they're actually true, just that it's most likely the case.

Though unless there's any actual impact to the performance or usability of the tooltip, I'd just leave this as-is if I were you. Even if this is "fixable", it seems like way more trouble than what it's worth.

Again, thanks for the effort.

@gabrieljablonski
Copy link
Member

Official release react-tooltip@5.25.1 fixes this and some other stuff.

If you still feel like there's something to improve here, mainly about this part from my previous comment:

Though unless there's any actual impact to the performance or usability of the tooltip, I'd just leave this as-is if I were you. Even if this is "fixable", it seems like way more trouble than what it's worth.

feel free to reopen the issue with more details.

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

Successfully merging a pull request may close this issue.

2 participants