From 5c8c5388fc53ef2430b0bb6bbbc628479819e23d Mon Sep 17 00:00:00 2001 From: Tim Yung Date: Mon, 2 Jun 2025 19:19:03 -0700 Subject: [PATCH] RN: Fix Shadowing of Animated Styles (#51719) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/51719 When `Animated` traverses props for instances of `AnimatedNode`, it flattens `props.style` before traversing it so that we correctly ignore any `AnimatedNode` instances that may be shadowed by static values: ``` { style: [ {transform: [{translateX: new Animated.Value(0)}]}, {transform: [{translateX: 100}]}, ], } ``` However, there is a bug that occurs when *every* `AnimatedNode` instance is shadowed. In this case, `AnimatedProps` assumes that there are *no* `AnimatedNode` instances in the entire `props.style`. It then incorrectly operates on the unflattened `props.style`, which *does* have `AnimatedNode` instances. When this is passed to `View`, the `AnimatedNode` instances are encountered and can cause a crash in `processTransform`, as reported by: https://github.com/facebook/react-native/issues/51395 The fix for this was originally attempted by riteshshukla04 in https://github.com/facebook/react-native/pull/51442. This diff reuses the same unit test case, but it applies a different fix that does not involve re-traversing the `props.style` object. The fix is gated behind a feature flag, `alwaysFlattenAnimatedStyles`. This will enable us to validate correctness of the new behavior before enabling it for everyone. (Beyond fixing the bug described above, this also causes styles to flatten more aggressively, so production testing is important to ensure stability.) Changelog: [General][Changed] - Creates a feature flag that changes Animated to no longer produce invalid `props.style` if every `AnimatedNode` instance is shadowed via style flattening. Reviewed By: javache Differential Revision: D75723284 fbshipit-source-id: 504f63e8edf836243d615783e119137a920ad271 --- .../Animated/__tests__/Animated-test.js | 21 ++++++ .../Libraries/Animated/nodes/AnimatedProps.js | 40 +++++++++-- .../Libraries/Animated/nodes/AnimatedStyle.js | 69 ++++++++++--------- .../__snapshots__/public-api-test.js.snap | 9 ++- .../ReactNativeFeatureFlags.config.js | 12 +++- .../featureflags/ReactNativeFeatureFlags.js | 8 ++- 6 files changed, 116 insertions(+), 43 deletions(-) diff --git a/packages/react-native/Libraries/Animated/__tests__/Animated-test.js b/packages/react-native/Libraries/Animated/__tests__/Animated-test.js index 0ac2a1d36c4..da78bc4c353 100644 --- a/packages/react-native/Libraries/Animated/__tests__/Animated-test.js +++ b/packages/react-native/Libraries/Animated/__tests__/Animated-test.js @@ -245,6 +245,27 @@ describe('Animated', () => { expect(callback).toBeCalled(); }); + it('renders animated and primitive style correctly', () => { + ReactNativeFeatureFlags.override({ + alwaysFlattenAnimatedStyles: () => true, + }); + + const anim = new Animated.Value(0); + const staticProps = { + style: [ + {transform: [{translateX: anim}]}, + {transform: [{translateX: 100}]}, + ], + }; + const staticPropsWithoutAnim = { + style: {transform: [{translateX: 100}]}, + }; + const node = new AnimatedProps(staticProps, jest.fn()); + expect(node.__getValueWithStaticProps(staticProps)).toStrictEqual( + staticPropsWithoutAnim, + ); + }); + it('send toValue when a critically damped spring stops', () => { const anim = new Animated.Value(0); const listener = jest.fn(); diff --git a/packages/react-native/Libraries/Animated/nodes/AnimatedProps.js b/packages/react-native/Libraries/Animated/nodes/AnimatedProps.js index 0364ed4e958..129cb1d28c3 100644 --- a/packages/react-native/Libraries/Animated/nodes/AnimatedProps.js +++ b/packages/react-native/Libraries/Animated/nodes/AnimatedProps.js @@ -13,7 +13,9 @@ import type {AnimatedNodeConfig} from './AnimatedNode'; import type {AnimatedStyleAllowlist} from './AnimatedStyle'; import NativeAnimatedHelper from '../../../src/private/animated/NativeAnimatedHelper'; +import * as ReactNativeFeatureFlags from '../../../src/private/featureflags/ReactNativeFeatureFlags'; import {findNodeHandle} from '../../ReactNative/RendererProxy'; +import flattenStyle from '../../StyleSheet/flattenStyle'; import {AnimatedEvent} from '../AnimatedEvent'; import AnimatedNode from './AnimatedNode'; import AnimatedObject from './AnimatedObject'; @@ -43,18 +45,30 @@ function createAnimatedProps( for (let ii = 0, length = keys.length; ii < length; ii++) { const key = keys[ii]; const value = inputProps[key]; + let staticValue = value; if (allowlist == null || hasOwn(allowlist, key)) { let node; if (key === 'style') { - node = AnimatedStyle.from(value, allowlist?.style); + // Ignore `style` if it is not an object (or array). + if (typeof value === 'object' && value != null) { + // Even if we do not find any `AnimatedNode` values in `style`, we + // still need to use the flattened `style` object because static + // values can shadow `AnimatedNode` values. We need to make sure that + // we propagate the flattened `style` object to the `props` object. + const flatStyle = flattenStyle(value as $FlowFixMe); + node = AnimatedStyle.from(flatStyle, allowlist?.style, value); + if (ReactNativeFeatureFlags.alwaysFlattenAnimatedStyles()) { + staticValue = flatStyle; + } + } } else if (value instanceof AnimatedNode) { node = value; } else { node = AnimatedObject.from(value); } if (node == null) { - props[key] = value; + props[key] = staticValue; } else { nodeKeys.push(key); nodes.push(node); @@ -134,8 +148,26 @@ export default class AnimatedProps extends AnimatedNode { const key = keys[ii]; const maybeNode = this.#props[key]; - if (key === 'style' && maybeNode instanceof AnimatedStyle) { - props[key] = maybeNode.__getValueWithStaticStyle(staticProps.style); + if (key === 'style') { + const staticStyle = staticProps.style; + const flatStaticStyle = flattenStyle(staticStyle); + if (maybeNode instanceof AnimatedStyle) { + const mutableStyle: {[string]: mixed} = + flatStaticStyle == null + ? {} + : flatStaticStyle === staticStyle + ? // Copy the input style, since we'll mutate it below. + {...flatStaticStyle} + : // Reuse `flatStaticStyle` if it is a newly created object. + flatStaticStyle; + + maybeNode.__replaceAnimatedNodeWithValues(mutableStyle); + props[key] = maybeNode.__getValueForStyle(mutableStyle); + } else { + if (ReactNativeFeatureFlags.alwaysFlattenAnimatedStyles()) { + props[key] = flatStaticStyle; + } + } } else if (maybeNode instanceof AnimatedNode) { props[key] = maybeNode.__getValue(); } else if (maybeNode instanceof AnimatedEvent) { diff --git a/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js b/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js index 89a2561ac44..87fee9c0c59 100644 --- a/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js +++ b/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js @@ -13,7 +13,6 @@ import type {AnimatedNodeConfig} from './AnimatedNode'; import {validateStyles} from '../../../src/private/animated/NativeAnimatedValidation'; import * as ReactNativeFeatureFlags from '../../../src/private/featureflags/ReactNativeFeatureFlags'; -import flattenStyle from '../../StyleSheet/flattenStyle'; import Platform from '../../Utilities/Platform'; import AnimatedNode from './AnimatedNode'; import AnimatedObject from './AnimatedObject'; @@ -22,8 +21,11 @@ import AnimatedWithChildren from './AnimatedWithChildren'; export type AnimatedStyleAllowlist = $ReadOnly<{[string]: true}>; +type FlatStyle = {[string]: mixed}; +type FlatStyleForWeb = [mixed, TStyle]; + function createAnimatedStyle( - inputStyle: {[string]: mixed}, + flatStyle: FlatStyle, allowlist: ?AnimatedStyleAllowlist, keepUnanimatedValues: boolean, ): [$ReadOnlyArray, $ReadOnlyArray, {[string]: mixed}] { @@ -31,10 +33,10 @@ function createAnimatedStyle( const nodes: Array = []; const style: {[string]: mixed} = {}; - const keys = Object.keys(inputStyle); + const keys = Object.keys(flatStyle); for (let ii = 0, length = keys.length; ii < length; ii++) { const key = keys[ii]; - const value = inputStyle[key]; + const value = flatStyle[key]; if (allowlist == null || hasOwn(allowlist, key)) { let node; @@ -62,10 +64,10 @@ function createAnimatedStyle( // WARNING: This is a potentially expensive check that we should only // do in development. Without this check in development, it might be // difficult to identify which styles need to be allowlisted. - if (AnimatedObject.from(inputStyle[key]) != null) { + if (AnimatedObject.from(flatStyle[key]) != null) { console.error( - `AnimatedStyle: ${key} is not allowlisted for animation, but it ` + - 'contains AnimatedNode values; styles allowing animation: ', + `AnimatedStyle: ${key} is not allowlisted for animation, but ` + + 'it contains AnimatedNode values; styles allowing animation: ', allowlist, ); } @@ -80,7 +82,7 @@ function createAnimatedStyle( } export default class AnimatedStyle extends AnimatedWithChildren { - #inputStyle: any; + #originalStyleForWeb: ?mixed; #nodeKeys: $ReadOnlyArray; #nodes: $ReadOnlyArray; #style: {[string]: mixed}; @@ -90,10 +92,10 @@ export default class AnimatedStyle extends AnimatedWithChildren { * Otherwise, returns `null`. */ static from( - inputStyle: any, + flatStyle: ?FlatStyle, allowlist: ?AnimatedStyleAllowlist, + originalStyleForWeb: ?mixed, ): ?AnimatedStyle { - const flatStyle = flattenStyle(inputStyle); if (flatStyle == null) { return null; } @@ -105,24 +107,31 @@ export default class AnimatedStyle extends AnimatedWithChildren { if (nodes.length === 0) { return null; } - return new AnimatedStyle(nodeKeys, nodes, style, inputStyle); + return new AnimatedStyle(nodeKeys, nodes, style, originalStyleForWeb); } constructor( nodeKeys: $ReadOnlyArray, nodes: $ReadOnlyArray, style: {[string]: mixed}, - inputStyle: any, + originalStyleForWeb: ?mixed, config?: ?AnimatedNodeConfig, ) { super(config); this.#nodeKeys = nodeKeys; this.#nodes = nodes; this.#style = style; - this.#inputStyle = inputStyle; + + if ((Platform.OS as string) === 'web') { + // $FlowIgnore[cannot-write] - Intentional shadowing. + this.__getValueForStyle = resultStyle => [ + originalStyleForWeb, + resultStyle, + ]; + } } - __getValue(): Object | Array { + __getValue(): FlatStyleForWeb | FlatStyle { const style: {[string]: mixed} = {}; const keys = Object.keys(this.#style); @@ -137,27 +146,23 @@ export default class AnimatedStyle extends AnimatedWithChildren { } } - /* $FlowFixMe[incompatible-type] Error found due to incomplete typing of - * Platform.flow.js */ - return Platform.OS === 'web' ? [this.#inputStyle, style] : style; + return this.__getValueForStyle(style); } /** - * Creates a new `style` object that contains the same style properties as - * the supplied `staticStyle` object, except with animated nodes for any - * style properties that were created by this `AnimatedStyle` instance. + * See the constructor, where this is shadowed on web platforms. */ - __getValueWithStaticStyle(staticStyle: Object): Object | Array { - const flatStaticStyle = flattenStyle(staticStyle); - const style: {[string]: mixed} = - flatStaticStyle == null - ? {} - : flatStaticStyle === staticStyle - ? // Copy the input style, since we'll mutate it below. - {...flatStaticStyle} - : // Reuse `flatStaticStyle` if it is a newly created object. - flatStaticStyle; + __getValueForStyle( + style: TStyle, + ): FlatStyleForWeb | TStyle { + return style; + } + /** + * Mutates the supplied `style` object such that animated nodes are replaced + * with rasterized values. + */ + __replaceAnimatedNodeWithValues(style: {[string]: mixed}): void { const keys = Object.keys(style); for (let ii = 0, length = keys.length; ii < length; ii++) { const key = keys[ii]; @@ -175,10 +180,6 @@ export default class AnimatedStyle extends AnimatedWithChildren { style[key] = maybeNode.__getValue(); } } - - /* $FlowFixMe[incompatible-type] Error found due to incomplete typing of - * Platform.flow.js */ - return Platform.OS === 'web' ? [this.#inputStyle, style] : style; } __getAnimatedValue(): Object { diff --git a/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap b/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap index cf3cda18943..8002401cdf7 100644 --- a/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap +++ b/packages/react-native/Libraries/__tests__/__snapshots__/public-api-test.js.snap @@ -925,16 +925,19 @@ declare export default class AnimatedProps extends AnimatedNode { exports[`public API should not change unintentionally Libraries/Animated/nodes/AnimatedStyle.js 1`] = ` "export type AnimatedStyleAllowlist = $ReadOnly<{ [string]: true }>; +type FlatStyle = { [string]: mixed }; +type FlatStyleForWeb = [mixed, TStyle]; declare export default class AnimatedStyle extends AnimatedWithChildren { static from( - inputStyle: any, - allowlist: ?AnimatedStyleAllowlist + flatStyle: ?FlatStyle, + allowlist: ?AnimatedStyleAllowlist, + originalStyleForWeb: ?mixed ): ?AnimatedStyle; constructor( nodeKeys: $ReadOnlyArray, nodes: $ReadOnlyArray, style: { [string]: mixed }, - inputStyle: any, + originalStyleForWeb: ?mixed, config?: ?AnimatedNodeConfig ): void; } diff --git a/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js b/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js index 1f76052a5d6..68500385508 100644 --- a/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js +++ b/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js @@ -621,7 +621,17 @@ const definitions: FeatureFlagDefinitions = { jsOnly: { ...testDefinitions.jsOnly, - + alwaysFlattenAnimatedStyles: { + defaultValue: false, + metadata: { + dateAdded: '2025-06-02', + description: + 'Changes `Animated` to always flatten style, fixing a bug with shadowed `AnimatedNode` instances.', + expectedReleaseValue: true, + purpose: 'experimentation', + }, + ossReleaseStage: 'none', + }, animatedShouldDebounceQueueFlush: { defaultValue: false, metadata: { diff --git a/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js b/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js index b6071cff819..e7a2c4ef3db 100644 --- a/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js +++ b/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<<7fe2abc6b638728b09b0194988f0264c>> + * @generated SignedSource<> * @flow strict * @noformat */ @@ -29,6 +29,7 @@ import { export type ReactNativeFeatureFlagsJsOnly = $ReadOnly<{ jsOnlyTestFlag: Getter, + alwaysFlattenAnimatedStyles: Getter, animatedShouldDebounceQueueFlush: Getter, animatedShouldUseSingleOp: Getter, avoidStateUpdateInAnimatedPropsMemo: Getter, @@ -111,6 +112,11 @@ export type ReactNativeFeatureFlags = $ReadOnly<{ */ export const jsOnlyTestFlag: Getter = createJavaScriptFlagGetter('jsOnlyTestFlag', false); +/** + * Changes `Animated` to always flatten style, fixing a bug with shadowed `AnimatedNode` instances. + */ +export const alwaysFlattenAnimatedStyles: Getter = createJavaScriptFlagGetter('alwaysFlattenAnimatedStyles', false); + /** * Enables an experimental flush-queue debouncing in Animated.js. */