Flipkart / recyclerlistview

High performance listview for React Native and web!
Apache License 2.0
5.19k stars 426 forks source link

Fix scroll position for dynamic contents #127

Open sm2017 opened 6 years ago

sm2017 commented 6 years ago

I want to create an infinite scroll view using recyclerlistview in both direction (up and down)

When I load more contents , the following code executed

   const dataProvider = new DataProvider((r1, r2) => {
            return r1 !== r2;
        });
  this.setState({
            dataProvider: dataProvider.cloneWithRows(myContents)
   });

Here the scroll offset will reset to zero and scroll go to the top

I use scrollToOffset to change the offset to getCurrentScrollOffset()+heightOfNewContents when scrolling to up direction , scrollToOffset works accuracy well but I see a small view refresh , the view become blank for a fraction of second

I find that changing dataProvider is not the cause , scrollToOffset is the reason because If I don't change data and just call scrollToOffset I see the view become blank for fraction of second , I test with a simple PureComponent and I'm sure scrollToOffset is problem not the CellContainer view itself

AbdallaElabd commented 6 years ago

You shouldn't be creating a new instance of the DataProvider. Just use the existing one.

this.setState(prevState => ({
  dataProvider: prevState.dataProvider.cloneWithRows([])
}));

Note that if you add items to the top of the list the scroll position won't be maintained though. You could try to do that imperatively.

naqvitalha commented 6 years ago

Remember scrollToOffset is an async call. So it might be difficult. In any case I would need a snack repro to look into this. @AbdallaMohamed you are right although cloneWithRows will also create a new data provider but it optimizes layout recompute.

sm2017 commented 6 years ago

@AbdallaMohamed I test your approach and see that is not working as I expected

When the scroll reaches at top I add items to the top of the list and if before loading items you see 100th item I want scroll show 100th item after scrolling , but it show top item

Can you show me an example code?

AbdallaElabd commented 6 years ago

@sm2017 I'm doing an ugly workaround right but at least it preserves the scroll position.

Before setting the state with the new dataProvider. I get a reference to the top row in the list. Then I set the state, and afterwards, I calculate the index of the top row and then I call scrollToIndex.

      const topRow = this.getTopRow(); // calculate the index of your top item here
      this.setState(
        prevState => ({
          dataProvider: prevState.dataProvider.cloneWithRows(newRows)
        }),
        () => {
          // Persist the scrolling position
          const topRowNewIndex = this.getIndexOfRow(topRow)... // calculate the new index of the item after it's been pushed down
          setTimeout(() => {
            this._recyclerListView.scrollToIndex(topRowNewIndex);
          });
        }
      );
sm2017 commented 6 years ago

@AbdallaMohamed your approach don't works as I expected

@naqvitalha I will send a working expo demo asap

AbdallaElabd commented 6 years ago

@sm2017 I think what you're describing is the same issue that I struggled with before. Check out this snack and let me know if it helps.

https://snack.expo.io/@abdallamohamed/preserving-scroll-position-with-recyclerlistview

sm2017 commented 6 years ago

@AbdallaMohamed your expo code preserves scroll position (but not accuracy in pixel) But the problem is when you load more data in a small moment your view refreshes and we see blank areas for milliseconds

AbdallaElabd commented 6 years ago

@sm2017 Yeah, it's a compromise I had to accept. I'd love to see what @naqvitalha has to say.

naqvitalha commented 6 years ago

Try this https://snack.expo.io/S1zKr6tPf. Even though it's a hack (uses private api) but it certainly proves that it is possible. Also, note none of these solutions work in between a scroll. Maybe we can transform the whole list instead to correct the offset. Post the animation work we will be introducing stableIds which will improve this further and the flicker will go away automatically without increasing renderAheadOffset like I did in the sample.

sm2017 commented 6 years ago

@naqvitalha I cannot understand why renderAheadOffset is very important here

naqvitalha commented 6 years ago

