software-mansion/react-native-screens

detachInactiveScreens={true} cause onScroll of previous tab screen triggered in react navigation v6 bottom tab

lclrobert2020 opened this issue · 10 comments

lclrobert2020 commented

Description

#10063 of react navigation
This bug only occurs in IOS, not for android
For example,
when you have a <Tab.Navigator> with two <Tab.Screen>
Let say you are currently on a Screen, say Screen 1, you have a with a onScroll props and a callback inside Screen 1,
When you press the Tab Bar Icon and jump to other Tab Screen, say Screen 2,
When the Screen 2 is shown and rendered,
The onScroll pf Screen 1 is magically called without any reason,
The same applies to onMomentumScrollEnd
if you set detachInactiveScreens={false} in the Tab Navigator
this problem will not occur

But for android,
this problem doesn't exist at all, no matter if detachInactiveScreens is set to true or false

Screenshots

Steps To Reproduce

1.go to my expo repo
2. press the tab bar to change screen
3. you will see the previous screen onScroll is called when the tab screen is changed

Expected behavior

Both IOS and Android should not call previous tab screen onScroll
This cause some issue as some of my tab screen has onScroll listener on it

Actual behavior

In IOS, you will see the previous screen onScroll is called when the tab screen is changed

Reproduction

https://snack.expo.dev/@robertli93/bug

Platform

  • iOS
  • Android
  • Web
  • Windows
  • tvOS

Workflow

exist both expo or bare react native project

  • [ X] Managed workflow
  • [ X] Bare workflow

Package versions

{
"dependencies": {
"react-native-screens": "~3.4.0",
"react-native-tab-view": "^3.0.0",
"@react-navigation/stack": "6.0.11",
"react-native-pager-view": "5.0.12",
"react-native-reanimated": "~2.2.0",
"@react-navigation/native": "6.0.2",
"@react-navigation/elements": "1.0.4",
"react-native-gesture-handler": "~1.10.2",
"@react-navigation/bottom-tabs": "6.0.9",
"react-native-safe-area-context": "3.2.0"
}
}

lclrobert2020 commented

update
the onScroll of Tab Screen is also triggered when , for example, some button in the Tab Screen will navigate to Other Stack Screen, only for IOS

WoLewicki commented

This behavior was introduced as a fix for unresponsive items of the scrollView when navigating away while it is still scrolling, you can see more info here: #594. I am not sure if there is an easy way to make it work correctly without this code. Maybe you could stop subscribing to the scroll events on blur of the event and resubscribe on focus events (You can see more here: https://reactnavigation.org/docs/navigation-events/)? Theoretically we could make the code from #594 switchable, but it reintroduces the previous problem in certain scenarios.

lclrobert2020 commented

This behavior was introduced as a fix for unresponsive items of the scrollView when navigating away while it is still scrolling, you can see more info here: #594. I am not sure if there is an easy way to make it work correctly without this code. Maybe you could stop subscribing to the scroll events on blur of the event and resubscribe on focus events (You can see more here: https://reactnavigation.org/docs/navigation-events/)? Theoretically we could make the code from #594 switchable, but it reintroduces the previous problem in certain scenarios.

This behavior was introduced as a fix for unresponsive items of the scrollView when navigating away while it is still scrolling, you can see more info here: #594. I am not sure if there is an easy way to make it work correctly without this code. Maybe you could stop subscribing to the scroll events on blur of the event and resubscribe on focus events (You can see more here: https://reactnavigation.org/docs/navigation-events/)? Theoretically we could make the code from #594 switchable, but it reintroduces the previous problem in certain scenarios.

Thanks for your reply,

