[BUG] Scroll flickering with expandable cards

[cryptoethic]

Hi!

I got something hard to reproduce which cause us a lot of pain.

https://codesandbox.io/s/react-virtuoso-reverse-scrolling-header-issue-forked-fj5ys?file=/src/App.js

Sometimes when i scroll up/down reflow occurs and my scroll position is moved from place where I have been frame before.

I have prepared sandbox with expandable cards (onClick), and this gif showing the issue (it does not happen all the time but is frequent enough to be painful)

Any idea what can cause this issue? I will appreciate any help ;-)

[petyosi]

I am not sure how much this example follows your actual source code, but to optimize this scenario, you do need to apply proper memoization on a few levels, like the parameters of the Virtuoso component as well as the rendering of your items. Take a look at this fork for more details.

https://codesandbox.io/s/react-virtuoso-reverse-scrolling-header-issue-forked-txuow?file=/src/Row.js:997-1116

[cryptoethic]

In final project we use memoization to reduce rerenders and it works as expected

 const renderItemContent = (index, notification) => (
    <div className={classes.notificationCardWrapper}>
      <Suspense fallback={<NotificationSkeleton />}>
        <LazyNotificationCardMemorized
          data={notification}
          index={index}
          virtuoso={virtuoso}
          expandedCardHeightCache={expandedCardHeightCache}
        />
      </Suspense>
    </div>
  );

Memoization does not solve the flickering problem. This recording is on your forked sandbox:

[petyosi]

Perhaps I am missing something - can you be more specific about what do you all a scroll flickering? There is some scroll repositioning due to item resize, and in reverse mode, this is not fully avoidable.

[cryptoethic]

I couldn't find a specific case where scroll jump occurs, but what can be seen on the above gif is something like that:

1. I'm scrolling down and expanding cards
2. I'm collapsing bottom card while it is still fully in my view
3. I'm starting scrolling upwards and than jump occurs - I'm moved some cards to the bottom.

Unfortunately this does not occur every time, but is frequent enough to be uncomfortable. It happens frequent enough so you could reproduce this behavior.

[cryptoethic]

And here is an example where I only expand cards and scroll jump occurs when i start scrolling upwards.

[petyosi]

Which browser and OS is this? I am doing similar steps and it works on my side. Chrome, MacOS.

https://user-images.githubusercontent.com/13347/127023686-8c4546c5-d4fd-4e87-8351-bfc133d23b09.mov

[cryptoethic]

Hey, I'm using Windows 10 with latest Chrome.

It does not occur every time. My way to reproduce was expand 3 cards while scrolling, collapse bottom card, start scrolling up. Sometimes scroll jumped.

[petyosi]

I've seen such issues related to specific scrolling devices - mice with wheels where the inertia scrolling on windows does not get reported properly through the DOM events, there are a few closed issues related to these.

Do you have a mac available? if so, try to repro on it, so that I can confirm that this is the same problem.

[cryptoethic]

I will ask someone to confirm on macOS device. Meanwhile I can confirm that this behavior is also reproducible on android device (my phone) - I have opened codesandbox on Samsung Galaxy S8 device.

[petyosi]

Can't reproduce on Motorolla here: https://txuow.csb.app/

[cryptoethic]

Strange ;/

I have played for ~30 seconds with scrolling, expanding and collapsing and I got scroll jump ;/

Maybe I could look into code to see if I can find something. Is react-virtuoso somehow changing scrollposition at any moment?

[petyosi]

Yes, it does. In reverse scrolling, when it encounters an element with an unexpected size, it readjusts the scroll location.

https://github.com/petyosi/react-virtuoso/blob/master/src/upwardScrollFixSystem.ts

[cryptoethic]

Thanks, I will give it a closer look

[cryptoethic]

So i can confirm that it is caused by upwardScrollFixSystem

I have copied above sandbox to e2e directory and added console log with offset before publishing scrollBy and managed to record following:

[cryptoethic]

Ok so I have played with this system a little, and If i comment out these lines:

Everything starts working. I believe this system was developed for some specific use case. Maybe it shouldn't be always active.

In my case the issue was with latest items in list: On this image I'm logging out on each render index and current offset After expanding row 30, scroll a little bit down, than upwards and i got message that row 34 has different offset than previously newDev 2 is log from u.scan:

What I also noticed, when i commented out methods publishing scrollby, flickering still occurred. I had to also comment out system that was publishing deviationOffset to deviation stream.