It removed the flicker, the reason for it was that items are moved to that location post the manual scroll request. Since this is async there is one frame where there are no items. In the the renderAheadOffset ensures that there is something drawn already there.

sm2017 commented 6 years ago

To remove the flicker you generate 10 items each time and set renderAheadOffset equal to 10xdim.height = 1000 , what about forceNonDeterministicRendering=true is it working?

Can renderAheadOffset be dynamic because may be one page has 10 items another has 2 items

sm2017 commented 6 years ago

@naqvitalha is there any tricks for forceNonDeterministicRendering=true

sm2017 commented 6 years ago

@naqvitalha Help please

sm2017 commented 6 years ago

@naqvitalha 😭 help needed!

naqvitalha commented 6 years ago

@sm2017 if you can cache layouts yourself it might work. The same solution will work only if you know exact heights. Why don't you start caching them externally?

sm2017 commented 6 years ago

@naqvitalha How exactly?? Assume I have added items 900-999 in data provider and when user scroll to top I prepend items 850-899 so as I cannot know exactly the heigh of new items , How can I use cache approach ?

At first time how can I read from cache as there is no cache?? Must I render items outside view? And mount again inside?? It is expensive

tafelito commented 6 years ago

@naqvitalha I just tested your snack code but in a web project. It seemd not to be working as native. What I saw debugging it is that the scrollcomponent does not have a ref here when calling the scrollToOffset I am also interested in know if using forceNonDeterministicRendering=true will work

naqvitalha commented 6 years ago

@tafelito Try 1.3.0-beta.11 that was an old bug. Will be fixed in 1.3.0 which is being tested internally. @sm2017 What is your majority use case? Is it like a chat app? Remember chat can be done using the inverted mode that you can easily build. Caching strategy can work to some extent but it will never be perfect in non deterministic mode. This is something we need to work on. You can try the expensive approach for now I guess but do cache layouts.

naqvitalha commented 6 years ago

@tafelito check https://codesandbox.io/s/lr20lpypvq

tafelito commented 6 years ago

@naqvitalha Awesome that worked and I like it works with react-native-web too because right now that wasn't working either.

regarding @sm2017 request, I'm also thinking on how to make it work even if using inverted for a chat app. Not every message in a chat has the same height. So how do you now the renderAheadOffset value beforehand? Or if you have a feed like twitter's

Also regarding the inverted prop, I gotta finish what I started before you moved everything to Typescript if you are not working internally.

sm2017 commented 6 years ago

@naqvitalha I need for chat app but inverted prop is not solution because , I have both up/down direction infinite scrolling

You can scroll up to we history You can have many unread message and scroll jump to first unread message , then you can scroll down and fetch new messages from server in lazy approach

naqvitalha commented 6 years ago

You can fetch new messages on coming down and smoothly insert using animations. Wouldn't that help? See I do have a solution for this but that is still quite far away. I'm thinking to transform entire list to lock scroll positions. I'm sure that will work but I can't start doing that because I need to do sticky headers and stableId work before that. You can experiment around it though.

sm2017 commented 6 years ago

@naqvitalha Can you share more details about transform entire list to lock scroll positions

AbdallaElabd commented 6 years ago

@naqvitalha I'm loving what you guys are doing with this library! Just out of curiosity, how do you intend to implement sticky headers?

sm2017 commented 6 years ago

I do have a solution for this but that is still quite far away. I'm thinking to transform entire list to lock scroll positions. I'm sure that will work but I can't start doing that because I needs to do sticky headers and stableId work before that. You can experiment around it though.

@naqvitalha Have you plan to implement lock scroll positions feature? Can you share some details about your idea? When you think it is implemented?

sm2017 commented 6 years ago

@naqvitalha Reply please

naqvitalha commented 6 years ago

@sm2017 Let's say my first visible index was 5, I can compute it's offset from top and after inserting items I can transform the whole list such that the position of the same item from top stays the same. This will work in the middle of a scroll as well. Now as soon as the scroll stops I can fix the offset by removing the transform and calling scrollTo.