Actually my case is more complex then trigger scrollview on previous screen, let me explain
I have make a masonry grid layout by flat list using a tricky method, i nested 2 flat list inside a main flat list or virtual list (I know its a trick, but sadly native rn doesn't support these layout )

I have used memo or pure component to get the best performance, and add a infinite scroll function on it, the grid works good and even with infinite loading 1000 items it doesn't trigger "VirtualizedList: You have a large list that is slow to update "

But, the grid will trigger this warning, only on one situation, when switching screen on tabs

you can see my https://snack.expo.dev/@robertli93/bugdemo
and video
https://www.youtube.com/watch?v=zMOupjf8IKY&ab_channel=RBL

in this repo, the 2 flatlist inside only renders a small amount of 20 items

and if you scroll the masonry grid down or up, not warning is triggered

But when you do the following, the error will be triggered, quite consistently

  1. Say, you are initially on the page with grid,( in the repo, its the setting page)
  2. you switch to other page by pressing the tab icon, (in the repo, switch to home page)
  3. you switch back to the page with grid by pressing the tab icon ( in the repo, its the setting page)
  4. you switch to other page by pressing the tab icon, (in the repo, switch to home page)
  5. you switch back to the page with grid by pressing the tab icon ( in the repo, its the setting page)
  6. you switch to other page by pressing the tab icon, (in the repo, switch to home page) <== error triggered very consist in this point

this happen in expo or bare rn apps
if 6 can't trigger error, repeat it a fews time to do so

basically when u switch back and forth 3 times, 3 errors throws
and there are 3 VirtualizedList (Flatlist is also a VirtualizedList)
after this error being throw, they will never throw again no matter how many times you switch again

My initial guess is that this must be related to scrolling triggered when screen changed, and when i delete the code related to the fix mentioned by you, it doesn't throw error anymore

But this fix seems important for interaction, is there any way to add a little fix so that I can use this fix and get rid of error at the same time, can we first stop the scroll, then change the page with a little time delay to fix it?

really appreciate you reply and your team's work,

WoLewicki commented

The easiest solution is something similar to what I proposed in the previous comment. You can use isFocused to detect if you already left the screen and not do anything when the event comes if it returns false. Is this enough?

lclrobert2020 commented

The easiest solution is something similar to what I proposed in the previous comment. You can use isFocused to detect if you already left the screen and not do anything when the event comes if it returns false. Is this enough?

The problem is even if you don't add any scroll listeners to any of the Flatlist, the error will be throw when coming back and forth between tabs 3 times

you can visit my expo https://snack.expo.dev/@robertli93/bugdemo and video
https://www.youtube.com/watch?v=zMOupjf8IKY&ab_channel=RBL

All flat list in the example has no scroll related listeners
but still the bug is trigger very consistently

I guess this is caused by the native side invoking scrollViewDidEndDecelerating on the parent and nested Child Flatlist together?

But I didn't why this bug is triggered when going back to and forth 3 times.

Practically, this warning is shown why the screen is changed to other screen, so I guess it will not be noticed by user

But I am not sure if this will cause any un noticed bugs and performance issues for these nested flat list structures

WoLewicki commented

It is warning from here: https://github.com/facebook/react-native/blob/afe0c1daea0aaf7b40b7ded08d6eb6d45b17099c/Libraries/Lists/VirtualizedList.js#L1614. It is probably triggered because onScroll event is sent from native side because of the our fix. You can add logs in these lines and check the exact values, but I think it is caused by these dt values being weird due to this flow.

lclrobert2020 commented

It is warning from here: https://github.com/facebook/react-native/blob/afe0c1daea0aaf7b40b7ded08d6eb6d45b17099c/Libraries/Lists/VirtualizedList.js#L1614. It is probably triggered because onScroll event is sent from native side because of the our fix. You can add logs in these lines and check the exact values, but I think it is caused by these dt values being weird due to this flow.

Thanks for ur reply,
I can confirm this is a dt problem or internal calculation problem
When I look into it deeper,
This problem occurs even when only using 1 FlatList
I have make a new demo
see my latest demo @ https://snack.expo.dev/@robertli93/flatlist-bug-when-changing-tabs-even-using-single-flatlist
see this latest demo video https://www.youtube.com/watch?v=vcGAuRvQ1LA&ab_channel=RBL
This is only 1 FlatList in this demo
Thanks to @WoLewicki
I have found the reason,

the warning is trigger by 4 calculations, on each internal _onScroll Event

  1. dt > 500 ( dt calculated by, const dt = this._scrollMetrics.timestamp ? Math.max(1, timestamp - this._scrollMetrics.timestamp): 1;)
  2. this. _scrollMetrics.dt > 500 (which seems to be the previous dt of last scroll)
  3. contentLength > 5 * visibleLength ( true if the list is long, even if you use PureComponent or React.memo)
  4. !this._hasWarned.perf (when a warning is shown , this._hasWarned.perf = true, in order to prevent showing this warning again, that explain why it only shown once for each virtualizedList)

Let me explain step by step

  1. You are on the page with flatlist (The page is just initialized, you didn't scroll the list)

  2. You change to another tab, the native scrollViewDidEndDecelerating is triggered, and the FlatList _onScroll get the event, and start calculating these value , assuming the timestamp of this scrollEvent is TimeStamp1

  • dt = 1, the this._scrollMetrics store the value of previous scroll value, because you didn't scroll the list at all previously, so this._scrollMetrics.timestamp is 0 (the init value), and 1 is returned (dt > 500 = false)
  • this._scrollMetrics.dt is default value = 10 (this. _scrollMetrics.dt > 500 = false)
  • contentLength > 5 * visibleLength = true (we an data array of length 100 in case)

  1. this._scrollMetrics get updated with these value
    this._scrollMetrics = { contentLength, dt, dOffset, offset, timestamp, velocity, visibleLength};
    Where dt is the dt = 1, timestamp is the TimeStamp1

  2. When you switch the tab back to the flatlist page and then switch to other page again, the native scrollViewDidEndDecelerating is triggered, and the FlatList _onScroll get the event, and start calculating these value again, assuming the timestamp of this scrollEvent is TimeStamp2

  • dt = TimeStamp2 - this._scrollMetrics.timestamp = TimeStamp1, which is easily > 500 because switching page cause time and its usually > 500 ms, so now dt = true
  • this._scrollMetrics.dt = 1 (step 2 value) which is not > 500 (this. _scrollMetrics.dt > 500 = false)
  • contentLength > 5 * visibleLength = true (we an data array of length 100 in case)

5.this._scrollMetrics get updated with these value
this._scrollMetrics = { contentLength, dt, dOffset, offset, timestamp, velocity, visibleLength};
Where dt is the dt =TimeStamp2 - TimeStamp1 (>500), timestamp is the TimeStamp2

  1. This is the tricky part. When you switch the tab back to the flatlist page and then switch to other page again, the native scrollViewDidEndDecelerating is triggered, assuming the timestamp of this scrollEvent is TimeStamp3
  • dt = TimeStamp3 - this._scrollMetrics.timestamp = TimeStamp2, which is easily > 500 because switching page cause time and its usually > 500 ms, so now dt = true
  • this._scrollMetrics.dt = TimeStamp2 - TimeStamp1 > 500, this. _scrollMetrics.dt > 500 = true
  • contentLength > 5 * visibleLength = true (we an data array of length 100 in case)

so this time, the warning is triggered, even if there is no any performance issue at all, its just some calculation problem, as Facebook React Time assume people will not trigger the native scrollViewDidEndDecelerating alone

That's it, this warning will show even on an optimized single flatlist

I think we should add explanation so that user will not get confused with this warning when they are switch tabs with flatlist? It is because this error will be triggered even with a single flatlist

WoLewicki commented

Thanks for investigating it deeply @lclrobert2020 ! Do you mean some exact place where we should add this explanation? I think we could add it to Common problems section: https://github.com/software-mansion/react-native-screens#common-problems. Also, is there anything more that can be done about it? Or should we close it after that?

lclrobert2020 commented

Thanks for investigating it deeply @lclrobert2020 ! Do you mean some exact place where we should add this explanation? I think we could add it to Common problems section: https://github.com/software-mansion/react-native-screens#common-problems. Also, is there anything more that can be done about it? Or should we close it after that?

I also think that Common problems is a good place to add it to
I think we can't do anything about this as the calculation is inside the flatlist, and we should not rewrite or touch the code of flatlist too. On the other hand, it may require some work to find a way to bypass this. I think we should close this issue, leave it alone and move to other improvements first. It is fine if it doesn't cause any performance related issue. So, I think this issue can be closed now?

WoLewicki commented

If you have some time and will, could you make such PR, so we could link this issue to the PR and close it in such way?