Skip to content

Commit

Permalink
Don't restore default values when components unmount (#26978)
Browse files Browse the repository at this point in the history
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: #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
  • Loading branch information
janicduplessis authored and facebook-github-bot committed Nov 4, 2019
1 parent e9b4928 commit 686ab49
Show file tree
Hide file tree
Showing 12 changed files with 72 additions and 10 deletions.
7 changes: 7 additions & 0 deletions Libraries/Animated/src/NativeAnimatedHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions Libraries/Animated/src/NativeAnimatedModule.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
22 changes: 22 additions & 0 deletions Libraries/Animated/src/__tests__/AnimatedNative-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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(<Animated.View style={{opacity}} />);
expect(NativeAnimatedModule.restoreDefaultValues).not.toHaveBeenCalled();

root.update(<Animated.View style={{opacity}} />);
expect(NativeAnimatedModule.restoreDefaultValues).toHaveBeenCalledWith(
expect.any(Number),
);

root.unmount();
// Make sure it doesn't get called on unmount.
expect(NativeAnimatedModule.restoreDefaultValues).toHaveBeenCalledTimes(
1,
);
});
});
});
5 changes: 4 additions & 1 deletion Libraries/Animated/src/createAnimatedComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,10 @@ function createAnimatedComponent<Props: {+[string]: mixed}, Instance>(
// 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({
Expand Down
10 changes: 10 additions & 0 deletions Libraries/Animated/src/nodes/AnimatedProps.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ + (RCTManagedPointer *)JS_NativeAnimatedModule_EventMapping:(id)json
return static_cast<ObjCTurboModule&>(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<ObjCTurboModule&>(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<ObjCTurboModule&>(turboModule).invokeObjCMethod(rt, VoidKind, "dropAnimatedNode", @selector(dropAnimatedNode:), args, count);
}
Expand Down Expand Up @@ -344,6 +348,9 @@ + (RCTManagedPointer *)JS_NativeAnimatedModule_EventMapping:(id)json
methodMap_["disconnectAnimatedNodeFromView"] = MethodMetadata {2, __hostFunction_NativeAnimatedModuleSpecJSI_disconnectAnimatedNodeFromView};


methodMap_["restoreDefaultValues"] = MethodMetadata {1, __hostFunction_NativeAnimatedModuleSpecJSI_restoreDefaultValues};


methodMap_["dropAnimatedNode"] = MethodMetadata {1, __hostFunction_NativeAnimatedModuleSpecJSI_dropAnimatedNode};


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 7 additions & 3 deletions Libraries/NativeAnimation/RCTNativeAnimatedModule.mm
Original file line number Diff line number Diff line change
Expand Up @@ -160,14 +160,18 @@ - (void)setBridge:(RCTBridge *)bridge
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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ public abstract void removeAnimatedEventFromView(double viewTag, String eventNam
@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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,18 +373,22 @@ public void execute(NativeAnimatedNodesManager animatedNodesManager) {

@ReactMethod
public void disconnectAnimatedNodeFromView(final int animatedNodeTag, final int viewTag) {
mPreOperations.add(
mOperations.add(
new UIThreadOperation() {
@Override
public void execute(NativeAnimatedNodesManager animatedNodesManager) {
animatedNodesManager.restoreDefaultValues(animatedNodeTag, viewTag);
animatedNodesManager.disconnectAnimatedNodeFromView(animatedNodeTag, viewTag);
}
});
mOperations.add(
}

@ReactMethod
public void restoreDefaultValues(final int animatedNodeTag) {
mPreOperations.add(
new UIThreadOperation() {
@Override
public void execute(NativeAnimatedNodesManager animatedNodesManager) {
animatedNodesManager.disconnectAnimatedNodeFromView(animatedNodeTag, viewTag);
animatedNodesManager.restoreDefaultValues(animatedNodeTag);
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ public void disconnectAnimatedNodeFromView(int animatedNodeTag, int viewTag) {
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -981,7 +981,7 @@ public void testRestoreDefaultProps() {
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"));
}
Expand Down

0 comments on commit 686ab49

Please sign in to comment.