sm2017 commented 6 years ago

@naqvitalha why when I append data to data provider just some items are rendered but when I prepend whole items are renered

tafelito commented 6 years ago

@naqvitalha I just saw that on the February release of RN, the ScrollView now includes a property maintainVisibleContentPosition that they implemented here and here. I haven't had the chance to fully reviewed it, and it's only iOS, but it might be the same logic you described it. Have you seen it?

AbdallaElabd commented 6 years ago

I was just going to comment with a link to that commit, @tafelito Would love to hear @naqvitalha's thoughts on this

naqvitalha commented 6 years ago

@tafelito @AbdallaMohamed Doing it in native makes sense, however an acceptable implementation should be possible in JS too. Plus this can never work on web and we want to solve that too. What I don't understand is why use contentOffset do they intend to make it iOS only?

naqvitalha commented 6 years ago

@tafelito it looks like the same logic

tafelito commented 6 years ago

@naqvitalha I think right now yes it only works for iOS but I read they are also working on the Android version so they might change that in the future. But as you said, it'd definitely make more sense doint it in JS whenever is possible so it works for both web and native

Kimotn commented 6 years ago

Hello, @sm2017 @naqvitalha @AbdallaMohamed same problem, any solution for this. I'm work with dynamic height 100, 120, 60, 300 , .... if i'm load more items and lock scroll position don't work . Please Help and Thanks .

tafelito commented 5 years ago

@naqvitalha were you able to take a look at this? I'm still trying to find a smoother way to lock scroll position when loading dynamic content but everything I tried seems like a hack

immortalx commented 4 years ago

Version 2.0.13-alpha.1

If i understand the issue, I'm having the same problems with a list of fixed size items. It's a grid of pictures. Whenever a push new data the scroll position goes up a little causing a "flick", "bump" effect. This is the normal behavior? I have this working with Flatlist, since I'm new to this library, I'm not sure if is up to me to handle this.

That's the only issue so far, the performance is really great even with thousands of pictures.

nikolaigeorgie commented 4 years ago

@immortalx i fixed that issue by making a custom scroll view and removing the scroll to method on it . It seems to be embedded into the recycler list out of the box

GSManganneau commented 4 years ago

hey @nikolas7892 , thanks for your answer. Could you please give more details about the way you fixed it please ? I have the same issue

nikolaigeorgie commented 4 years ago

@GSManganneau Despite my fixes I've had so many issues with this project its better to just stick with Facebooks RN Flatlist and just optimize your cell items. I've done weeks of research on the best solution and that's it..... This native solution is great in concept but it has no where the same community around it like Facebook to actively fix bugs for production application..

I recommend using why did you render to see which parts of your applications are causing rerenders (for sure its a ton...) and use React.memo with areEqual to determine when to rerender the cell.. https://github.com/welldone-software/why-did-you-render

Let me know if you still need the example, i can spin one up for you.

afrinsulthana1 commented 4 years ago

@GSManganneau Despite my fixes I've had so many issues with this project its better to just stick with Facebooks RN Flatlist and just optimize your cell items. I've done weeks of research on the best solution and that's it..... This native solution is great in concept but it has no where the same community around it like Facebook to actively fix bugs for production application..

I recommend using why did you render to see which parts of your applications are causing rerenders (for sure its a ton...) and use React.memo with areEqual to determine when to rerender the cell.. https://github.com/welldone-software/why-did-you-render

Let me know if you still need the example, i can spin one up for you.

Can you provide an example for the above approach that you have recommended using React.memo? Thanks in advance.

nikolaigeorgie commented 4 years ago

@afrinsulthana1 docs = https://reactjs.org/docs/react-api.html#reactmemo

const areEqual = (prevProps, nextProps) => {
if (prevProps.text !== nextProps.text) {
return false
} else {
return true 
}
}

const HomeScreen = (props: Props) => {
return (
<View>
<Text>{props.text}</Text>
</View>
)
}

export default React.memo(HomeScreen, areEqual)