-
-
Notifications
You must be signed in to change notification settings - Fork 530
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
fix: native transition for native-like animations #2413
fix: native transition for native-like animations #2413
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I haven't tested it, but just got some questions 😄
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).
mViewManager.setTransitionDuration(view, value == null ? 350 : ((Double) value).intValue()); | ||
mViewManager.setTransitionDuration(view, value == null ? 500 : ((Double) value).intValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why we want to have 500ms
there? Is it taken from your own observations, or is it supported by iOS documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tboba I would say both 😅 I measured duration of animation in QuickTime
player and it was showing ~480ms (which is very close to 500ms and 20ms could be due to incorrect cut out of a video).
And second observation is that 7 << 16
animation takes 500ms to complete 🙃
// same value is used in other projects using similar approach for transistions | ||
// and it looks the most similar to the value used by Apple | ||
static constexpr float RNSShadowViewMaxAlpha = 0.1; | ||
static const int UIViewAnimationOptionCurveDefaultTransition = 7 << 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a comment why is it 7 << 16
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first time when I found it was in https://stackoverflow.com/questions/18870447/how-to-use-the-default-ios7-uianimation-curve
Should I add this as a comment above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking & playing around with this PR yesterday and came to conclusion that 7<< 16
value does not exist as valid animation options. The option
parameter takes values of type UIViewAnimationOptions
(mask combined from the options) & if you take a look on how it is defined:
typedef NS_OPTIONS(NSUInteger, UIViewAnimationOptions) {
UIViewAnimationOptionLayoutSubviews = 1 << 0,
UIViewAnimationOptionAllowUserInteraction = 1 << 1, // turn on user interaction while animating
UIViewAnimationOptionBeginFromCurrentState = 1 << 2, // start all views from current value, not initial value
UIViewAnimationOptionRepeat = 1 << 3, // repeat animation indefinitely
UIViewAnimationOptionAutoreverse = 1 << 4, // if repeat, run animation back and forth
UIViewAnimationOptionOverrideInheritedDuration = 1 << 5, // ignore nested duration
UIViewAnimationOptionOverrideInheritedCurve = 1 << 6, // ignore nested curve
UIViewAnimationOptionAllowAnimatedContent = 1 << 7, // animate contents (applies to transitions only)
UIViewAnimationOptionShowHideTransitionViews = 1 << 8, // flip to/from hidden state instead of adding/removing
UIViewAnimationOptionOverrideInheritedOptions = 1 << 9, // do not inherit any options or animation type
UIViewAnimationOptionCurveEaseInOut = 0 << 16, // default
UIViewAnimationOptionCurveEaseIn = 1 << 16,
UIViewAnimationOptionCurveEaseOut = 2 << 16,
UIViewAnimationOptionCurveLinear = 3 << 16,
UIViewAnimationOptionTransitionNone = 0 << 20, // default
UIViewAnimationOptionTransitionFlipFromLeft = 1 << 20,
UIViewAnimationOptionTransitionFlipFromRight = 2 << 20,
UIViewAnimationOptionTransitionCurlUp = 3 << 20,
UIViewAnimationOptionTransitionCurlDown = 4 << 20,
UIViewAnimationOptionTransitionCrossDissolve = 5 << 20,
UIViewAnimationOptionTransitionFlipFromTop = 6 << 20,
UIViewAnimationOptionTransitionFlipFromBottom = 7 << 20,
UIViewAnimationOptionPreferredFramesPerSecondDefault = 0 << 24,
UIViewAnimationOptionPreferredFramesPerSecond60 = 3 << 24,
UIViewAnimationOptionPreferredFramesPerSecond30 = 7 << 24,
} API_AVAILABLE(ios(4.0)) API_UNAVAILABLE(watchos);
using 7 << 16
rather does not make any sense (or am I getting this wrong)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second thing is that we would rather construct our mask explicitly from available value in lieu of using a magic constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I have few threads here.
First of all - thank you noticing this and taking your time to prepare the PR.
The code changes look solid, besides few remarks raised by @tboba and now by me.
My only objection is that I do not know, how "native slide animation" evolved between various iOS versions & I'm afraid that fixing it e.g. for iOS 18 will make matters worse for other iOS versions.
Overall I like the changes & will want to proceed in this direction, however I need to do some research on how the animation has evolved between various iOS versions.
static const float RNSFadeOpenTransitionDurationProportion = 0.2 / 0.5; | ||
static const float RNSSlideCloseTransitionDurationProportion = 0.25 / 0.5; | ||
static const float RNSFadeCloseTransitionDurationProportion = 0.15 / 0.5; | ||
static const float RNSFadeCloseDelayTransitionDurationProportion = 0.1 / 0.5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where these values come from? Do you have some kind of reference here, or are these found empirically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about 0.5? I captured a transition and measured the length of the video 🙈
I think there is no any notes about that in official documentation, but it's 500ms and many iOS developers aware about that 🙃
@@ -22,7 +23,7 @@ - (instancetype)initWithOperation:(UINavigationControllerOperation)operation | |||
{ | |||
if (self = [super init]) { | |||
_operation = operation; | |||
_transitionDuration = 0.35; // default duration in seconds | |||
_transitionDuration = 0.5; // default duration in seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also does it not differ from one iOS version to another? Value of 0.35s is around for longer that I'm in this project 😅 but I'm almost sure that when it was introduced it was meant to resemble native animation as close as possible, hence my impression that this differs between various iOS versions. It would be nice to have some kind of reference, because changing it here to e.g. 0.5s might look "more natively" on iOS 18 but "less natively" on iOS 16 - this is my main objection right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kkafar the curve 7 << 16 was introduced in iOS 7 (when Spring animations were added) and I believe since then that transition wasn't changed. I tested this PR on iOS 15 as well and it looks/feels like a native transition there as well (I was comparing with Settings app).
Thank you for a comment @tboba 🙌 Actually I was trying to introduce as less as possible breaking changes there, and animation with 500ms and However what I noticed now is that it looks like my changes introduce breaking changes, because looks like I think it's because So we have kind of this:
Maybe it's worth to ask @WoLewicki (I think he was a person who introduced the concept of I believe if we merge the PR in its current state, then we will introduce many breaking changes (especially for people who relied on Let me know what do you think 🙌 |
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). |
@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 animation: 'simple_push',
interpolator: Interpolators.Default, // or Interpolators.EaseInEaseOut. Alternatively can be plain strings 'default' | 'ease-in-ease-out', etc. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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'll be playing more with this PR right now, because it seems inadequate that specifying animation curve disables the animationDuration
. Animation curve & duration are supposed to work tightly together and this should be doable, even if we need to do a little math. I'll get back to you once I have more deets.
// same value is used in other projects using similar approach for transistions | ||
// and it looks the most similar to the value used by Apple | ||
static constexpr float RNSShadowViewMaxAlpha = 0.1; | ||
static const int UIViewAnimationOptionCurveDefaultTransition = 7 << 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking & playing around with this PR yesterday and came to conclusion that 7<< 16
value does not exist as valid animation options. The option
parameter takes values of type UIViewAnimationOptions
(mask combined from the options) & if you take a look on how it is defined:
typedef NS_OPTIONS(NSUInteger, UIViewAnimationOptions) {
UIViewAnimationOptionLayoutSubviews = 1 << 0,
UIViewAnimationOptionAllowUserInteraction = 1 << 1, // turn on user interaction while animating
UIViewAnimationOptionBeginFromCurrentState = 1 << 2, // start all views from current value, not initial value
UIViewAnimationOptionRepeat = 1 << 3, // repeat animation indefinitely
UIViewAnimationOptionAutoreverse = 1 << 4, // if repeat, run animation back and forth
UIViewAnimationOptionOverrideInheritedDuration = 1 << 5, // ignore nested duration
UIViewAnimationOptionOverrideInheritedCurve = 1 << 6, // ignore nested curve
UIViewAnimationOptionAllowAnimatedContent = 1 << 7, // animate contents (applies to transitions only)
UIViewAnimationOptionShowHideTransitionViews = 1 << 8, // flip to/from hidden state instead of adding/removing
UIViewAnimationOptionOverrideInheritedOptions = 1 << 9, // do not inherit any options or animation type
UIViewAnimationOptionCurveEaseInOut = 0 << 16, // default
UIViewAnimationOptionCurveEaseIn = 1 << 16,
UIViewAnimationOptionCurveEaseOut = 2 << 16,
UIViewAnimationOptionCurveLinear = 3 << 16,
UIViewAnimationOptionTransitionNone = 0 << 20, // default
UIViewAnimationOptionTransitionFlipFromLeft = 1 << 20,
UIViewAnimationOptionTransitionFlipFromRight = 2 << 20,
UIViewAnimationOptionTransitionCurlUp = 3 << 20,
UIViewAnimationOptionTransitionCurlDown = 4 << 20,
UIViewAnimationOptionTransitionCrossDissolve = 5 << 20,
UIViewAnimationOptionTransitionFlipFromTop = 6 << 20,
UIViewAnimationOptionTransitionFlipFromBottom = 7 << 20,
UIViewAnimationOptionPreferredFramesPerSecondDefault = 0 << 24,
UIViewAnimationOptionPreferredFramesPerSecond60 = 3 << 24,
UIViewAnimationOptionPreferredFramesPerSecond30 = 7 << 24,
} API_AVAILABLE(ios(4.0)) API_UNAVAILABLE(watchos);
using 7 << 16
rather does not make any sense (or am I getting this wrong)?
// same value is used in other projects using similar approach for transistions | ||
// and it looks the most similar to the value used by Apple | ||
static constexpr float RNSShadowViewMaxAlpha = 0.1; | ||
static const int UIViewAnimationOptionCurveDefaultTransition = 7 << 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second thing is that we would rather construct our mask explicitly from available value in lieu of using a magic constant.
Yes, this value is not available in public UIKit types.
I agree with you, but it looks like
Not sure I understand how we can derive the value from existing options 🙈 Would you mind to give a code snippet or a hint? 👀 |
Okay, my conclusions after playing around a bit are as follows:
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. |
@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 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! |
I want to update you, that using the API of |
I figured this out ☝🏻 ! |
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! |
// Damping of 1.0 seems close enough. | ||
// id<UITimingCurveProvider> timingCurveProvider = [[UISpringTimingParameters alloc] init]; | ||
|
||
id<UITimingCurveProvider> timingCurveProvider = [[UISpringTimingParameters alloc] initWithDampingRatio:1.0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think such config is definitely better, but animation begins very slowly (unlike native iOS animation). Compare it yourself (I took 3rd frame for a reference):
I think next config gives a better results:
id<UITimingCurveProvider> timingCurveProvider = [[UISpringTimingParameters alloc] initWithMass:3 stiffness:1000 damping:500 initialVelocity:CGVectorMake(0, 0)];
Let me see if such config supports custom duration 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, even though a new config is matching a native animation more closely - it doesn't allow to customize duration in this case 😔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely it does not, but let me know.
The spring curve for default animation is just [[UISpringTimingParameters alloc] init]
(according to "iOS Programming Guide" by Matt Neuburg, there is no need to guess the params or use some magic constants 😅 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #2477, as making this work requires more changes that I've thought initially. I'm in process of writing PR description but have some meetings currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kkafar but if we are using damping: 1
, then we have following transition:
Variant | 1 frame | 2 frame | 3 frame | 4 frame | 5 frame |
---|---|---|---|---|---|
damping: 1 | |||||
Native iOS |
For me it looks like a native iOS animation (default
) is smoother than simple_push
(with damping: 1
it feels like it's moving slightly faster than I expect to).
## Description > [!note] > A big chunk of discussion for these changes is under initial PR by @kirillzyusko, please see: #2413 Associated PR in `react-navigation`: * react-navigation/react-navigation#12233 Recently in #2413 @kirillzyusko noticed that our iOS animations (in particular `simple_push` and `slide_from_left`) do not resemble native slide-in animation as much as we wish and as we claim in our type definitions / guide for library authors. The approach suggested by @kirillzyusko in #2413 is as follows: We add new prop (draft name) `animationInterpolation`; when specified it allows to set `Interpolation.DEFAULT` which would use default `UISpringTimingParameters` used by default by UIKit. This solution has advantage of enabling easy extension in form of exposing more timing animation curves. At the same time it comes with disadvantage: setting default system params (spring animation) disables ability to specify animation duration, effectively disabling our `transitionDuration` prop (exposed as `animationDuration` by react-navigation). I don't want that ☝🏻 I want too keep `animationDuration` working as is, therefore we need to approximate default spring timing curve as closely as possible using damping ratio (initializer with damping ratio allows for control over final transition duration). According to Matt Neuburg's "Programming iOS 14" the params for default spring are as follows: - mass = 3, - stiffness = 1000, - damping = 500 We can compute damping ratio as: damping / (2 * sqrt(stiffness * mass)) => giving us approximately 4,56 (overdamping) <- and this is what we'll use now. > [!important] > Important side-effect of the refactor is the fact that `animationDuration` now impacts duration of the completion animation **after the gesture has been cancelled** during interactive dismissal. I've decided on keeping this behaviour, but it has both pros and cons. Any feedback on this would be welcome. See video below (animation duration set to 2000ms). https://github.com/user-attachments/assets/a13b2e5d-7b90-4597-a33a-956f2f393cd9 ## Changes The default animation time change applies to all animations. Maybe we should consider applying it only to animations for which we use new spring'y timing curves. The animation curve change applies to `simple_push`, `slide_from_left`, `slide_from_right`. The rest of animations kept EaseInOut curve. ## Test code and steps to reproduce I've played around on test `Test1072`. ## Before / After |Animation|Before|After| |----------|------------|-------| |`simple_push`|<video width="454" alt="image" src="">|<video width="452" alt="image" src="https://github.com/user-attachments/assets/4fb45c2f-d77b-4737-b5ee-8b406b90c15f">| |`fade`|<video width="454" alt="image" src="">|<video width="454" alt="image" src="https://github.com/user-attachments/assets/59114dd5-bc45-4933-ab02-869b35e1725c">| |`slide_from_bottom`|<video width="454" alt="image" src="">|<video width="454" alt="image" src="https://github.com/user-attachments/assets/4580fe9f-112d-4ead-8377-68c1caaf6d46">| ## Improvement possibilities > [!note] > 1. fade_from_bottom works ugly - it looks like the screen underneath disappears immediately - we should look into it > 2. add possibility of describing custom transition curves (new API idea), or at least expose some presets > 3. add prop to control "completion transition duraction" ## Checklist - [x] Included code example that can be used to test this change - [ ] Ensured that CI passes
Closing in favour of #2477 |
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:
Below is a comparison table comparing a
default
(native) animation withsimple_push
:default-transition-native-stack.mov
simple-push-transition.mov
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:
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
transitionDuration
to 500ms from 350ms;UIViewAnimationOptionCurveDefaultTransition
;UIViewAnimationOptionCurveDefaultTransition
insimple_push
,slide_from_left
,slide_from_bottom
,fade
animations;Screenshots / GIFs
Before
simple-push-transition.mov
After
simple-push-like-native.mov
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