Flickering when row height changes

mattwondra commented 11 months ago

Describe the bug When row heights change soon after the component mounts, it can cause on-screen rows to flicker.

Video demo: https://cloudup.com/cm6OIUfnGhP
Reduced test case: https://github.com/inokawa/virtua/compare/main...mattwondra:virtua:bug/flicker-when-height-changes

To Reproduce Generate components that change their height very soon after mount (e.g. they make a network request and render something based on it). As you scroll them into the overscan, you will often notice rows already rendered on the screen flickering (disappearing and then reappearing).

I've set up a reduced test cases in a branch on my fork, load Storybook and scroll around in the Default story to trigger.

Expected behavior Components that are already rendered to screen should not flicker at all, even if their height or other components' height changes.

Platform:

OS: MacOS
Browser: Chrome, Firefox, Electron
Version of react: 18.2.0
Version of this package: 0.16.4

Additional context Thanks for all your work on this package! Let me know if I missed something obvious here or if there's more I can do to help debug.

inokawa commented 11 months ago

It's strange that increasing overscan makes the situation worse!

I haven't figured out the reason yet, but in theory, this lib absorbs resizes outside of the viewport using shifting scroll position so huge shift might cause something bad. Setting min-height to img or item div may suppress the glitch as a workaround.

inokawa commented 11 months ago

I hope it was fixed in 0.17.0. Thank you for reporting it!

mattwondra commented 11 months ago

Thank you for your work to address this @inokawa! Unfortunately the changes made in 0.17.0 didn't fix it... I've rebased my forked branch on main so you can verify in Storybook.

I did a little more sleuthing and here's some more clues that will hopefully lead us to a solution:

1. Deferring scroll position jumping fixes this problem (but causes others)

If I update applyJump so all browsers use pendingJump, the problem is solved!

const applyJump = (j: ScrollJump) => {
  pendingJump += j;
};

However it creates at least one regression, with scrolling to the end of the list. You can see this in the "Reverse" story — it gets scrolled past the last item. This happens even without the reverse prop.

2. Safari does not have this bug

I tested in Safari, and both my original branch on 0.16.4 and the current version on 0.17.0 do not have this bug! I noticed that on item resize, diff is always 0 so applyJump never seems to be called.

3. `onScrollStop` is called twice when the bug happens

I stumbled on this while console.log debugging. When scrolling down and the bug gets triggered, onScrollStop gets called twice. When I dug a little deeper, it's because when scrolling stops for some reason scrollDirection goes from SCROLL_DOWN to SCROLL_IDLE (expected) and then to SCROLL_UP and then SCROLL_IDLE again.

So for some reason it thinks I scrolled back up again, when I didn't.

4. Items outside the viewport are remounted without range changes

This may be unrelated and should be filed as a separate bug, but — on very small scrolls that don't change which items are visible (onRangeChange doesn't fire), a bunch of the off-screen overscan items are re-mounted. This seems like a bug, those components are already mounted and there shouldn't be any need for them to be destroyed and re-mounted when the range hasn't changed.

inokawa commented 11 months ago

I didn't notice that. As far as I've tested it again, feels less often but certainly it still happens. And thank you for your investigation! It may be better to publish a build for debug like react-virtuoso...

I'm not sure it is solvable problem but I try to fix it as much as possible. What I can say for now is:

1 will defer adjusting scroll position so the adjusting itself may be the cause. That mode is a workaround for iOS Safari, which cause serious junk if we didn't defer it.
2 sounds strange... 🤔
3, 4 are expected behavior but it seems that they have side effects for this issue.
- If resize happens, virtua adjust scroll position to suppress it. Scroll direction is determined based on scroll events, but adjusted scroll position also included in the scroll events so it's difficult to distinguish between them. Currently some of the suspectable events are ignored but it might not be enough.
- onRangeChange returns range of visible items so it can be different from actually mounted range. The overscaned items opposite to the scroll direction will be unmounted during scrolling to reduce cost. That's similar behavior to react-window.

inokawa commented 11 months ago

@mattwondra Could you try #255 to see if it makes your situation better? Thanks.

mattwondra commented 11 months ago

@inokawa Eureka! That completely fixes the content flickering on Mac Chrome / Firefox, and confirming Safari still works too! The scrollbar still jitters (see videos below) but that isn't nearly as noticeable. Before/after videos:

`main` (at ae98ff71514d73d745648e0fa5cd6be2082ced10)

Notice content and scrollbar flickering:

https://github.com/inokawa/virtua/assets/518059/6ddab0ba-11fc-4120-94ab-3fb7fec0e9aa

`ignore-scroll-jump`

Content is smooth! Just scrollbar flickering:

https://github.com/inokawa/virtua/assets/518059/2a02d2c2-8703-482e-99ab-23699325e29c

inokawa commented 11 months ago

Thank you for confirmation! I'll publish this fix in the next release.

inokawa commented 11 months ago

Released in 0.17.2.

inokawa / virtua