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

Several issues with 2.0.0-alpha #153

Closed
ferrannp opened this issue Sep 10, 2019 · 15 comments
Closed

Several issues with 2.0.0-alpha #153

ferrannp opened this issue Sep 10, 2019 · 15 comments

Comments

@ferrannp
Copy link
Contributor

ferrannp commented Sep 10, 2019

The branch implementing this can be found https://github.com/ferrannp/fithero/pull/148.

Basically is replacing all the createStackNavigator for createNativeStackNavigator.

  1. On Android, when I have to native stacks, when I navigate to one screen and I go back, I get a red screen:

red-screen

  1. On Android the tabs are flickering:

tabs-flicker

  1. On Android the header is not respecting the content but tabs are ok:

5f9d837c-4bb4-478b-ad2f-5cae0a5cb0a2

  1. On iOS the header is respecting the content but not the tabs:

Simulator Screen Shot - iPhone X - 2019-09-10 at 15 50 20

  1. On mode: modal there is no header anymore (only on iOS, on Android there is header) and I cannot close the modal using gestures either:

ios-modal

android-modal

  1. On iOS the FAB from react-native-paper button is not there anymore. On Android is ok:

Native stack:

Simulator Screen Shot - iPhone X - 2019-09-10 at 15 52 54

Normal stack:

Simulator Screen Shot - iPhone X - 2019-09-10 at 15 55 14

  1. That can be ok but headerTitle does not accept a string anymore so we need to actually come up with a Text component that looks the same as the original.

  2. As you see on the screens, I cannot remove the elevation & shadow on toolbar anymore. I used to do it with this code:

headerStyle: {
  elevation: 0,
  backgroundColor: theme.colors.toolbar,
  borderBottomColor: theme.colors.toolbar,
},

Not working anymore though.

@ghondar
Copy link

ghondar commented Sep 17, 2019

@ferrannp
Copy link
Contributor Author

ferrannp commented Oct 19, 2019

Hey @kmagiera, here the second round of testing after #184 as we discussed. New PR https://github.com/ferrannp/fithero/pull/148.

iOS

  1. I cannot swipe back to the previous screen (gesture)

  2. The right buttons on the Toolbar have more padding on the right side that they should. On Android is ok:

Screenshot 2019-10-19 at 14 35 11

Android

  1. I am still getting the removeView crash if the screen uses a ViewPager. I am using https://github.com/madhu314/react-native-tabbed-view-pager-android which uses React Native Android ViewPager. Also the History tab is empty, it should not be empty. See:

tabs

  1. Tabs are still flickering. It would be nice just to be able to remove the animation.

flicker

Both platforms

  1. If you do headerTitle: () => 'Title' it crashes. It expects you to wrap it on a Text component. But this used to work and user is not force to provide its own styled Text on Android and iOS.

Screenshot 2019-10-19 at 14 57 58

  1. I am still not able to remove the borderBottom on iOS and the elevation on Android for the Toolbar:

Screenshot 2019-10-19 at 14 35 11

Screenshot 2019-10-19 at 15 02 45

The code is:

  headerStyle: {
    elevation: 0,
    backgroundColor: theme.colors.toolbar,
    borderBottomColor: theme.colors.toolbar,
  },

@kmagiera
Copy link
Member

kmagiera commented Oct 22, 2019

Thanks for checking again!

As for hiding shadow since native headers are not "just RN views" they can no longer be styled this way. I exposed a native stack property called "hideShadow" such that it can now be used from the react-navigation binding: #192

As we discussed on github flickering on android is due to the fact shadows are visible on the navigation bar. If you use the property I mentioned to hide shadow you should no longer observe it. The root cause of why you see it flicker when shadows are ON is in react-native-paper library and needs to be addressed there. In short react-native-paper does not update alpha compositing when animating between screens. If you turn shadows ON with non native stack you will see it flicker as well.

@kmagiera
Copy link
Member

As for the back swipe I noticed it was broken in some version but checked recent master and seem to be working there, so chances are one of Janic's PRs we merged in the meantime fixes this

@tobiaslins
Copy link

tobiaslins commented Oct 23, 2019

@kmagiera What about the headerbar on mode: 'modal' for iOS. It's missing for me aswell? Backswipe on modal is working for me!
Thanks again for your work! Do you have opencollective? 🤗

I just found out that

- (void)navigationController:(UINavigationController *)navigationController willShowViewController:(UIViewController *)viewController animated:(BOOL)animated

Doesn't get called for modal presentation. I'm not an iOS dev so not sure how to solve this :/

kmagiera added a commit to kmagiera/react-native-tabbed-view-pager-android that referenced this issue Oct 23, 2019
…mounting.

