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

[NavigationTransitioner] Fix animation in fast navigation between scenes #10761

Closed

Conversation

gitim
Copy link
Contributor

@gitim gitim commented Nov 5, 2016

There is a bug in navigation animation:
-05-2016 16-04-23
navigation animation from route-2 was awful, route-3 scene appeared without any animation, I pushed above example to gitim@46dd8c9.

This bug can be reproduced when user navigates to the next scene immediately after navigation to the current one or when navigating between scenes programmatically (e.g. between loading scene and whatever next scene). I made some investigation and It looks like that progress should be reseted before building the new _transitionProps.

The above example after applying this patch:
-05-2016 16-12-16

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @hedgerwang and @ericvicenti to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 5, 2016
@ericvicenti
Copy link
Contributor

Thanks for looking into this! Could you possibly post a gist of a sample app that demonstrates this issue? I'd like to look into this further because I'm slightly surprised that this fixes the problem.

@gitim
Copy link
Contributor Author

gitim commented Nov 5, 2016

Yes, sure. I created a separate app https://github.com/gitim/NavigationExperimentalAnimation, it is almost the same as an example from UIExplorer.

@gitim
Copy link
Contributor Author

gitim commented Nov 8, 2016

Hmm, it is strange, not adding progress to animations (https://github.com/facebook/react-native/blob/master/Libraries/NavigationExperimental/NavigationTransitioner.js#L147-153) also fixes an animation, I just tried to remove these lines and the animation plays fine. Can't figure out how they related, @ericvicenti any idea?

@ericvicenti
Copy link
Contributor

cc @janicduplessis, I think this might be a race condition with animated values.

Has this changed in recent versions of RN, or has the issue always been there?

@janicduplessis
Copy link
Contributor

Is it using native animations? There are some race conditions that can happen on iOS, you can test out #10663 if it's the case.

@gitim
Copy link
Contributor Author

gitim commented Nov 9, 2016

@ericvicenti I noticed this in 0.31 I think, when NavigationAnimatedView was removed, and I had to transition to NavigationTransitioner.

@gitim
Copy link
Contributor Author

gitim commented Nov 10, 2016

@janicduplessis @ericvicenti I checked this with patch from #10663, it doesn't fix the animation.

upd: @janicduplessis it is not using native animations

@ericvicenti
Copy link
Contributor

Ok, seems like this is a minor issue that has been here for a while. The fix makes sense

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Nov 14, 2016
@facebook-github-bot
Copy link
Contributor

Something went wrong when importing this pull request. Please cc someone from the team at fb to help with importing this.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Nov 15, 2016
@mkonicek
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Nov 23, 2016
@facebook-github-bot
Copy link
Contributor

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
There is a bug in navigation animation:
![-05-2016 16-04-23](https://cloud.githubusercontent.com/assets/3778452/20030228/8d93bc3e-a371-11e6-87d6-2c5c994733b5.gif)
navigation animation from route-2 was awful, route-3 scene appeared without any animation, I pushed above example to gitim@46dd8c9.

This bug can be reproduced when user navigates to the next scene immediately after navigation to the current one or when navigating between scenes programmatically (e.g. between loading scene and whatever next scene). I made some investigation and It looks like that `progress` should be reseted before building the new _transitionProps.

The above example after applying this patch:
![-05-2016 16-12-16](https://cloud.githubusercontent.com/assets/3778452/20030259/a800681e-a372-11e6-847a-991d355a5940.gif)
Closes facebook#10761

Differential Revision: D4226864

Pulled By: mkonicek

fbshipit-source-id: 31dceb6c8e497b2cbd891bbda4cb3add01cbcca0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants