react-native-screens icon indicating copy to clipboard operation
react-native-screens copied to clipboard

fix: native transition for native-like animations

Open kirillzyusko opened this issue 1 year ago • 3 comments

Description

According to description:

    • "simple_push" – performs a default animation, but without shadow and native header transition (iOS only)
    • "slide_from_left" - slide in the new screen from right to left (Android only, resolves to default transition on iOS)

Both statements say that these transitions should act as a default one, but:

  • default duration for these transitions is 350ms (by default, but it's customizable)
  • these transitions looks similar to native transitions, but don't exactly match each other

Below is a comparison table comparing a default (native) animation with simple_push:

Default Simple push

As you can see they are quite different. In this PR I'd like to propose to make them matching native transition (even though it will be managed by UIView animateWithDuration code).

For that I:

  • changed default duration from 350ms to 500ms
  • changed animation curve to match native transition.

In current PR shape it's a breaking change, because 500ms will be spreaded to fade_from_bottom animations, which are currently based on Android animations (and they rely on custom duration, like 250ms, if I understand the code correctly).

Anyway, I'll be glad to hear your feedback on whether you want to have something like this in the code in upcoming v4 🙌

Changes

  • change transitionDuration to 500ms from 350ms;
  • added UIViewAnimationOptionCurveDefaultTransition;
  • use UIViewAnimationOptionCurveDefaultTransition in simple_push, slide_from_left, slide_from_bottom, fade animations;

Screenshots / GIFs

Before

https://github.com/user-attachments/assets/45b4a2fd-75f2-4d33-8e2c-4dba75fc1de3

After

https://github.com/user-attachments/assets/782a625b-514f-4865-ae5a-e8555f17e858

Test code and steps to reproduce

You can use example app to test these changes. And you can take iOS Settings app as a reference for transitions.

Checklist

  • [x] Included code example that can be used to test this change
  • [x] Updated TS types
  • [x] Updated documentation:
    • [x] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md
    • [x] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md
    • [x] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx
    • [x] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx
  • [x] Ensured that CI passes

kirillzyusko avatar Oct 16 '24 16:10 kirillzyusko

Also, I believe this PR introduces breaking changes, as we're changing the default value of animation? So it's ideal to push it for 4.x version of screens and mark it with ! (I think).

Thank you for a comment @tboba 🙌 Actually I was trying to introduce as less as possible breaking changes there, and animation with 500ms and 7 << 16 curve looks and feels like the animation with 350ms duration. 150ms just spending in the end for 20-40 pixels and it's just for smooth stopping of the animation.

However what I noticed now is that it looks like my changes introduce breaking changes, because looks like animationDuration is not taken into consideration and you can not change the duration of the animation 😲

I think it's because 7 << 16 curve is a spring animation and in spring animation you can not control the duration of the animation because duration calculated only based on a physics rules 😔

So we have kind of this:

  • 7 << 16 is used natively on iOS to animate screen transitions;
  • 7 << 16 doesn't allow to change duration since it's a spring-animation, so this PR introduce breaking changes 🙈

Maybe it's worth to ask @WoLewicki (I think he was a person who introduced the concept of simple_push animation) why he decided to use default curves and 350ms animation? And what is the best way to handle changes from this PR?

I believe if we merge the PR in its current state, then we will introduce many breaking changes (especially for people who relied on animationDuration/transitionDuration property). Maybe we can create another param called curved/interpolator and pass it from JS?

Let me know what do you think 🙌

kirillzyusko avatar Oct 21 '24 09:10 kirillzyusko

As for 350ms, I don't remember it exactly too, I think it was there already (yep: https://github.com/software-mansion/react-native-screens/blob/2.0/ios/RNSScreenStack.m#L336).

WoLewicki avatar Oct 21 '24 12:10 WoLewicki

@kkafar you reacted with emoji that scares me 😅 Do you have any ideas in your head how this PR can be adopted to introduce as less as possible breaking changes and at the same time to give a native transitions feeling if they are needed?

I think additional curved/interpolation properties would give that flexibility without introducing breaking changes, so the API could look like:

animation: 'simple_push',
interpolator: Interpolators.Default, // or Interpolators.EaseInEaseOut. Alternatively can be plain strings 'default' | 'ease-in-ease-out', etc.

kirillzyusko avatar Oct 25 '24 16:10 kirillzyusko

@kirillzyusko I admit I am not decided what is the best option here yet. I'll give it more consideration this afternoon and let you know how we want to proceed.

kkafar avatar Oct 28 '24 08:10 kkafar

First I have few remarks on 7 << 16 value, which seem to be incorrect (see below). Or maybe there are some hidden values not represented in public UIKit types?

Yes, this value is not available in public UIKit types.

Second thing is that I don't think we would want to completely disable opportunity to specify animationDuration prop. This is fine for default value, because it is supposed to look like the native one, however for the library-defined set we would want to keep the duration configurable.

I agree with you, but it looks like 7 << 16 uses a different animation (CASpringAnimation vs CABasicAnimation) and in CASpringAnimation you can not specify duration. That's why I suggested to add interpolator property, so that people can use additional property if they want to have a closer animation to a native one (but they will loose an opportunity to customize duration - for me it's a fair alternative).

Second thing is that we would rather construct our mask explicitly from available value in lieu of using a magic constant.

Not sure I understand how we can derive the value from existing options 🙈 Would you mind to give a code snippet or a hint? 👀

kirillzyusko avatar Oct 29 '24 09:10 kirillzyusko

Okay, my conclusions after playing around a bit are as follows:

  1. I think I haven't understood you correctly initially. For some reason I've thought that you want to just disable animation duration and rely only on transition curves, but the idea of yours is that animation duration stops working only if you opt-in for specifying transition curve. I guess we could got with that, however I think that with a little more effort we could achieve the desired effect without any tradeoff necessity 👇🏻
  2. We want the default animation to resemble native-default-animation as closely as possible;
  3. UIView animateXXX API does not allow for usage of custom timing curves thus to achieve desired effect we most likely need to refactor whole RNSStackAnimator to make use of Core Animation directly (animate the layers) and then just mimic default UIKit animation closely.

I think I have some throughput now.

I have to attend to sheetFooter a bit & various scrollview interactions in formSheets, however I might be able to do that this week.

kkafar avatar Oct 29 '24 13:10 kkafar

@kirillzyusko I've updated implementation of simple_push & I would like to get your opinion whether you think this will be good enough now. Let me know, please. If we can adjust parameters to get satisfying result we'll go with the approach I've proposed in 39b599f

kkafar avatar Oct 29 '24 23:10 kkafar

I've updated implementation of simple_push & I would like to get your opinion whether you think this will be good enough now. Let me know, please. If we can adjust parameters to get satisfying result we'll go with the approach I've proposed in 39b599f

@kkafar Cool, I'm going to check that implementation tomorrow or the day after tomorrow and will let you know! The approach looks promising, however I'm still concerned that we can control duration and use spring animation simultaneously... Will see! Thank you for the code!

kirillzyusko avatar Oct 30 '24 17:10 kirillzyusko

I want to update you, that using the API of UIViewPropertyAnimator ruined the full screen swipe gesture, for some reason. I'm doing some more research now in order to fix it, however it might not work in current state of the PR, heads up.

kkafar avatar Oct 31 '24 08:10 kkafar

I figured this out ☝🏻 !

kkafar avatar Oct 31 '24 13:10 kkafar

Thanks for your comments @kkafar

I'm going to revisit this PR tomorrow and check if we can force Spring animations to follow duration constraint! I'll get back to you tomorrow!

kirillzyusko avatar Nov 04 '24 15:11 kirillzyusko

Closing in favour of https://github.com/software-mansion/react-native-screens/pull/2477

kirillzyusko avatar Nov 07 '24 09:11 kirillzyusko