segmentio / evergreen

🌲 Evergreen React UI Framework by Segment
https://evergreen.segment.com
MIT License
12.38k stars 830 forks source link

Popover disappears when rendered and received a single scroll event #1556

Closed dengribar closed 1 year ago

dengribar commented 1 year ago

👋 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:

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 1 year ago

@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 https://github.com/segmentio/evergreen/pull/1550.

dengribar commented 1 year ago

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

https://user-images.githubusercontent.com/111731892/213792623-f8a6104e-47f9-4dd4-b5ca-90529074a568.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:

https://user-images.githubusercontent.com/111731892/213793030-449fa353-34ba-4886-aa16-2c620791a76f.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!

brandongregoryscott commented 1 year ago

This should be fixed in https://github.com/segmentio/evergreen/releases/tag/v7.1.3!