From 686ab49107df8ed20d4e810f1366715cd70b4a31 Mon Sep 17 00:00:00 2001 From: Janic Duplessis Date: Mon, 4 Nov 2019 15:37:45 -0800 Subject: [PATCH] Don't restore default values when components unmount (#26978) 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 JS running with the older native code. ## Changelog [General] [Fixed] - NativeAnimated - Don't restore default values when components unmount Pull Request resolved: https://github.com/facebook/react-native/pull/26978 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 Reviewed By: mmmulani Differential Revision: D18197735 Pulled By: JoshuaGross fbshipit-source-id: 20fa0f31a3edf1bc57ccb03df9d1486aba83edc4 --- .../Animated/src/NativeAnimatedHelper.js | 7 ++++++ .../Animated/src/NativeAnimatedModule.js | 1 + .../src/__tests__/AnimatedNative-test.js | 22 +++++++++++++++++++ .../Animated/src/createAnimatedComponent.js | 5 ++++- Libraries/Animated/src/nodes/AnimatedProps.js | 10 +++++++++ .../FBReactNativeSpec-generated.mm | 7 ++++++ .../FBReactNativeSpec/FBReactNativeSpec.h | 1 + .../RCTNativeAnimatedModule.mm | 10 ++++++--- .../specs/NativeAnimatedModuleSpec.java | 3 +++ .../react/animated/NativeAnimatedModule.java | 18 +++++++++------ .../animated/NativeAnimatedNodesManager.java | 2 +- .../NativeAnimatedNodeTraversalTest.java | 2 +- 12 files changed, 75 insertions(+), 13 deletions(-) diff --git a/Libraries/Animated/src/NativeAnimatedHelper.js b/Libraries/Animated/src/NativeAnimatedHelper.js index acb994a9b7d..41d0e52e60d 100644 --- a/Libraries/Animated/src/NativeAnimatedHelper.js +++ b/Libraries/Animated/src/NativeAnimatedHelper.js @@ -116,6 +116,13 @@ const API = { invariant(NativeAnimatedModule, 'Native animated module is not available'); NativeAnimatedModule.disconnectAnimatedNodeFromView(nodeTag, viewTag); }, + restoreDefaultValues: function(nodeTag: number): void { + invariant(NativeAnimatedModule, 'Native animated module is not available'); + // Backwards compat with older native runtimes, can be removed later. + if (NativeAnimatedModule.restoreDefaultValues != null) { + NativeAnimatedModule.restoreDefaultValues(nodeTag); + } + }, dropAnimatedNode: function(tag: number): void { invariant(NativeAnimatedModule, 'Native animated module is not available'); NativeAnimatedModule.dropAnimatedNode(tag); diff --git a/Libraries/Animated/src/NativeAnimatedModule.js b/Libraries/Animated/src/NativeAnimatedModule.js index 9c3ef2d9215..aa9e1257d2c 100644 --- a/Libraries/Animated/src/NativeAnimatedModule.js +++ b/Libraries/Animated/src/NativeAnimatedModule.js @@ -45,6 +45,7 @@ export interface Spec extends TurboModule { +extractAnimatedNodeOffset: (nodeTag: number) => void; +connectAnimatedNodeToView: (nodeTag: number, viewTag: number) => void; +disconnectAnimatedNodeFromView: (nodeTag: number, viewTag: number) => void; + +restoreDefaultValues: (nodeTag: number) => void; +dropAnimatedNode: (tag: number) => void; +addAnimatedEventToView: ( viewTag: number, diff --git a/Libraries/Animated/src/__tests__/AnimatedNative-test.js b/Libraries/Animated/src/__tests__/AnimatedNative-test.js index b6e34047f6c..c511638be6a 100644 --- a/Libraries/Animated/src/__tests__/AnimatedNative-test.js +++ b/Libraries/Animated/src/__tests__/AnimatedNative-test.js @@ -46,6 +46,7 @@ describe('Native Animated', () => { extractAnimatedNodeOffset: jest.fn(), flattenAnimatedNodeOffset: jest.fn(), removeAnimatedEventFromView: jest.fn(), + restoreDefaultValues: jest.fn(), setAnimatedNodeOffset: jest.fn(), setAnimatedNodeValue: jest.fn(), startAnimatingNode: jest.fn(), @@ -837,4 +838,25 @@ describe('Native Animated', () => { expect(NativeAnimatedModule.stopAnimation).toBeCalledWith(animationId); }); }); + + describe('Animated Components', () => { + it('Should restore default values on prop updates only', () => { + const opacity = new Animated.Value(0); + opacity.__makeNative(); + + const root = TestRenderer.create(); + expect(NativeAnimatedModule.restoreDefaultValues).not.toHaveBeenCalled(); + + root.update(); + expect(NativeAnimatedModule.restoreDefaultValues).toHaveBeenCalledWith( + expect.any(Number), + ); + + root.unmount(); + // Make sure it doesn't get called on unmount. + expect(NativeAnimatedModule.restoreDefaultValues).toHaveBeenCalledTimes( + 1, + ); + }); + }); }); diff --git a/Libraries/Animated/src/createAnimatedComponent.js b/Libraries/Animated/src/createAnimatedComponent.js index 4a47ce6203e..d803781e158 100644 --- a/Libraries/Animated/src/createAnimatedComponent.js +++ b/Libraries/Animated/src/createAnimatedComponent.js @@ -111,7 +111,10 @@ function createAnimatedComponent( // This way the intermediate state isn't to go to 0 and trigger // this expensive recursive detaching to then re-attach everything on // the very next operation. - oldPropsAnimated && oldPropsAnimated.__detach(); + if (oldPropsAnimated) { + oldPropsAnimated.__restoreDefaultValues(); + oldPropsAnimated.__detach(); + } } _setComponentRef = setAndForwardRef({ diff --git a/Libraries/Animated/src/nodes/AnimatedProps.js b/Libraries/Animated/src/nodes/AnimatedProps.js index b45073f8014..be630133f1a 100644 --- a/Libraries/Animated/src/nodes/AnimatedProps.js +++ b/Libraries/Animated/src/nodes/AnimatedProps.js @@ -147,6 +147,16 @@ class AnimatedProps extends AnimatedNode { ); } + __restoreDefaultValues(): void { + // When using the native driver, view properties need to be restored to + // their default values manually since react no longer tracks them. This + // is needed to handle cases where a prop driven by native animated is removed + // after having been changed natively by an animation. + if (this.__isNative) { + NativeAnimatedHelper.API.restoreDefaultValues(this.__getNativeTag()); + } + } + __getNativeConfig(): Object { const propsConfig = {}; for (const propKey in this._props) { diff --git a/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec-generated.mm b/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec-generated.mm index 2460facf8a0..36330dc0cf0 100644 --- a/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec-generated.mm +++ b/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec-generated.mm @@ -281,6 +281,10 @@ namespace facebook { return static_cast(turboModule).invokeObjCMethod(rt, VoidKind, "disconnectAnimatedNodeFromView", @selector(disconnectAnimatedNodeFromView:viewTag:), args, count); } + static facebook::jsi::Value __hostFunction_NativeAnimatedModuleSpecJSI_restoreDefaultValues(facebook::jsi::Runtime& rt, TurboModule &turboModule, const facebook::jsi::Value* args, size_t count) { + return static_cast(turboModule).invokeObjCMethod(rt, VoidKind, "restoreDefaultValues", @selector(restoreDefaultValues:), args, count); + } + static facebook::jsi::Value __hostFunction_NativeAnimatedModuleSpecJSI_dropAnimatedNode(facebook::jsi::Runtime& rt, TurboModule &turboModule, const facebook::jsi::Value* args, size_t count) { return static_cast(turboModule).invokeObjCMethod(rt, VoidKind, "dropAnimatedNode", @selector(dropAnimatedNode:), args, count); } @@ -344,6 +348,9 @@ namespace facebook { methodMap_["disconnectAnimatedNodeFromView"] = MethodMetadata {2, __hostFunction_NativeAnimatedModuleSpecJSI_disconnectAnimatedNodeFromView}; + methodMap_["restoreDefaultValues"] = MethodMetadata {1, __hostFunction_NativeAnimatedModuleSpecJSI_restoreDefaultValues}; + + methodMap_["dropAnimatedNode"] = MethodMetadata {1, __hostFunction_NativeAnimatedModuleSpecJSI_dropAnimatedNode}; diff --git a/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h b/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h index 51fe41d78de..1af255238dd 100644 --- a/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h +++ b/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h @@ -290,6 +290,7 @@ namespace JS { viewTag:(double)viewTag; - (void)disconnectAnimatedNodeFromView:(double)nodeTag viewTag:(double)viewTag; +- (void)restoreDefaultValues:(double)nodeTag; - (void)dropAnimatedNode:(double)tag; - (void)addAnimatedEventToView:(double)viewTag eventName:(NSString *)eventName diff --git a/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm b/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm index 6b7332c243a..ee4beec587d 100644 --- a/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm +++ b/Libraries/NativeAnimation/RCTNativeAnimatedModule.mm @@ -160,14 +160,18 @@ RCT_EXPORT_METHOD(connectAnimatedNodeToView:(double)nodeTag RCT_EXPORT_METHOD(disconnectAnimatedNodeFromView:(double)nodeTag viewTag:(double)viewTag) { - [self addPreOperationBlock:^(RCTNativeAnimatedNodesManager *nodesManager) { - [nodesManager restoreDefaultValues:[NSNumber numberWithDouble:nodeTag]]; - }]; [self addOperationBlock:^(RCTNativeAnimatedNodesManager *nodesManager) { [nodesManager disconnectAnimatedNodeFromView:[NSNumber numberWithDouble:nodeTag] viewTag:[NSNumber numberWithDouble:viewTag]]; }]; } +RCT_EXPORT_METHOD(restoreDefaultValues:(double)nodeTag) +{ + [self addPreOperationBlock:^(RCTNativeAnimatedNodesManager *nodesManager) { + [nodesManager restoreDefaultValues:[NSNumber numberWithDouble:nodeTag]]; + }]; +} + RCT_EXPORT_METHOD(dropAnimatedNode:(double)tag) { [self addOperationBlock:^(RCTNativeAnimatedNodesManager *nodesManager) { diff --git a/ReactAndroid/src/main/java/com/facebook/fbreact/specs/NativeAnimatedModuleSpec.java b/ReactAndroid/src/main/java/com/facebook/fbreact/specs/NativeAnimatedModuleSpec.java index 9ca4222c09a..91710dbc5eb 100644 --- a/ReactAndroid/src/main/java/com/facebook/fbreact/specs/NativeAnimatedModuleSpec.java +++ b/ReactAndroid/src/main/java/com/facebook/fbreact/specs/NativeAnimatedModuleSpec.java @@ -59,6 +59,9 @@ public abstract class NativeAnimatedModuleSpec extends ReactContextBaseJavaModul @ReactMethod public abstract void setAnimatedNodeOffset(double nodeTag, double offset); + @ReactMethod + public abstract void restoreDefaultValues(double nodeTag); + @ReactMethod public abstract void startAnimatingNode(double animationId, double nodeTag, ReadableMap config, Callback endCallback); diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java index 7bd186202a6..538f7672789 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java @@ -373,13 +373,6 @@ public class NativeAnimatedModule extends ReactContextBaseJavaModule @ReactMethod public void disconnectAnimatedNodeFromView(final int animatedNodeTag, final int viewTag) { - mPreOperations.add( - new UIThreadOperation() { - @Override - public void execute(NativeAnimatedNodesManager animatedNodesManager) { - animatedNodesManager.restoreDefaultValues(animatedNodeTag, viewTag); - } - }); mOperations.add( new UIThreadOperation() { @Override @@ -389,6 +382,17 @@ public class NativeAnimatedModule extends ReactContextBaseJavaModule }); } + @ReactMethod + public void restoreDefaultValues(final int animatedNodeTag) { + mPreOperations.add( + new UIThreadOperation() { + @Override + public void execute(NativeAnimatedNodesManager animatedNodesManager) { + animatedNodesManager.restoreDefaultValues(animatedNodeTag); + } + }); + } + @ReactMethod public void addAnimatedEventToView( final int viewTag, final String eventName, final ReadableMap eventMapping) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java index cff5d610d07..9ce9e66470c 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java @@ -318,7 +318,7 @@ import java.util.Queue; propsAnimatedNode.disconnectFromView(viewTag); } - public void restoreDefaultValues(int animatedNodeTag, int viewTag) { + public void restoreDefaultValues(int animatedNodeTag) { AnimatedNode node = mAnimatedNodes.get(animatedNodeTag); // Restoring default values needs to happen before UIManager operations so it is // possible the node hasn't been created yet if it is being connected and diff --git a/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java index 9ce85cac0d9..0a854969495 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/animated/NativeAnimatedNodeTraversalTest.java @@ -981,7 +981,7 @@ public class NativeAnimatedNodeTraversalTest { assertThat(stylesCaptor.getValue().getDouble("opacity")).isEqualTo(0); reset(mUIManagerMock); - mNativeAnimatedNodesManager.restoreDefaultValues(propsNodeTag, viewTag); + mNativeAnimatedNodesManager.restoreDefaultValues(propsNodeTag); verify(mUIManagerMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture()); assertThat(stylesCaptor.getValue().isNull("opacity")); }