Further steps - It would be great to know for what use cases this system has been created, and if it might by altered to handle my usecase with expandable cards properly as well ;-)

[cryptoethic]

I think (but it might not be true) that if offset change occurs for item that is below our current scrolltop position, browser reflow will do all things for us, and extra deviation push (and later scrollby) does no good.

So modifying this system as so:

should cover my use case and be safe to implement (hopefully)

@petyosi sorry for tons of messages. What do you think?

Edit: I have no clue how to properly pass scrollTop position to u.scan BTW i'm impressed by your urx module ;)

[petyosi]

Before we go nuts down this rabbit hole - can you try disabling animations? I feel like something might come down to the fact that scrolling is combined with the animation changing offsets.

[cryptoethic]

Worth giving a try. So I have replaced Collapse component with following div element:

 <div style={{height: ex? '100%' : '2px', overflow: 'hidden'}}>

And flickering still occurs, but this time we can spot that upwardScrollFixSystem has jumpt into action twice, and only once there was unintuitive scroll jump:

It worked as expected when offset was positive number (Card collapsed and stayed ad the bottom of screen). Behaved badly when number was negative

[petyosi]

Cool, we might be on to something here. scrollFix should not kick in, but it does. Can you push that into a branch somewhere? I will try to look into it in the next few days. I would appreciate it if you can as much additional config as possible that should not be not relevant to the example, like MUI components, overscan, headers, etc, and retest.

Thank you very much for your help and patience with the code.

[cryptoethic]

Yeap. I will do it tomorrow morning and let you know.

Thanks for helping me on resolving this issue!

[cryptoethic]

Ok, so I have removed code not relevant and published it here: https://github.com/cryptoethic/react-virtuoso/tree/fix-upward-scroll-system-flickering

I can confirm that bug still occurs after code cleaning.

[cryptoethic]

Hey!

Have you maybe found some time to take a closer look into this issue? If I could provide some further help just let me know ;-)

[petyosi]

Hey, no, I haven't. Busy daytime job ATM unfortunately - I will likely take a look in the following days though.

[petyosi]

@cryptoethic can you try this on your side?

https://github.com/petyosi/react-virtuoso/commit/0b727b6175fb4aa9dbc4cc5f46951160a3e2e44c

[cryptoethic]

Hey @petyosi.

I gave it a try and it does work but when adding initialTopMostIndex > 0 causes same error to exist. Here is an example where I start with initial topmost item index = 10.

Unfortunately we are using initialTopMostIndex in our app at some places.

[petyosi]

The entire setup in the system is related to reverse lists that start from a certain location rather than the top. It tries to detect items that have a height different than expected, and adjust the scroll location so that the list does not jump. There are a few test cases in the e2e directory that check that.

Expressing the above is easier in prose than in code. In certain moments, your test case (expandable cards) checks all the checks of the system, causing it to apply the re-positioning (which, in my case, works as intended - it keeps the items below correctly positioned). I did manage to reproduce the repositioning but not the jump in the gif. Chances are, this is related to a specific scroll optimization on Windows, where scrollBy calls are dropped.

I am afraid that, at this point, I can't help any further based on what I know. I have not discovered any way to overcome the scrollBy call dropping. Feel free to explore that further, there is sufficient amount of tests that illustrate what the intent of the current code is.

[cryptoethic]

Thanks for your time with this issue. I will spend more trying to solve this problem tommorow morning.

As I understand this system should jump in when we scroll upwards and there is new item loaded for the first time with height different than expected. Without that system our scroll position is pushed by layout reflow and there is a scroll jump.

If I'm right this system could execute only for the topmost loaded item as only this item can cause scroll jump. (so we have to check if item below the top has moved its offset) - there is no need for checking all loaded items.

What's more if initial topmost item index is zero then we can safely disable that system.

[petyosi]

Up to your assumptions:

loaded the first time - no. It can happen at any moment. Items can change size due to resizing, business logic change, etc. topmost item - no. Scrolling quickly or using overscan can introduce multiple items. topmost item index - I would say no as well. You can start from zero, but jump to 1000 (using scrollToIndex) and then scroll up.

[cryptoethic]

Thanks for your input. So what I can suggest is my first idea. execute scrollBy only if it is caused by items above our current scrollTop position. If it is caused by items below current scrollTop - browser reflow should handle correct scroll position.