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

fix(iOS): restore old header animation to prevent content jump #2563

Merged

Conversation

kkafar
Copy link
Member

@kkafar kkafar commented Dec 10, 2024

Description

Fixes #2550

Empirical research shows that UIKit uses interruptibleAnimatorForTransition: method on UIViewAnimatedTransitioning (our RNSStackAnimator)
for navigation item animation. This allowed us customizing navigation item timing curve & animation style, however it has caused unexpected content jumps (see #2550 or video below 👇).

Important

This PR reverts changes to navigation item animation introduced with v4. I guess it could be considered breaking unless it weren't a bit broken => I'm treating this as a necessary fix
& will try to bring back "new behaviour" soon, once we figure why interruptibleAnimatorForTransition: causes such bugs.

content-jump-with-new-animations.mov

After the changes this looks as follows:

current-state.mov

Changes

  • Add reproduction
  • Do not override interruptibleAnimatorForTransition: preventing content jump

Test code and steps to reproduce

TestAnimation

Checklist

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

jump

**important**: however at the same time we restore v3 header animation!
@kkafar kkafar self-assigned this Dec 10, 2024
@kkafar kkafar merged commit 5571050 into main Dec 10, 2024
5 checks passed
@kkafar kkafar deleted the @kkafar/fix-header-animation-revert-timing-curve-for-header branch December 10, 2024 09:32
kkafar added a commit that referenced this pull request Dec 16, 2024
## 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: <!-- For adding new props to native-stack
-->
- [ ]
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant