Skip to content

Conversation

@ryangomba
Copy link
Contributor

This diff attempts to fix a number of Android native animation bugs related to incomplete node invalidation, e.g. #10657 (comment).

For full correctness, we should mark any node as needing update when it is:

  • created
  • updated (value nodes)
  • attached to new parents
  • detached from old parents
  • attached to a view (prop nodes)

cc/ @janicduplessis

@ryangomba ryangomba changed the title Proper invalidation on android Proper native animation invalidation on Android Nov 9, 2016
@facebook-github-bot
Copy link
Contributor

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

@ryangomba ryangomba changed the title Proper native animation invalidation on Android Proper NativeAnimated node invalidation on Android Nov 9, 2016
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 9, 2016
Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to change one of the calls to mUpdatedNodes from add to put in setAnimatedNodeOffset. The rests looks good to me, gonna test it in my app.

@janicduplessis
Copy link
Contributor

Tested it and it works well, it fixed an issue on android where sometimes there was a white screen after poping a route on android with NavigationExperimental + native animations.

@asgvard
Copy link

asgvard commented Nov 10, 2016

@janicduplessis Not sure if it has something to do with this particular PR, but is there any chance that the 8968 have the same root cause? Like not proper node invalidation after the node is removed from the array of siblings with the different zIndexes?
I'm struggling with the same problem with white screen when transitioning between routes, but using react-router v4 and react-motion for animation. When the fading-out component should became null at the end of animation, I see the white screen instead. However during animation itself zIndexes behave as expected, I can see the "new" page floating above and the "old" fading-out page stays on background. So it happens exactly when the old one becomes null (probably not fully detached).

@janicduplessis
Copy link
Contributor

This only affects native driven animations (when you start an animation with useNativeDriver=true) so unless you are using that it doesn't seems related.

@ryangomba ryangomba force-pushed the native-animation-invalidate-android branch from 4977cfd to cb19cc5 Compare November 11, 2016 00:28
@janicduplessis
Copy link
Contributor

@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 11, 2016
@ryangomba ryangomba deleted the native-animation-invalidate-android branch November 11, 2016 15:30
DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
This diff attempts to fix a number of Android native animation bugs related to incomplete node invalidation, e.g. facebook#10657 (comment).

For full correctness, we should mark any node as needing update when it is:

- created
- updated (value nodes)
- attached to new parents
- detached from old parents
- attached to a view (prop nodes)

cc/ janicduplessis
Closes facebook#10837

Differential Revision: D4166446

fbshipit-source-id: dbf6b9aa34439e286234627791bb7fef647c8396
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.

4 participants