Skip to content

Commit

Permalink
Use initial value of natively driven nodes on renders
Browse files Browse the repository at this point in the history
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
  • Loading branch information
genkikondo authored and facebook-github-bot committed Jun 8, 2022
1 parent b869680 commit d8c25ca
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 14 deletions.
12 changes: 8 additions & 4 deletions Libraries/Animated/createAnimatedComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
_prevComponent: any;
_propsAnimated: AnimatedProps;
_eventDetachers: Array<Function> = [];
_isInitialRender: boolean = true;
_initialAnimatedProps: Object;

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

render() {
const {style = {}, ...props} =
this._propsAnimated.__getValue(this._isInitialRender) || {};
const animatedProps =
this._propsAnimated.__getValue(this._initialAnimatedProps) || {};
const {style = {}, ...props} = animatedProps;
const {style: passthruStyle = {}, ...passthruProps} =
this.props.passthroughAnimatedPropExplicitValues || {};
const mergedStyle = {...style, ...passthruStyle};

if (!this._initialAnimatedProps) {
this._initialAnimatedProps = animatedProps;
}

// Force `collapsable` to be false so that native view is not flattened.
// Flattened views cannot be accurately referenced by a native driver.
return (
Expand Down Expand Up @@ -234,7 +239,6 @@ function createAnimatedComponent<Props: {+[string]: mixed, ...}, Instance>(
this._propsAnimated.setNativeView(this._component);
this._attachNativeEvents();
this._markUpdateComplete();
this._isInitialRender = false;
}

UNSAFE_componentWillReceiveProps(newProps: any) {
Expand Down
14 changes: 10 additions & 4 deletions Libraries/Animated/nodes/AnimatedProps.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,31 @@ class AnimatedProps extends AnimatedNode {
this._callback = callback;
}

__getValue(isInitialRender: boolean = true): Object {
__getValue(initialProps: ?Object): Object {
const props: {[string]: any | ((...args: any) => void)} = {};
for (const key in this._props) {
const value = this._props[key];
if (value instanceof AnimatedNode) {
// During initial render we want to use the initial value of both natively and non-natively
// driven nodes. On subsequent renders, we cannot use the value of natively driven nodes
// as they may not be up to date.
// as they may not be up to date, so we use the initial value to ensure that values of
// native animated nodes do not impact rerenders.
if (value instanceof AnimatedStyle) {
props[key] = value.__getValue(isInitialRender);
} else if (isInitialRender || !value.__isNative) {
props[key] = value.__getValue(
initialProps ? initialProps.style : null,
);
} else if (!initialProps || !value.__isNative) {
props[key] = value.__getValue();
} else if (initialProps.hasOwnProperty(key)) {
props[key] = initialProps[key];
}
} else if (value instanceof AnimatedEvent) {
props[key] = value.__getHandler();
} else {
props[key] = value;
}
}

return props;
}

Expand Down
15 changes: 9 additions & 6 deletions Libraries/Animated/nodes/AnimatedStyle.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,32 @@ class AnimatedStyle extends AnimatedWithChildren {
}

// Recursively get values for nested styles (like iOS's shadowOffset)
_walkStyleAndGetValues(style: any, isInitialRender: boolean) {
_walkStyleAndGetValues(style: any, initialStyle: ?Object) {
const updatedStyle: {[string]: any | {...}} = {};
for (const key in style) {
const value = style[key];
if (value instanceof AnimatedNode) {
// During initial render we want to use the initial value of both natively and non-natively
// driven nodes. On subsequent renders, we cannot use the value of natively driven nodes
// as they may not be up to date.
if (isInitialRender || !value.__isNative) {
// as they may not be up to date, so we use the initial value to ensure that values of
// native animated nodes do not impact rerenders.
if (!initialStyle || !value.__isNative) {
updatedStyle[key] = value.__getValue();
} else if (initialStyle.hasOwnProperty(key)) {
updatedStyle[key] = initialStyle[key];
}
} else if (value && !Array.isArray(value) && typeof value === 'object') {
// Support animating nested values (for example: shadowOffset.height)
updatedStyle[key] = this._walkStyleAndGetValues(value, isInitialRender);
updatedStyle[key] = this._walkStyleAndGetValues(value, initialStyle);
} else {
updatedStyle[key] = value;
}
}
return updatedStyle;
}

__getValue(isInitialRender: boolean = true): Object {
return this._walkStyleAndGetValues(this._style, isInitialRender);
__getValue(initialStyle: ?Object): Object {
return this._walkStyleAndGetValues(this._style, initialStyle);
}

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

0 comments on commit d8c25ca

Please sign in to comment.