From d8c25ca1b62df2b93f70bbb1f7b379643ab9ccd4 Mon Sep 17 00:00:00 2001 From: Genki Kondo Date: Tue, 7 Jun 2022 20:02:57 -0700 Subject: [PATCH] Use initial value of natively driven nodes on renders Summary: D36902220 (https://github.com/facebook/react-native/commit/a04195167bbd8f27c6141c0239a61a345cac5a88) 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 --- Libraries/Animated/createAnimatedComponent.js | 12 ++++++++---- Libraries/Animated/nodes/AnimatedProps.js | 14 ++++++++++---- Libraries/Animated/nodes/AnimatedStyle.js | 15 +++++++++------ 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/Libraries/Animated/createAnimatedComponent.js b/Libraries/Animated/createAnimatedComponent.js index a59b5fd4fd3..c9dbe808ed7 100644 --- a/Libraries/Animated/createAnimatedComponent.js +++ b/Libraries/Animated/createAnimatedComponent.js @@ -55,7 +55,7 @@ function createAnimatedComponent( _prevComponent: any; _propsAnimated: AnimatedProps; _eventDetachers: Array = []; - _isInitialRender: boolean = true; + _initialAnimatedProps: Object; // Only to be used in this file, and only in Fabric. _animatedComponentId: string = `${animatedComponentNextId++}:animatedComponent`; @@ -201,12 +201,17 @@ function createAnimatedComponent( }); 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 ( @@ -234,7 +239,6 @@ function createAnimatedComponent( this._propsAnimated.setNativeView(this._component); this._attachNativeEvents(); this._markUpdateComplete(); - this._isInitialRender = false; } UNSAFE_componentWillReceiveProps(newProps: any) { diff --git a/Libraries/Animated/nodes/AnimatedProps.js b/Libraries/Animated/nodes/AnimatedProps.js index ead32545bc8..d4ed70da8d6 100644 --- a/Libraries/Animated/nodes/AnimatedProps.js +++ b/Libraries/Animated/nodes/AnimatedProps.js @@ -36,18 +36,23 @@ 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(); @@ -55,6 +60,7 @@ class AnimatedProps extends AnimatedNode { props[key] = value; } } + return props; } diff --git a/Libraries/Animated/nodes/AnimatedStyle.js b/Libraries/Animated/nodes/AnimatedStyle.js index 79df1468d25..c64706a8158 100644 --- a/Libraries/Animated/nodes/AnimatedStyle.js +++ b/Libraries/Animated/nodes/AnimatedStyle.js @@ -34,20 +34,23 @@ 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; } @@ -55,8 +58,8 @@ class AnimatedStyle extends AnimatedWithChildren { 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)