PedroBern / react-native-collapsible-tab-view

A cross-platform Collapsible Tab View component for React Native
MIT License
815 stars 161 forks source link

fix: improve snap behavior by using scrollTo directly #326

Closed frw closed 6 months ago

frw commented 1 year ago

When using Reanimated v3, the scrolling becomes choppy when snapThreshold is specified and the header is snapping to the nearest point.

This PR fixes this by using scrollTo directly with animated set to true instead of through useAnimatedReaction.

Before After
frw commented 1 year ago

Hey @andreialecu, any chance you could take a quick look at this? The choppy snap animation is pretty visible and using scrollTo directly does fix it.

andreialecu commented 1 year ago

I believe there was a reason for doing it this way, mainly because you cannot interrupt an animated scroll and we need to in certain cases. See: https://github.com/software-mansion/react-native-reanimated/issues/1699

Reanimated 3.1.0 might fix this. In your video it doesn't look too bad.

frw commented 1 year ago

Hey @andreialecu,

Thanks for the response.

I updated the Reanimated version for the example app to v3.1.0 (https://github.com/frw/react-native-collapsible-tab-view/tree/frederick/example-reanimated-v3), and it seems like the choppiness for the snap behavior got even worse, and perhaps might even be unusable. Here are the results (changed the header height to 500 to better show the effects):

Before After

I also cannot seem to replicate the flickering issue on Reanimated v3.1.0 by interrupting the scroll, could you take another look and see if it's still necessary?

andreialecu commented 1 year ago

Interesting. Will take a look in the next few days.

andreialecu commented 1 year ago

I also cannot seem to replicate the flickering issue on Reanimated v3.1.0 by interrupting the scroll, could you take another look and see if it's still necessary?

I just took a look at this. I can replicate the issue shown at https://github.com/software-mansion/react-native-reanimated/issues/1699 on iOS still.

It doesn't seem to occur on Android, though. So I would propose having a platform check for Android and using the code in this PR for it.

frw commented 1 year ago

@andreialecu

I tried it on iOS, and it seems like I discovered another issue when using it with Reanimated v3.1.0 Changing the header height in example/src/Shared/Header.tsx from 250 to 500, then scrolling to the middle of the header and then allowing it to snap would result in a fatal error:

https://user-images.githubusercontent.com/1888212/235420102-34f91c66-5656-4197-b9eb-8beefb376f59.mp4

It seems like it happens only when there are many updates required for scrolling (thus the change of the header height to 500 is needed to reproduce this).

I can confirm the changes in the PR I made does fix this. Since this is the case, I think it's better to use scrollTo directly in both iOS and Android since it'll otherwise crash in iOS and will have choppy behavior on Android, versus a slight glitching when you interrupt the scroll during snap. Thoughts?

andreialecu commented 1 year ago

Interesting. Could you investigate why the stack overflow happens?

If it's a reanimated issue it would make sense to open it upstream.

Could also be something we need to refactor elsewhere.

frw commented 1 year ago

Still debugging this, but posting my initial findings. This is the stack trace I got from the crash:

ERROR  ReanimatedError: Maximum call stack size exceeded., js engine: reanimated
  <unknown> ([native code])
  <global> (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/NativeMethods.ts)
  anonymous ([native code])
  <global> (/react-native-collapsible-tab-view/src/helpers.tsx)
  scrollToImpl ([native code])
  useScroller (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  useAnimatedReaction$argument_1 (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  fun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/hook/useAnimatedReaction.ts)
  anonymous ([native code])
  mapperRun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/mappers.ts)
  global.__flushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  forEach ([native code])
  currentCallbacks.forEach$argument_0 (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  handleAndFlushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/core.ts)
  handleAndFlushAnimationFrame ([native code])
  callGuardDEV (worklet_1892348396866)
  <unknown> ([native code])
  <global> (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/NativeMethods.ts)
  anonymous ([native code])
  <global> (/react-native-collapsible-tab-view/src/helpers.tsx)
  scrollToImpl ([native code])
  useScroller (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  useAnimatedReaction$argument_1 (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  fun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/hook/useAnimatedReaction.ts)
  anonymous ([native code])
  mapperRun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/mappers.ts)
  global.__flushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  forEach ([native code])
  currentCallbacks.forEach$argument_0 (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  handleAndFlushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/core.ts)
  handleAndFlushAnimationFrame ([native code])
  callGuardDEV (worklet_1892348396866)
  <unknown> ([native code])
  <global> (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/NativeMethods.ts)
  anonymous ([native code])
  <global> (/react-native-collapsible-tab-view/src/helpers.tsx)
  scrollToImpl ([native code])
  useScroller (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  useAnimatedReaction$argument_1 (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  fun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/hook/useAnimatedReaction.ts)
  anonymous ([native code])
  mapperRun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/mappers.ts)
  global.__flushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  forEach ([native code])
  currentCallbacks.forEach$argument_0 (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  handleAndFlushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/core.ts)
  handleAndFlushAnimationFrame ([native code])
  callGuardDEV (worklet_1892348396866)
  <unknown> ([native code])
  <global> (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/NativeMethods.ts)
  anonymous ([native code])
  <global> (/react-native-collapsible-tab-view/src/helpers.tsx)
  scrollToImpl ([native code])
  useScroller (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  useAnimatedReaction$argument_1 (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  fun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/hook/useAnimatedReaction.ts)
  anonymous ([native code])
  mapperRun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/mappers.ts)
  global.__flushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  forEach ([native code])
  currentCallbacks.forEach$argument_0 (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  handleAndFlushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/core.ts)
  handleAndFlushAnimationFrame ([native code])
  callGuardDEV (worklet_1892348396866)
  <unknown> ([native code])
  <global> (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/NativeMethods.ts)
  anonymous ([native code])
  <global> (/react-native-collapsible-tab-view/src/helpers.tsx)
  scrollToImpl ([native code])
  useScroller (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  useAnimatedReaction$argument_1 (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  fun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/hook/useAnimatedReaction.ts)
  anonymous ([native code])
  mapperRun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/mappers.ts)
  global.__flushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  forEach ([native code])
  currentCallbacks.forEach$argument_0 (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/initializers.ts)
  handleAndFlushAnimationFrame (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/core.ts)
  handleAndFlushAnimationFrame ([native code])
  callGuardDEV (worklet_1892348396866)
  <unknown> ([native code])
  <global> (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/NativeMethods.ts)
  anonymous ([native code])
  <global> (/react-native-collapsible-tab-view/src/helpers.tsx)
  scrollToImpl ([native code])
  useScroller (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  useAnimatedReaction$argument_1 (/react-native-collapsible-tab-view/src/hooks.tsx)
  anonymous ([native code])
  fun (/react-native-collapsible-tab-view/example/node_modules/react-native-reanimated/src/reanimated2/hook/useAnimatedReaction.ts)

From what I can tell, it seems like useAnimatedReaction (https://github.com/frw/react-native-collapsible-tab-view/blob/ea2acf3407674cb5ce301ab34d20347a422396ea/src/hooks.tsx#L300-L307) is calling the scrollToImpl (https://github.com/frw/react-native-collapsible-tab-view/blob/ea2acf3407674cb5ce301ab34d20347a422396ea/src/hooks.tsx#L241) multiple times, and it seems like reanimated is perhaps not too happy when calling scrollTo too many times in a short span.

Will continue to debug and see if there's a workaround, but can't really tell if it's an issue upstream on the Reanimated side. Any ideas you might have on implementing a fix would be appreciated!

andreialecu commented 1 year ago

@andreialecu

I tried it on iOS, and it seems like I discovered another issue when using it with Reanimated v3.1.0 Changing the header height in example/src/Shared/Header.tsx from 250 to 500, then scrolling to the middle of the header and then allowing it to snap would result in a fatal error:

gnzi1779_i8HflIPV.mp4 It seems like it happens only when there are many updates required for scrolling (thus the change of the header height to 500 is needed to reproduce this).

I can confirm the changes in the PR I made does fix this. Since this is the case, I think it's better to use scrollTo directly in both iOS and Android since it'll otherwise crash in iOS and will have choppy behavior on Android, versus a slight glitching when you interrupt the scroll during snap. Thoughts?

I just tried to reproduce this but I can't with a header height of 500.

Initially I tried with reanimated 2, but v3.1.0 seems to be just fine too.

https://user-images.githubusercontent.com/697707/235449014-af0d0d2a-20a4-4cc7-9523-050c58b0a439.mp4

frw commented 1 year ago

Ah I was testing it on a physical device (iPhone 11 16.3.1, Reanimated v3.1.0). It also doesn't always reproduce. Had to try it about 3-5 times before the fatal error appears.

I just tried on the Simulator and it seems like it is indeed not reproducible there.

andreialecu commented 6 months ago

I will close for now, I believe recent reanimated versions might have resolved this.

Feel free to ask to reopen otherwise.