expo / ex-navigation

Route-centric navigation for React Native
997 stars 201 forks source link

Android - SlidingTabNavigation broken after navigating back within the root navigator (TabNavigation or DrawerNavigation) #415

Open rgommezz opened 7 years ago

rgommezz commented 7 years ago

I am using RN 0.41.2 and I started experiencing the issue after migrating ex-navigation from 2.5.0 to 2.9.1. The issue I am talking about is the same as the one described in the link below (that also provides a gif of the bug), with the subtle difference I am using a DrawerNavigation instead of TabNavigation: https://github.com/react-native-community/react-native-tab-view/issues/98

I pinpointed the issue to the update of react-native-tab-view to v0.41 where it switches to native pagers by default. Ex-navigation is indeed leveraging TabViewPagerAndroid that behinds the scenes uses ViewPagerAndroid from react-native. I tried using the JS based implementation TabViewPagerPan from 'react-native-tab-view' with the latest version from ex-navigation and the issue no longer appears.

So, I wanted to go a bit deeper to see what exactly was going on. I downloaded react-native-tab-view to tweak it and play a bit, so that I could understand its internals, but I couldn't not locate the bug in that tier.

Basically, every time we are changing a tab through a tab press (instead of swiping), the library calls setPage imperatively on the native ViewPager through a ref and it's been called properly in both cases (tabs working fine and tabs broken), but somehow, even though it was delegating work to the native side through the bridge, the pager does not swipe.

Then I dived down to native code, set some breakpoints and tried to find out some light. I gotta say I am not an expert in Android (I did some code before but now I am bit rusty), but I was able to see something interesting.

The mounting process of the native ViewPager.java from Android involves calling onAttachedToWindow, and then onLayout to determine the view layout and its edges (width, height, padding, scrollPos etc...). Once onLayout finishes it sets a member variable mFirstLayout to false, being that the flag that enables page scrolling.

From the UI perspective that would be the initial mount of The Drawer and the initialRoute that contains a SlidingTabNavigation. After that, any attempt to change the tab works fine, either by horizontal swiping or pressing the tabs. Doing so by pressing tabs, it boils down in the native code to call scrollToItem(item, ...) method that is only called if mFirstLayout is true

Now let's get to the point of reproducing the issue => I select a different route on my drawer and come back to my initial route where my sliding tabs are. It turns out that inside the native ViewPager.java, onDetachedFromWindow is called, which is making the scroll to be aborted and immediately after, onAttachedToWindow is called again (initializing mFirstLayout to true). This detaching-reattaching process does not follow with a call to onLayout, and that's the key thing, because the layout was not calculated internally, hence the scroll is disabled and scrollToITem is not called anymore.

Thoughts on top of that:

It's a pity this relevant bug, since I am pretty happy with the library results and I am about to release an app to the market that uses this library. I know it is in maintenance mode due to the newer (and recommended) https://github.com/react-community/react-navigation but it's still in beta and pretty raw. Of course, I can always keep using 2.5.0 for the time being, but the performance with native ViewPager is way better (specially with long ListViews rendered per tab). Also, the JS version doesn't set the content background color (everything under the tabs) immediately (mine is dark grey) and there is a fraction of time where it shows a white background that is a bit annoying. Those small details are preventing me from going all-in with the app release.

Hopefully after reading my research, somebody with the proper knowledge can see where the problem may lay in. And I am always happy to help in any way I can!

satya164 commented 7 years ago

Thanks a lot for looking into this issue. I'm not familiar enough with ex-navigation codebase to be able to say why it happens, but it sounds pretty strange. Most likely a bug in ViewPager.

Regarding the pager, the JS based pager needs to wait for layout before rendering anything since it needs to know the container width, that's why you see a frame delay in rendering. react-native-tab-view accepts an initialLayout prop to avoid this.

You can also try TabViewPagerScroll on Android. It works with some caveats. If you have initial page set to something else than the first tab, it'll have to scroll to that page after render because Android's ScrollView doesn't have contentOffset. Also I found the animation was way too fast, though you might be okay with it.

TabViewPagerScroll also needs to wait for rendering, unlike TabViewPagerAndroid, but there are some workarounds to prevent the delay, basically we render only the first screen before layout which grows to fit the screen with flexbox. When layout is available, we render all the pages and set the width instead of using flexbox. Similar technique might be possible for JS based pager too, but I didn't manage to get it work, and didn't have enough time to hack on it either.

Tam commented 7 years ago

+1 I'm getting the same issue using just ViewPagerAndroid.

rgommezz commented 7 years ago

Hi @Tam, I migrated my navigation library to react-navigation and the issue was gone. Since this library is in maintenance mode, I encourage you to migrate, the API is quite similar and the docs are very handy. I was able to perform the migration in one evening.

Tam commented 7 years ago

@rauliyohmc I'll take a look, thanks!

sibelius commented 7 years ago

react-navigation uses TabViewPagerPan instead of ViewPagerAndroid

that's the currently fix

allmaxgit commented 7 years ago

@sibelius no, it not fixed and in react-navigation https://github.com/react-community/react-navigation/issues/1442