segmentio/evergreen

Popover disappears when rendered and received a single scroll event

dengribar opened this issue · 3 comments

dengribar commented

👋 After 6.13.1 update, one of our automated integration tests started failing in the following scenario:

Popover shows up at the same moment when scrolling takes place which results in the Popover becoming invisible. After looking into this issue, I've found the following:

  • we subscribe on "scroll" event here, assigning update function as the event handler
  • when scroll event happens, we pass an event object as prevHeight param to the update function (ref)
  • if hasEntered is false in the update function, then we calculate height as Math.max(number, Object), which results in NaN (ref)
  • that results in setDimensions setting height and top as NaN making the Popover disappear from screen (ref)

I'm pretty sure that height = Math.max(number, Object) // NaN was not intentional, and seems like a bug to me.

Console output image
brandongregoryscott commented

@dengribar Is this reproducible in a CodeSandbox (or can you provide your test code that's failing)? https://codesandbox.io/s/create-react-app-evergreen-starter-ts-3q99w

The code that is subscribing to resize/scroll events has been there for a bit now (but you're right, we probably shouldn't be passing update directly as event handlers), but may be being hit in a different situation from the fix for the Tooltip flickering #1550.

dengribar commented

Better late than never, but I was able to reproduce the bug that happens in our automated tests:

https://codesandbox.io/s/evergreen-ui-6-12-0-popover-works-fine-on-scroll-ws9xox?file=/src/App.tsx

evergreen-ui-6.mov

Not exactly, though, since on v6.12.0 our automated tests work fine, but the sandbox example still doesn't work.

On the other hand, on v7.0.0, despite the same NaN value for top property warning, the popover is displayed in an appropriate position:

evergreen-ui-7.mov

Anyway, even though v7.0.0 kind of patches the issue somehow, the bug with passing an event object as prevHeight param to the update function (ref) needs to be fixed, and I hope that it will properly patch the issue. Thanks!

  • ❤️1