Skip to content

Commit d8c25ca

Browse files
genkikondofacebook-github-bot
authored andcommitted
Use initial value of natively driven nodes on renders
Summary: D36902220 (a041951) changed Animated to only use value of natively driven nodes on initial render. However, there remained a case where we could end up with a race condition between Fabric prop update (via SurfaceMountingManager.updateProps) and Animated (via NativeAnimatedNodesManager.runUpdates), when an animation on a node that was created natively is triggered close to render (such as in componentDidUpdate). This happens as Animated and Fabric aren't synchronized, and at the platform level, they do not know each other's state. Say we have two items, where opacity is used to indicate whether the item is selected. On initial render, A's opacity is set to 1, and animated sets opacity to 1; B's opacity is set to 0, and animated sets opacity to 0. When B is selected (and causes A and B to rerender), A's opacity is now set to null, and animated sets opacity to 0; B's opacity is also now set to null, and animated sets opacity to 1. A's props have changed, and thus the default opacity value of 1 is applied via Fabric, but Animated also sets the opacity to 0 - either may end up being the value visible to the user due to the nondeterministic order of Fabric update props and Animated. This is what is causing T122469354. This diff addresses this edge case by using the initial prop values for native animated nodes, for subsequent renders, to ensure that values of native animated nodes do not impact rerenders. This diff also fixes a bug in OCAnimation where translateX/Y values of 0 in transition will result in render props containing translateX/Y instead of transform, resulting in potentially incorrect pressability bounds. Changelog: [Internal][Fixed] - Only use initial value of natively driven nodes on render Reviewed By: JoshuaGross, javache Differential Revision: D36958882 fbshipit-source-id: 10be2ad91b645fa4b8a4a12808e9299da33aaf7d
1 parent b869680 commit d8c25ca

File tree

3 files changed

+27
-14
lines changed

3 files changed

+27
-14
lines changed

Libraries/Animated/createAnimatedComponent.js

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
5555
_prevComponent: any;
5656
_propsAnimated: AnimatedProps;
5757
_eventDetachers: Array<Function> = [];
58-
_isInitialRender: boolean = true;
58+
_initialAnimatedProps: Object;
5959

6060
// Only to be used in this file, and only in Fabric.
6161
_animatedComponentId: string = `${animatedComponentNextId++}:animatedComponent`;
@@ -201,12 +201,17 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
201201
});
202202

203203
render() {
204-
const {style = {}, ...props} =
205-
this._propsAnimated.__getValue(this._isInitialRender) || {};
204+
const animatedProps =
205+
this._propsAnimated.__getValue(this._initialAnimatedProps) || {};
206+
const {style = {}, ...props} = animatedProps;
206207
const {style: passthruStyle = {}, ...passthruProps} =
207208
this.props.passthroughAnimatedPropExplicitValues || {};
208209
const mergedStyle = {...style, ...passthruStyle};
209210

211+
if (!this._initialAnimatedProps) {
212+
this._initialAnimatedProps = animatedProps;
213+
}
214+
210215
// Force `collapsable` to be false so that native view is not flattened.
211216
// Flattened views cannot be accurately referenced by a native driver.
212217
return (
@@ -234,7 +239,6 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
234239
this._propsAnimated.setNativeView(this._component);
235240
this._attachNativeEvents();
236241
this._markUpdateComplete();
237-
this._isInitialRender = false;
238242
}
239243

240244
UNSAFE_componentWillReceiveProps(newProps: any) {

Libraries/Animated/nodes/AnimatedProps.js

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,25 +36,31 @@ class AnimatedProps extends AnimatedNode {
3636
this._callback = callback;
3737
}
3838

39-
__getValue(isInitialRender: boolean = true): Object {
39+
__getValue(initialProps: ?Object): Object {
4040
const props: {[string]: any | ((...args: any) => void)} = {};
4141
for (const key in this._props) {
4242
const value = this._props[key];
4343
if (value instanceof AnimatedNode) {
4444
// During initial render we want to use the initial value of both natively and non-natively
4545
// driven nodes. On subsequent renders, we cannot use the value of natively driven nodes
46-
// as they may not be up to date.
46+
// as they may not be up to date, so we use the initial value to ensure that values of
47+
// native animated nodes do not impact rerenders.
4748
if (value instanceof AnimatedStyle) {
48-
props[key] = value.__getValue(isInitialRender);
49-
} else if (isInitialRender || !value.__isNative) {
49+
props[key] = value.__getValue(
50+
initialProps ? initialProps.style : null,
51+
);
52+
} else if (!initialProps || !value.__isNative) {
5053
props[key] = value.__getValue();
54+
} else if (initialProps.hasOwnProperty(key)) {
55+
props[key] = initialProps[key];
5156
}
5257
} else if (value instanceof AnimatedEvent) {
5358
props[key] = value.__getHandler();
5459
} else {
5560
props[key] = value;
5661
}
5762
}
63+
5864
return props;
5965
}
6066

Libraries/Animated/nodes/AnimatedStyle.js

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,29 +34,32 @@ class AnimatedStyle extends AnimatedWithChildren {
3434
}
3535

3636
// Recursively get values for nested styles (like iOS's shadowOffset)
37-
_walkStyleAndGetValues(style: any, isInitialRender: boolean) {
37+
_walkStyleAndGetValues(style: any, initialStyle: ?Object) {
3838
const updatedStyle: {[string]: any | {...}} = {};
3939
for (const key in style) {
4040
const value = style[key];
4141
if (value instanceof AnimatedNode) {
4242
// During initial render we want to use the initial value of both natively and non-natively
4343
// driven nodes. On subsequent renders, we cannot use the value of natively driven nodes
44-
// as they may not be up to date.
45-
if (isInitialRender || !value.__isNative) {
44+
// as they may not be up to date, so we use the initial value to ensure that values of
45+
// native animated nodes do not impact rerenders.
46+
if (!initialStyle || !value.__isNative) {
4647
updatedStyle[key] = value.__getValue();
48+
} else if (initialStyle.hasOwnProperty(key)) {
49+
updatedStyle[key] = initialStyle[key];
4750
}
4851
} else if (value && !Array.isArray(value) && typeof value === 'object') {
4952
// Support animating nested values (for example: shadowOffset.height)
50-
updatedStyle[key] = this._walkStyleAndGetValues(value, isInitialRender);
53+
updatedStyle[key] = this._walkStyleAndGetValues(value, initialStyle);
5154
} else {
5255
updatedStyle[key] = value;
5356
}
5457
}
5558
return updatedStyle;
5659
}
5760

58-
__getValue(isInitialRender: boolean = true): Object {
59-
return this._walkStyleAndGetValues(this._style, isInitialRender);
61+
__getValue(initialStyle: ?Object): Object {
62+
return this._walkStyleAndGetValues(this._style, initialStyle);
6063
}
6164

6265
// Recursively get animated values for nested styles (like iOS's shadowOffset)

0 commit comments

Comments
 (0)