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

Scroll jank when prepending async relay pagination data #248

Closed
uncvrd opened this issue Dec 27, 2020 · 16 comments
Closed

Scroll jank when prepending async relay pagination data #248

uncvrd opened this issue Dec 27, 2020 · 16 comments
Labels

Comments

@uncvrd
Copy link

uncvrd commented Dec 27, 2020

Hi! Great work on this library - really appreciate the standard support for inverse (chat) style scrolling. I have run in to a small issue when prepending data that comes from an asynchronous source that I believe draws from the small delay between when the data is appended and when firstItemIndex is changed and I was wondering if you had any suggestions?

For example, here's a video of the jank I am experiencing: https://streamable.com/x1q88p

You'll notice at around the 0:02 second mark there is some jitter as the data has been loaded but the start index has not been updated just yet. That animation jitter you see is actually the new data that was loaded before the scroll is pushed back down to where it should be by the firstItemIndex. The code to merge new data into the existing data array handled by Apollo is kind of long which is where I think the problem stems from as I explain below. Is there a way to update both the data and the firstItemIndex at the same time?

Here's my code, I am using Apollo Server relay style pagination (edges and nodes). FETCH_ITEM_COUNT is a constant that is set to 20:

<Virtuoso
    firstItemIndex={firstItemIndex}
    initialTopMostItemIndex={FETCH_ITEM_COUNT - 1}
    data={data?.artistMessages?.edges || []}
    startReached={fetch}
    itemContent={(index, message) => {
    return (
        message?.node && (
        <ChatMessage message={message.node} key={index} />
        )
    );
    }}
/>

I took influence from your prepending example and used useState to initialize a firstItemIndex like so:

const [firstItemIndex, setFirstItemIndex] = useState(START_INDEX);

Where START_INDEX is a constant set to an arbitrarily large value: 1,000,000.

When start is reached, I have a fetch method to that loads the next batch of data (cursor based pagination) like so:

const fetch = useCallback(
async (index: number) => {
    // check to confirm that there is data to query for before running the fetch
    if (fetchMore && data?.artistMessages?.pageInfo.hasPreviousPage) {
    const items = await fetchMore({
        variables: {
            last: FETCH_ITEM_COUNT,
            before: data.artistMessages?.pageInfo.startCursor,
            artistId: parseInt(artistId),
        },
    });
    // items contains the new elements, subtract the new items from the firstItemIndex
    setFirstItemIndex((curr) => {
        return curr - (items.data.artistMessages?.edges?.length || 0);
    });
    }
},
[data, fetchMore, artistId]
);

I call fetch and receive a new batch of data to prepend to my list. Notice how I do not have a useState for the data attribute of Virtuoso? This is because Apollo Server Relay pagination handles updating the data.artistMessages.edges internally. I'm thinking this is where the jank comes from since the code to prepend relay data is kind of lengthy see here. Apollo writes to client cache so that might be where the delay is coming from.

Now with the full scope of this issue, do you think it possible to be able to update the firstItemIndex and data at the same time? I may not fully understand the rendering process in React but I'm led to believe that having two points that need to be updated is causing the temporary misalignment of data on screen.

I thought I could utilize my datasource length to prevent needing a firstItemIndex state hook like so:

firstItemIndex={1000000 - (data?.artistMessages?.edges?.length || 0)}

However this results in an error on page load:

upwardScrollFixSystem.ts:93 Uncaught TypeError: Cannot read property 'originalIndex' of undefined

Let me know if there's anything else I can provide!! Thanks :)

@petyosi
Copy link
Owner

petyosi commented Dec 27, 2020

Sorry - can't help much here. Setting data and firstItemIndex them with subsequent calls seems to work fine in the example. Even if there's an 'incorrect' frame, it is likely a short one. I am not an Apollo user, so I can't advise on how to sync the prop values correctly.

@petyosi petyosi closed this as completed Dec 27, 2020
@uncvrd
Copy link
Author

uncvrd commented Dec 27, 2020

@petyosi yes it likely has to do with the delay when writing to apollo's cache which I suppose will not work with this library with the dependance on a firstItemIndex being required on the component. Regardless thanks for advising!

