[NativeAnimated] Don't restore default values when components unmount

[janicduplessis]

Summary

There are some cases where restoring default values on component unmount is not desirable. For example in react-native-screens we want to keep the native view displayed after react has unmounted them. Restoring default values causes an issue there because it will change props controlled my native animated back to their default value instead of keeping whatever value they had been animated to.

Restoring default values is only needed for updates anyway, where removing a prop controlled by native animated need to be reset to its default value since react no longer tracks its value.

This splits restoring default values and disconnecting from views in 2 separate native methods, this way we can restore default values only on component update and not on unmount. This takes care of being backwards compatible for OTA JS updates.

Changelog

[General] [Fixed] - NativeAnimated - Don't restore default values when components unmount

Test Plan

• Tested in an app using react-native-screens to make sure native views that are kept after their underlying component has been unmount don't change. Also tested in RNTester animated example.

• Tested that new JS works with old native code

[janicduplessis]

Note: This also requires updating the generated spec to work, I did it manually to test but I'm not sure if codegen is runnable in OSS so I didn't include this change.

[JoshuaGross]

If this is running against an older runtime, does this check actually work?

[janicduplessis]

I did test running just the JS without the native changes and it worked fine. On older runtime the function doesn't exist and the disconnectFromView method handles both disconnection and restoring default values.

[JoshuaGross]

If appropriate could you attach before/after videos?

[janicduplessis]

You can see the bug at the end of this video, the navbar flicker white during the screen back transition:

https://imgur.com/cPHdnxT

How react-native-screen works for screen animation is when a screen component gets unmounted the native view does not get removed from the native view hierarchy and gets animated. In my app I have a navbar with an opacity value controlled by Animated initially at 0. When the screen gets unmounted the animated native driver would restore the default value for opacity (so 1 in this case) and case the header to show during the transition.

Restoring default values is not needed on component unmount since the view will usually get removed and if it doesn't we want it to keep its current state.

Here's a video after the fix:

https://imgur.com/a/bAZ9O54

[facebook-github-bot]

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

[janicduplessis]

@JoshuaGross Rebased and added a JS test

[facebook-github-bot]

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

[JoshuaGross]

@janicduplessis one of the tests is failing to pass locally:

stderr: /tmp/react-native-github/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java:984: error: method restoreDefaultValues in class com.facebook.react.animated.NativeAnimatedNodesManager cannot be applied to given types;
    mNativeAnimatedNodesManager.restoreDefaultValues(propsNodeTag, viewTag);
                               ^
  required: int
  found: int,int
  reason: actual and formal argument lists differ in length

[janicduplessis]

@JoshuaGross Fixed, I forgot to update android tests after changing the signature of restoreDefaultValues (The 2nd parameter was unused).

[janicduplessis]

@JoshuaGross Also do you know what the strategy is to land changes to native modules signature with codegen for OSS PRs? I did not include generated file changes in my PR since AFAIK running codegen in OSS is broken. I did test by editing the codegened files manually.

[facebook-github-bot]

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

[JoshuaGross]

@janicduplessis Not really sure - longterm we have to fix codegen in OSS. I can't land your diff without running codegen manually, so for now our land-blocking tests will prevent us from merging in anything that breaks trunk.

[react-native-bot]

This pull request was successfully merged by @janicduplessis in 686ab49.

^{When will my fix make it into a release? | Upcoming Releases}