feat(iOS)!: change default animation curve & duration #2477

kkafar · 2024-11-04T14:44:27Z

Description

Note

A big chunk of discussion for these changes is under initial PR by @kirillzyusko, please see: #2413

Associated PR in react-navigation:

docs(native-stack): update JSDoc on default animation duration 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).

Screen.Recording.2024-11-06.at.10.25.13.mov

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`		Screen.Recording.2024-11-06.at.16.47.14.mov
`fade`		Screen.Recording.2024-11-06.at.16.47.27.mov
`slide_from_bottom`		Screen.Recording.2024-11-06.at.16.47.43.mov

Improvement possibilities

Note

fade_from_bottom works ugly - it looks like the screen underneath disappears immediately - we should look into it
add possibility of describing custom transition curves (new API idea), or at least expose some presets
add prop to control "completion transition duraction"

Checklist

Included code example that can be used to test this change
Ensured that CI passes

…ion proportions

… [PRB]

…tyAnimator

hirbod · 2024-11-04T15:36:14Z

Interesting, will this change address the hard animation cancel when the fullScreenGesture swipe to pop was aborted?

kkafar · 2024-11-05T09:22:45Z

@hirbod, could you elaborate a bit? What do you mean exactly by "hard animation cancel"?

kirillzyusko · 2024-11-05T10:35:11Z

ios/RNSScreenStackAnimator.mm

+  // Damping of 1.0 seems close enough and we keep `animationDuration` functional.
+  // id<UITimingCurveProvider> timingCurveProvider = [[UISpringTimingParameters alloc] init];
+
+  return [[UISpringTimingParameters alloc] initWithDampingRatio:1.0];


I think this PR is a continuation of #2413

I left a comment about this config: https://github.com/software-mansion/react-native-screens/pull/2413/files#r1829111470 :)

I've answered there ;D

hirbod · 2024-11-05T11:19:49Z

Here is a video showing how it looks like with the custom fullScreenGestureEnabled cancel

trim.39631C56-93B4-474A-92A4-4480E996B2DE.MOV

If you compare this with the default one (which is smoothed), this looks pretty bad.

kkafar · 2024-11-05T11:58:21Z

Then yes - these changes will impact how the "completion animation after gesture release" looks. It'll use spring curve (with very high damping) instead of straight up linear curve.

Edit: at least for simple_push (and other slides) animation, I haven't decided what I want to do for fade yet.

hirbod · 2024-11-05T12:14:56Z

Yes, I think having this for simple_push is good enough. Damping for fade doesn't seem necessary

…ve-native

kkafar · 2024-11-06T09:22:32Z

@kirillzyusko

Thanks for this extensive research! I've done some quick math and got damping ratio of 4,56 (previously I've set damping ratio to 1.0). I think it should improve the situation and give even better feeling.

My only concerns now are that in #2477 I've changed default animation duration to 0.5 for all animations, while changing timing curves only for subset of animations, making other animations feel slow.

Before releasing I might bring back animation duration for these animations which do not use spring timing curve back to 350ms.

kirillzyusko · 2024-11-06T11:18:24Z

Awesome stuff 🔥 I'm going to check and review this PR today/tomorrow!

hirbod · 2024-11-06T11:36:20Z

The after video for all animations appears to break the header. The back button and header title transition looks terrible in the after video. :/

I hope you're not going to merge in this state. I rather take the non "native" animation curve instead of this horrible header transition.

MaxAst · 2024-11-06T11:55:52Z

I have to second @hirbod, the navigation header transition looks quite abrupt, imo worse than the current version

WoLewicki

Left some comments! Nice to see progress in that thing!

ios/RNSPercentDrivenInteractiveTransition.mm

ios/RNSScreenStack.mm

WoLewicki · 2024-11-06T11:55:37Z

ios/RNSScreenStackAnimator.h

@@ -2,7 +2,20 @@

 @interface RNSScreenStackAnimator : NSObject <UIViewControllerAnimatedTransitioning>

- (instancetype)initWithOperation:(UINavigationControllerOperation)operation;
+/// This property is filled only when there is an property aniamator associated with an ongoing interactive transition.
+/// In our case this is when we're handling full swipe or edge back gesture.


Also why 3 /?

Suggested change

/// In our case this is when we're handling full swipe or edge back gesture.

/// In our case this is when we're handling full screen swipe or edge back gesture.

This is a doc-style comment in objective-c (XCode?), same as /** */ Idk your keybindings, but when using vim plugin, in normal mode you can press shift + k to display the docs for given symbol. I'm sure w/o Vim there is some other key combination to trigger this.

ios/RNSScreenStackAnimator.mm

WoLewicki · 2024-11-06T12:05:36Z

ios/RNSPercentDrivenInteractiveTransition.mm

+  if (_animationController != nil) {
+    [self.animationController.inFlightAnimator setFractionComplete:percentComplete];
+  }
+  [super updateInteractiveTransition:percentComplete];


Not sure why header looks so bad in those transitions, I think this line should control the state of animation of header too. Are those values not changing pretty linearly ?

Header looks bad only in non-interactive transitions. TBH I'm surprised that it does not look bad in old implementation - the docs suggest that it should look bad

I do not know what code drives the header transition in non-interactive transitions (notice that I'm not swiping on these recordings). Need to spend some time on this.

The docs mention the native header transition where the back button title travels to the place of title when navigating back/forward. It works only with the default animations. For simple_push it was the fade animation in header iirc.

I guess you're right. I've just confirmed that it has not been a fluke - on main there is nice fade transition. Do you happen to remember where this is animated? Or is this handled entirely by UIKit?

Figured this out in: 7a2dfc9

kkafar · 2024-11-06T14:25:52Z

Yeah, I agree. I'll give my self an hour to figure this out, as I need to release 4.0.0. If I don't make it on time I'll release it with next minor behind some kind of feature flag.

kkafar · 2024-11-06T15:50:14Z

@WoLewicki @hirbod @kirillzyusko @MaxAst I've fixed the header animation. Take a look when you have a moment. I've updated the "after" videos.

I'll proceed with merging these changes, as I want to include these in 4.0.0 which will be released any minute from now. If we'll want to make some minor adjustments I'll release them as fixes in next minor.

**Motivation** In `react-native-screens` v4 we change default animation on iOS duration to 500ms. See: * software-mansion/react-native-screens#2477

kirillzyusko · 2024-11-07T10:01:03Z

Great job @kkafar 💪

I tested the example app and I can not spot any issues: header works fine, animation feels native, navigation after gesture also feels like a native, transition duration is still customizable 🔥 Awesome stuff! And congrats with 4.0 release!

kkafar · 2024-11-07T10:50:40Z

Glad to hear that!

[...] congrats with 4.0 release!

Thanks!

hirbod · 2024-11-07T11:39:54Z

Thanks for the updates—it’s definitely improved, but the header still isn't quite right and differs from the native stack crossfade. It feels delayed and uses a spring animation, but the header should animate linearly to avoid harsh crossfades. (Crossfades are always linear, and it becomes very noticeable when going back, as the back button hides abruptly.)

Also, the cancel animation is too long. It should maybe be 50% of the animation duration, or it should have its own prop. I also believe that the distance is important (a short swipe followed by cancel shouldn’t take 2000ms). Both the header and cancel need adjustments. In my opinion, this PR was rushed to get merged, considering it’s a significant change that affects many apps.

kirillzyusko · 2024-11-07T12:15:20Z

Also, the cancel animation is too long. It should maybe be 50% of the animation duration, or it should have its own prop. I also believe that the distance is important (a short swipe followed by cancel shouldn’t take 2000ms).

@hirbod I was comparing cancel animation side by side with iOS Settings app and in my opinion the native app uses the same transition as native-stack does now (but I used default 500ms transition). Of course, if you specify 2000ms it feels slow, but then plain navigation also works slow (and in this case it's kind of consistency?).

It feels delayed and uses a spring animation, but the header should animate linearly to avoid harsh crossfades. (Crossfades are always linear, and it becomes very noticeable when going back, as the back button hides abruptly.)

Probably you are right here - header animation indeed feels delayed, but i think it can be fixed in subsequent releases 👀

In my opinion, this PR was rushed to get merged, considering it’s a significant change that affects many apps.

I think the main reason to include this PR was because SWM team had to release RNS for react-navigation@v7. And this PR brings breaking changes, so it was very reasonable to include it in 4.0 (maybe it does not have an ideal state of things, but it can be always fixed later - main thing here is to highlight breaking changes into a single release).

hirbod · 2024-11-07T12:22:46Z

I tested the change, and indeed, the cancel animation works fine as long as the animationDuration hasn’t been modified. I’m not sure if canceling should take that long when animationDuration is set to a high value—it would be consistent, but I think having an option to override this would be beneficial.

The header is definitely off. After testing it more thoroughly myself, I really don’t like it; it stands out quite strongly for me. I understand the reasoning for shipping this with a major (and breaking) release, but I was hoping for more community testing and feedback before releasing it, as SDK 52 will ship with Expo Router v4, which will use v7 and RNS 4. This means the impact will be larger, as people are likely to upgrade without even knowing.

That said, it’s still a fresh release, and a subsequent update could quickly address the header issue. I really appreciate both your work @kirillzyusko and @kkafar

kkafar · 2024-11-08T14:56:52Z

[...] the header still isn't quite right and differs from the native stack crossfade. It feels delayed and uses a spring animation, but the header should animate linearly to avoid harsh crossfades. (Crossfades are always linear, and it becomes very noticeable when going back, as the back button hides abruptly.)

&

Probably you are right here - header animation indeed feels delayed, but i think it can be fixed in subsequent releases 👀

Can agree here, but I'm not sure what should be the direction. Before this merge it has looked as follows:

simple_push_before_refactor.mov

That is, in case of:

non-interactive transition we have indeed the crossfade
interactive transition the being-popped navbar item first fades out completely, and just then the entering one starts fading in (I guess that's "sequenced fade"). This results in few frames when the navigation bar is completely blank I don't like this personally, but I'm curious of your opinion.

We could:

restore old behaviour here (I see one technical problem, but might be able to workaround it, will turn out when I'll start coding),
make it always linearly crossfade (I think that's better option)

[...] I’m not sure if canceling should take that long when animationDuration is set to a high value—it would be consistent, but I think having an option to override this would be beneficial.

Yep, I've highlighted this effect in important section in PR description & suggested that it might be reasonable to allow to control this also in PR description. I will expose a flag to control whether animation duration should impact the "completion animation".

Thanks for your input

hirbod · 2024-11-08T15:03:40Z

I believe make it crossfade linearly could be enough, but would appreciate a recording.

kkafar · 2024-11-08T17:57:37Z

Linear	EaseInOut	DefaultSpring
simple_push_after_linear_header.mov	simple_push_after_easeinout_header.mov	simple_push_after_springy_header.mov

wdyt?

kirillzyusko · 2024-11-08T22:17:16Z

I like ease-in-ease-out - I think it was a default transition before?

Correct me if I'm wrong @kkafar 🙈

hirbod · 2024-11-08T22:41:47Z

While dragging, all look fine but I'd say Linear is best. But when you press the back button, it feels like it is kind of delays and "flashes", before it cross fades. Something is off, but it's not too bad. Almost there

kkafar · 2024-11-12T13:24:19Z

I've created #2492 improving the header animation (ease-in-out).

[...] it feels like it is kind of delays and "flashes", before it cross fades [...]

☝🏻 also explaining this and requesting opinions

## Description There is still some header animation noticeable for some reason. <-- This is because we use `fade` transition with duration 0 and do not override interruptible animator! To prevent the animation we could either return `nil` interruptible animator (but overriding the method comes with it's own set of problems, see #2563 and other related) or handle the `none` animation much earlier, when calling `showViewControllers:animated:` in `updateContainer` (pass `animated: NO`). PS: If we would want to pass `animated: NO` I wonder what would happen to dismiss prevention - we implemented it at the stage of the animation start... We need to think this through. Note: Must be implemented with old animation API, because `UIViewPropertyAnimator` does not allow for 0 duration (it uses default if the specified animation duration is below some undocumented treshold). This regression was introduced with #2477 ## Changes We now use old API for `animation: none` & still rely on fade animation to implement it. Note the points made above ☝🏻 - we should refactor this code to make advantage of `animated:` parameter of the `showViewControllers:animated:`. ## Test code and steps to reproduce `TestAnimation` - set stack presentation to `none` - it works as prior to v4. WIP VIDEO ## Checklist - [ ] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Updated documentation:  - [ ] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [ ] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes

kkafar added 11 commits October 30, 2024 14:30

Use 0.5 for default transition duration value __but__ keep old animat…

4e1c12d

…ion proportions

Write Push transition using property animator and spring timing curve

cc641c6

Linter

845ab1c

Write Pop transition using property animator and spring timing curve

d5d5809

Implement non-interactive simple push using UIViewPropertyAnimator [PRB]

24fc134

Implement interactive simple-push using UIViewPropertyAnimator

ebe507e

Make RNSScreenStackAnimator a source of truth on animation properties…

aeec238

… [PRB]

Update documentation: default transition lasts 500ms

12b747d

Update default transition duration in Android spec

9bde620

Implement both interactive and not slide_from_left using UIViewProper…

0e1f818

…tyAnimator

Move comment on default spring animation to helper function

81fe5ed

kirillzyusko reviewed Nov 5, 2024

View reviewed changes

kkafar mentioned this pull request Nov 5, 2024

fix: native transition for native-like animations #2413

Closed

8 tasks

stash changes for another animation

23300c6

kkafar added 3 commits November 6, 2024 09:36

Merge branch 'main' into @kkafar/native-native-animtions-for-ios-nati…

0e13917

…ve-native

Set damping ratio to 4.56

82d3629

Restore slide_from_bottom to old behaviour

bbad9e3

kkafar requested a review from kirillzyusko November 6, 2024 09:19

kkafar requested review from alduzy, WoLewicki and kirillzyusko and removed request for kirillzyusko November 6, 2024 09:22

kkafar mentioned this pull request Nov 6, 2024

docs(native-stack): update JSDoc on default animation duration react-navigation/react-navigation#12233

Merged

Fix typos

a772a33

WoLewicki reviewed Nov 6, 2024

View reviewed changes

kkafar added 2 commits November 6, 2024 15:39

Review fixes

7b0bdca

Add fix for missing header animation

7a2dfc9

Update comment for inFlightAnimator

a8e13c3

kkafar merged commit 375b7ee into main Nov 6, 2024
9 checks passed

kkafar deleted the @kkafar/native-native-animtions-for-ios-native-native branch November 6, 2024 16:08

kkafar mentioned this pull request Nov 12, 2024

fix(iOS): fix navigation item animation #2492

Open

8 tasks

kkafar mentioned this pull request Dec 11, 2024

fix(iOS): restore behaviour of RNSScreenStackAnimationNone #2565

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iOS)!: change default animation curve & duration #2477

feat(iOS)!: change default animation curve & duration #2477

kkafar commented Nov 4, 2024 •

edited

Loading

hirbod commented Nov 4, 2024

kkafar commented Nov 5, 2024

kirillzyusko Nov 5, 2024

kkafar Nov 5, 2024

hirbod commented Nov 5, 2024

kkafar commented Nov 5, 2024 •

edited

Loading

hirbod commented Nov 5, 2024

kkafar commented Nov 6, 2024

kirillzyusko commented Nov 6, 2024

hirbod commented Nov 6, 2024 •

edited

Loading

MaxAst commented Nov 6, 2024

WoLewicki left a comment

WoLewicki Nov 6, 2024

kkafar Nov 6, 2024

WoLewicki Nov 6, 2024

kkafar Nov 6, 2024

kkafar Nov 6, 2024

WoLewicki Nov 6, 2024

kkafar Nov 6, 2024

kkafar Nov 6, 2024

kkafar commented Nov 6, 2024

kkafar commented Nov 6, 2024

kirillzyusko commented Nov 7, 2024

kkafar commented Nov 7, 2024

hirbod commented Nov 7, 2024 •

edited

Loading

kirillzyusko commented Nov 7, 2024

hirbod commented Nov 7, 2024

kkafar commented Nov 8, 2024

hirbod commented Nov 8, 2024

kkafar commented Nov 8, 2024

kirillzyusko commented Nov 8, 2024

hirbod commented Nov 8, 2024

kkafar commented Nov 12, 2024

	/// In our case this is when we're handling full swipe or edge back gesture.
	/// In our case this is when we're handling full screen swipe or edge back gesture.

feat(iOS)!: change default animation curve & duration #2477

feat(iOS)!: change default animation curve & duration #2477

Conversation

kkafar commented Nov 4, 2024 • edited Loading

Description

Changes

Test code and steps to reproduce

Before / After

Improvement possibilities

Checklist

hirbod commented Nov 4, 2024

kkafar commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hirbod commented Nov 5, 2024

kkafar commented Nov 5, 2024 • edited Loading

hirbod commented Nov 5, 2024

kkafar commented Nov 6, 2024

kirillzyusko commented Nov 6, 2024

hirbod commented Nov 6, 2024 • edited Loading

MaxAst commented Nov 6, 2024

WoLewicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkafar commented Nov 6, 2024

kkafar commented Nov 6, 2024

kirillzyusko commented Nov 7, 2024

kkafar commented Nov 7, 2024

hirbod commented Nov 7, 2024 • edited Loading

kirillzyusko commented Nov 7, 2024

hirbod commented Nov 7, 2024

kkafar commented Nov 8, 2024

hirbod commented Nov 8, 2024

kkafar commented Nov 8, 2024

kirillzyusko commented Nov 8, 2024

hirbod commented Nov 8, 2024

kkafar commented Nov 12, 2024

kkafar commented Nov 4, 2024 •

edited

Loading

kkafar commented Nov 5, 2024 •

edited

Loading

hirbod commented Nov 6, 2024 •

edited

Loading

hirbod commented Nov 7, 2024 •

edited

Loading