@petyosi
Copy link
Owner

petyosi commented Dec 27, 2020

If you find a solution, please post it here; extending the library examples with data fetching strategies is something I want to do for a long time, but I am intimidated by the number of options out there!

@uncvrd
Copy link
Author

uncvrd commented Dec 27, 2020

No problem - totally understand that! Will do :)

@uncvrd
Copy link
Author

uncvrd commented Dec 27, 2020

@petyosi hey! ok I've recreated this problem in a codesandbox with a generic async request which highlights the scroll jank (Apollo is not included), it doesn't always happen because we may be getting lucky with the render loop. Having two properties (firstItemIndex and data) that need to be updated at the exact same time to prevent render jank is what I imagine is causing problems. Happens most often when scrolling slowly.

Streamable showing the issue: https://streamable.com/u3000q

Link to codesandbox: https://codesandbox.io/s/modern-meadow-24ljr?file=/src/Scroll.js

I hope this helps!!

@petyosi
Copy link
Owner

petyosi commented Dec 27, 2020

Thank you, I see what you mean. IMO, the jump is less severe than the initial recording, so I guess there is a space for optimization in the async scenario, too.

Apart from that, I am not sure that this is completely unavoidable. The prepend implemenation happens in two steps - a react re-render and an element.scrollTo method call. I can't think of how the two can be forced into a single frame, but maybe there is a way.

@uncvrd
Copy link
Author

uncvrd commented Dec 27, 2020

You're welcome! And yes I completely agree with it being less severe in this scenario due to no cache write delay when using Apollo Client. I understand the complication of the two step process, I'll brainstorm some ideas as well

@uncvrd
Copy link
Author

uncvrd commented Dec 28, 2020

I found a vue virtual scroll component that appears to handle prepending async information without the jank. I'm not too familiar with vue so it'll take some time for me to get a handle of how this works, but thought I'd pass this along for inspiration. https://tangbc.github.io/vue-virtual-scroll-list/#/chat-room

petyosi added a commit that referenced this issue Dec 28, 2020
Fix #248

Caveat of this approach is that if
the list is scrolled to the bottom, the jump won't work.
@github-actions
Copy link

🎉 This issue has been resolved in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@petyosi
Copy link
Owner

petyosi commented Dec 28, 2020

@uncvrd - I think the change in 1.2.0 improved the behavior. Let me know what you think.

@seahorsepip
Copy link
Contributor

I still see a jump once in a every few load more on scrolls at https://virtuoso.dev/prepend-items/

If I understand correctly the implementation is as following:

  1. Scroll to top
  2. Load more items
  3. Insert items
  4. Wait for item heights
  5. Scroll top is compensated for new item heights

The jump probably happens around step 4.

What about wrapping new items in 0 height containers so they don't affect the list height/offsets until they have been measured? Then it would something like the following:

  1. Scroll to top
  2. Load more items
  3. Insert items (0 height wrapper)
  4. Wait for item heights (height inside the 0 height wrappers)
  5. Scroll top is compensated for new item heights, remove 0 height from item wrappers just before changing the scroll top.

@petyosi
Copy link
Owner

petyosi commented Dec 28, 2020

@seahorsepip can you record a video? I can't see a jump, but I am probably missing something. Introducing measurement containers is not something I want to deal with ATM, as it causes too much complexity.

@seahorsepip
Copy link
Contributor

@petyosi
Copy link
Owner

petyosi commented Dec 28, 2020

@seahorsepip Thanks, what's the browser/OS?

@seahorsepip
Copy link
Contributor

@petyosi Window 10 (latest insider slow ring) with latest stable Chrome. It's a 4k screen running most of the time on the intel integrated GPU so performance is a bit more limited unless I force the Nvidia GPU 😅.

@seahorsepip
Copy link
Contributor

seahorsepip commented Dec 28, 2020

@petyosi
Also keep in mind that there might be still scroll events emitted when the end is reached due to inertia from most touchpad drivers that might shift the scroll between measure and scroll compensation.

Never mind, also happens when I scroll with a mouse which doesn't have inertia.

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

No branches or pull requests

3 participants