The default ViewManager implementation of removeAllViews iterates over all existing children and calls removeViewAt(index) method. When this happens the ViewPager implementation triggers a code path that adds new tab views (the path starts from calling notifyDatasetChanged in ReactViewPager.java which ends up triggering TabLayout view updates that clean up all the views and add individual tabs from scratch https://android.googlesource.com/platform/frameworks/support/+/5c7d7bb/design/src/android/support/design/widget/TabLayout.java#616).

Apparently when using libraries that provides Android's native transition (e.g. react-native-screens v2) it is undesirable for the views being unmounted to remove and then add back children. It is because these libraries rely on a mechanism called view transitions which prevents children from being removed so long the transition is ongoing. As a result the children removed in TabLayout implementation does not go through the complete removal cycle and they cannot be added back because they are not yet unmounted. This cased a crash documented here software-mansion/react-native-screens#153 (comment)
@ferrannp
Copy link
Contributor Author

ferrannp commented Oct 23, 2019

I'm giving it another round of testing with latest commit on master.

So far not seeing issues on iOS. There are issues on Android, and also listeners like didFocus. I need some time to test more Android and document it.

By the way @kmagiera another thing that does not work properly with react-navigation and this is the persistenceKey. It will persist but not all the controllers so you won't be able to go back :D. Any ideas on how we can fix this?

@kmagiera
Copy link
Member

Hey @tobiaslins thanks for your feedback. I haven't yet decided what to do with the header on modals. I'm leaning towards disabling it on Android too. In native iOS in order to have the navbar there you'd render another stack inside a modal. There is a way to only render plain navbar but then you loose a bunch of styling options that are available when using the whole navigator. I'll keep you updated which route I ended up choosing, if you have any suggestions I'd love to hear them.

As for the open collective I don't have one, what I'd be more happy about is if you decided to support RN community events I help organize, e.g., you can buy a ticket to App.js 2020 conference or just tell your frends or coworkers about it :)

@satya164
Copy link
Collaborator

Hey @kmagiera, maybe you can tell me a bit more about what you mean by "In short react-native-paper does not use update alpha compositing when animating between screens"?

@kmagiera
Copy link
Member

kmagiera commented Oct 28, 2019

Hey @satya164 – you can check this snack here https://snack.expo.io/@kzzzf/sadistic-chips

You'll have to run this on android. Then when you switch between tabs you will see the button flashing for a fraction of second. If you record the screen and see this in slow motion you will notice that it is due to the opacity animation not reflecting the colors properly. This effect is due to the performance optimization RN views use, it's been documented here https://www.facebook.com/notes/andy-street/react-native-android-offscreen-alpha-compositing/10153949352079590/ In short what happens here is in the middle of animation all subviews animate their opacity from 0 to 1 (or the other way around for the disappearing views). What you'd expect instead is for all the views to be rendered to an offscreen buffer and then the opacity should be applied onto that buffer.

The fix would be to set https://facebook.github.io/react-native/docs/view.html#needsoffscreenalphacompositing to true for the view that animates opacity but you only want to do that for the duration of animation. Otherwise rendering each screen would require additional buffering and hence use more memory. For example, react-native screens does that by default when transitioning between screens.

@kmagiera
Copy link
Member

out-031

@kmagiera
Copy link
Member

This is a mid-animation state above. As on both screens in between you animate have white top it should never be the case that the top of the screen is not white because you animate white into white. The same should be for the bottom of the screen and the bottom should also not change its color for the duration of animation.

@satya164
Copy link
Collaborator

satya164 commented Oct 28, 2019

Makes sense. Thanks for the explanation. I think we'll also need to do this for react-navigation's animations.

@wibb36
Copy link

wibb36 commented Mar 11, 2020

Hey @tobiaslins thanks for your feedback. I haven't yet decided what to do with the header on modals. I'm leaning towards disabling it on Android too. In native iOS in order to have the navbar there you'd render another stack inside a modal. There is a way to only render plain navbar but then you loose a bunch of styling options that are available when using the whole navigator. I'll keep you updated which route I ended up choosing, if you have any suggestions I'd love to hear them.

As for the open collective I don't have one, what I'd be more happy about is if you decided to support RN community events I help organize, e.g., you can buy a ticket to App.js 2020 conference or just tell your frends or coworkers about it :)

@kmagiera Have you thought what you wanted to do with the header on modal? It's still missing on RNS 2.3.0. I'd collaborate but I'm not an iOS developer. I'd appreciate your help with this.

@WoLewicki
Copy link
Member

It looks like this issue is concerning #260 only now, so can I close it @ferrannp ?

@ferrannp
Copy link
Contributor Author

ferrannp commented Apr 1, 2020

Hey @WoLewicki ! Yes let's close this as I need to do another round of tests but if I find another issue, I can open a new one. I think this has been long enough here :).

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

7 participants