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

Enabling fullscreenGestures or custom animations can cause content shifts when navigating from a screen without a header to one with a header. #2550

Closed
hirbod opened this issue Dec 5, 2024 · 15 comments · Fixed by #2563
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided

Comments

@hirbod
Copy link
Contributor

hirbod commented Dec 5, 2024

Description

I discovered a bug that occurs when navigating from a page without a header to a page with a header. The content shifts upward. This issue is reproducible with fullscreenGestureEnabled or any other custom animation.

The problem is likely caused by this PR:
#2477

The following Snack isolates this bug to react-native-screens. I created two reproductions using react-navigation v7 and expo-router. Additionally, @satya164 created one using react-native-screens directly:

https://snack.expo.dev/@hirbod/react-native-screens-full-screen-swipe-bug

I suspect the changes in the header animation are the cause.

ScreenRecording_12-05-2024.03-55-43_1.MP4

Very likely related

#2536
#2548

Maybe related

expo/expo#33008

Steps to reproduce

Navigate to the screen with a header, and touch / move it. Swipe back to the screen before and now navigate to that screen again.

Bug does not happen in v3

Snack or a link to a repository

https://snack.expo.dev/@hirbod/react-native-screens-full-screen-swipe-bug

Screens version

4.3.0

React Native version

0.76+

Platforms

iOS

JavaScript runtime

Hermes

Workflow

None

Architecture

None

Build type

None

Device

None

Device model

No response

Acknowledgements

Yes

@github-actions github-actions bot added Platform: iOS This issue is specific to iOS Missing info The user didn't precise the problem enough Repro provided A reproduction with a snack or repo is provided labels Dec 5, 2024
@github-actions github-actions bot removed the Missing info The user didn't precise the problem enough label Dec 5, 2024
@kkafar
Copy link
Member

kkafar commented Dec 5, 2024

Hey! Looks reproducible. Thanks for reporting!

@hirbod
Copy link
Contributor Author

hirbod commented Dec 5, 2024

@kkafar I wanna also add two more things, as all of these issues appeared with said #2477

  1. animation set to none does not work anymore. It fades instead when set to none. Works with transparentModals though
  2. animationMatchesGesture together with fade (and the forced fade lol) is very jittery

In my video, animation is set to none and it turns it into a fade. And when I add animationMatchesGesture, it goes wild

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-12-05.at.13.09.20.mp4

Even animationDuration of 0 is not working. The only way I can "somehow" achieve animation "none" right now is by setting animation to fade with an animationDuration of 10

@kkafar
Copy link
Member

kkafar commented Dec 10, 2024

Hey @hirbod , #2563 should fix this (unfortunately bringing back old header animation, however we'll try to improve this). I've released 4.4.0-rc.0 (available on NPM) - if you have capacity to test this please let me know whether it also works for you or I've still overlooked some case.

@hirbod
Copy link
Contributor Author

hirbod commented Dec 10, 2024

I'll check it in about 3 hours, once I am back at the desk. This only addresses the header, not the second message of this issue report regarding the broken animation, right?

@kkafar
Copy link
Member

kkafar commented Dec 10, 2024

animation none will be fixed here: #2565
as of the "jitter" - I haven't looked into it yet - but if I can reproduce it - there will be separate PR.

@hirbod
Copy link
Contributor Author

hirbod commented Dec 10, 2024

rc0 does fix the jump, but the header animation is broken compared to v3. For me, this doesn’t solve the problem; it’s still a regression and not shippable. The only acceptable solution is to fully revert #2477 and release once everything is stable. Right now, it’s just patching with band-aids.

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-12-10.at.14.20.21.mp4

@kkafar
Copy link
Member

kkafar commented Dec 11, 2024

@hirbod could you elaborate a bit more what more regressions do you notice in comparison to v3?

simplepush-transition-header-mix-before-changes.mov

This is a recording from v3 & it seems to have the same issue I notice on your video.

@hirbod
Copy link
Contributor Author

hirbod commented Dec 11, 2024

In my video I am navigating from no header to a screen with header and the crossfade is much slower and "behind" compared to v3. v3 is also not great though.

@hirbod
Copy link
Contributor Author

hirbod commented Dec 11, 2024

Compared to v3 it looks worse because it's a lot slower and much more visible of the timing I guess

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-12-11.at.11.42.08.mp4

@kmagiera
Copy link
Member

@hirbod is this last recording from v3?

@hirbod
Copy link
Contributor Author

hirbod commented Dec 11, 2024

@kmagiera v4, but I know the timing curve very well, and the v3 header didn’t crossfade so slowly, which makes the bug more obvious since v4. Either way, the jumping is definitely much worse than this weird-looking header animation, gladly this is gone since rc-0, I hope we can get a decent looking header animation soon

@matinzd
Copy link

matinzd commented Dec 11, 2024

I am bit confused now. Do we have a different issue now? :))

@hirbod
Copy link
Contributor Author

hirbod commented Dec 11, 2024

@matinzd well kind of :D
The jumping is gone, but we still have a different animation behavior and a not so good looking header animation in general, but the jumping should be fixed

@kkafar
Copy link
Member

kkafar commented Dec 17, 2024

Regarding the animation jitter you reported here: #2550 (comment) - it seems that it also has been fixed with #2565. The animationMatchesGesture + animation: none looks bad between screens with header and no header, but it seems that there is no difference between v3 & v4 behaviour and this needs to be fixed separately.

Otherwise we now have "fade" transition on gesture.

Screen.Recording.2024-12-17.at.04.56.33.mov

@hirbod
Copy link
Contributor Author

hirbod commented Dec 17, 2024

Yes, that looks good. Agree that the Header is a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: iOS This issue is specific to iOS Repro provided A reproduction with a snack or repo is provided
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants