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

Fix scroll position for dynamic contents #127

Open
sm2017 opened this issue Feb 20, 2018 · 42 comments
Open

Fix scroll position for dynamic contents #127

sm2017 opened this issue Feb 20, 2018 · 42 comments

Comments

@sm2017
Copy link

sm2017 commented Feb 20, 2018

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
Copy link

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
Copy link
Collaborator

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
Copy link
Author

sm2017 commented Feb 20, 2018

@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
Copy link

@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
Copy link
Author

sm2017 commented Feb 20, 2018

@abdallamohamed your approach don't works as I expected

@naqvitalha I will send a working expo demo asap

@AbdallaElabd
Copy link

@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
Copy link
Author

sm2017 commented Feb 20, 2018

@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
Copy link

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

@naqvitalha
Copy link
Collaborator

naqvitalha commented Feb 20, 2018

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
Copy link
Author

sm2017 commented Feb 20, 2018

@naqvitalha I cannot understand why renderAheadOffset is very important here

@naqvitalha
Copy link
Collaborator

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
Copy link
Author

sm2017 commented Feb 21, 2018

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
Copy link
Author

sm2017 commented Feb 22, 2018

@naqvitalha is there any tricks for forceNonDeterministicRendering=true

@sm2017
Copy link
Author

sm2017 commented Feb 24, 2018

@naqvitalha Help please

@sm2017
Copy link
Author

sm2017 commented Feb 25, 2018

@naqvitalha 😭 help needed!

@naqvitalha
Copy link
Collaborator

@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
Copy link
Author

sm2017 commented Feb 26, 2018

@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
Copy link
Contributor

@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
Copy link
Collaborator

@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
Copy link
Collaborator

@tafelito
Copy link
Contributor

@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
Copy link
Author

sm2017 commented Feb 27, 2018

@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
Copy link
Collaborator

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
Copy link
Author

sm2017 commented Feb 27, 2018

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

@AbdallaElabd
Copy link

@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
Copy link
Author

sm2017 commented Mar 3, 2018

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
Copy link
Author

sm2017 commented Mar 13, 2018

@naqvitalha Reply please

@naqvitalha
Copy link
Collaborator

@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
Copy link
Author

sm2017 commented Mar 14, 2018

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

@tafelito
Copy link
Contributor

@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
Copy link

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

@naqvitalha
Copy link
Collaborator

@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
Copy link
Collaborator

@tafelito it looks like the same logic

@tafelito
Copy link
Contributor

@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
Copy link

Kimotn commented Aug 16, 2018

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
Copy link
Contributor

@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
Copy link

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
Copy link

@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
Copy link

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
Copy link

@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
Copy link

@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
Copy link

nikolaigeorgie commented May 31, 2020

@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)

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

No branches or pull requests

9 participants