fix: Patch AnimatedStyle to avoid discarding the initial style info #35198

gabrieldonadel · 2022-11-04T02:12:08Z

Summary

This PR patches AnimatedStyle to avoid discarding the initial style information which destroys the output of web-style compilers and breaks rendering, as requested on #34425. This uses a slightly modified version of a patch used by react-native-web necolas/react-native-web@4c678d2.

Changelog

[General] [Fixed] - Patch AnimatedStyle to avoid discarding the initial style info

Test Plan

Run yarn jest Animated and ensure CI is green

analysis-bot · 2022-11-04T02:34:42Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,070,315	-31,948
android	hermes	armeabi-v7a	6,442,768	-28,676
android	hermes	x86	7,485,557	-34,042
android	hermes	x86_64	7,345,103	-32,923
android	jsc	arm64-v8a	8,934,976	-32,057
android	jsc	armeabi-v7a	7,669,092	-28,785
android	jsc	x86	8,995,029	-34,166
android	jsc	x86_64	9,473,961	-33,049

Base commit: e504141
Branch: main

analysis-bot · 2022-11-04T03:04:05Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: e504141
Branch: main

pull-bot · 2022-11-04T03:17:04Z

PR build artifact for bdc9819 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

javache · 2022-11-04T09:29:31Z

Please provide a unit tests that shows how this behaviour was broken before and how this change resolves it. This will help me understand the goal here and prevent us from breaking it in the future :)

gabrieldonadel · 2022-11-04T12:30:20Z

Please provide a unit tests that shows how this behaviour was broken before and how this change resolves it. This will help me understand the goal here and prevent us from breaking it in the future :)

@necolas would you mind helping me with this one? I know that the problem is described on necolas/react-native-web#2387 but I'm not completely sure how to recreate that scenario on this repo

necolas · 2022-11-04T17:36:01Z

Libraries/Animated/nodes/AnimatedStyle.js

  _style: Object;

  constructor(style: any) {
    super();
-    style = flattenStyle(style) || ({}: {[string]: any});


On web, using flattenStyle creates a new object each time which causes correctness and perf issues:

It mixes inline and compiled styles into a single object.

The style runtime can no longer recognize the style object as static / pre-compiled.

The style runtime can no longer cache the result of style merges involving this style.

necolas · 2022-11-04T17:38:23Z

Libraries/Animated/nodes/AnimatedStyle.js

+function createAnimatedStyle(inputStyle: any): Object {
+  const style = flattenStyle(inputStyle);
+  const animatedStyles: any = {};
+  for (const key in style) {
+    const value = style[key];
+    if (key === 'transform') {
+      animatedStyles[key] = new AnimatedTransform(value);
+    } else if (value instanceof AnimatedNode) {
+      animatedStyles[key] = value;
+    } else if (value && !Array.isArray(value) && typeof value === 'object') {
+      animatedStyles[key] = createAnimatedStyle(value);
+    }
+  }
+  return animatedStyles;
+}


Since Animated nodes can only exist on inline styles, this function plucks them out of the provided styles and places them into a separate object that can be appended to the original input styles.

Libraries/Animated/nodes/AnimatedStyle.js

necolas · 2022-11-04T17:47:16Z

Please provide a unit tests that shows how this behaviour was broken before and how this change resolves it. This will help me understand the goal here and prevent us from breaking it in the future :)

The behavior is broken on web because using flattenStyle is fundamentally incompatible with any approach to compiling styles to CSS. necolas/react-native-web#2387

A unit test for this change in RN would only show that Animated nodes are duplicated in a new style object appended to the input styles, and that the return from the Style node is an array instead of an object. There would be no functional difference on native.

pull-bot · 2022-11-08T04:57:15Z

PR build artifact for c47a9f4 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-08T12:55:11Z

PR build artifact for c45bf58 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-09T00:01:54Z

PR build artifact for b45837a is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

Libraries/Animated/__tests__/Animated-web-test.js

Libraries/Animated/nodes/AnimatedStyle.js

javache · 2022-11-10T14:48:51Z

Does this correctly handle scenarios like this?

style={[
   {backgroundColor: animatedValue},
   {backgroundColor: 'red'},
]}

The expected outcome would be that animatedValue is not used.

necolas · 2022-11-10T17:39:52Z

Yes, it does. The first thing we do is a flatten, and then extract any animated values that are left

pull-bot · 2022-11-11T04:49:12Z

PR build artifact for 45e8c02 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

gabrieldonadel · 2022-11-12T21:26:49Z

@necolas FYI this should be good for another review round

Libraries/Animated/__tests__/Animated-web-test.js

necolas · 2022-11-14T18:26:24Z

Libraries/Animated/__tests__/Animated-web-test.js

+    it('does not discard initial style', () => {
+      const value1 = new Animated.Value(1);
+      const scale = value1.interpolate({
+        inputRange: [0, 1],
+        outputRange: [1, 2],
+      });
+      const callback = jest.fn();
+      const node = new AnimatedProps(
+        {
+          style: {
+            transform: [
+              {
+                scale,
+              },
+            ],
+          },
+        },
+        callback,
+      );


This test will be a bit confusing without changes / explanation. The behavior matters when the input style is a mix of values from StyleSheet.create (ie contains no animated values, could be optimized to another format) and an inline style with an animation

Sounds good, I've updated tests a bit to include StyleSheet.create

pull-bot · 2022-11-16T05:07:00Z

PR build artifact for 5b6fede is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-11-17T06:04:07Z

PR build artifact for 650c45e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

Libraries/Animated/nodes/AnimatedStyle.js

pull-bot · 2022-11-17T16:14:40Z

PR build artifact for 11daeda is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

gabrieldonadel · 2022-11-17T17:45:34Z

Hi @cipolleschi, do you mind importing this PR? 😅

facebook-github-bot · 2022-11-17T17:58:20Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…acebook#35198) Summary: This PR patches `AnimatedStyle` to avoid discarding the initial style information which destroys the output of web-style compilers and breaks rendering, as requested on facebook#34425. This uses a slightly modified version of a patch used by react-native-web necolas/react-native-web@4c678d2. ## Changelog [General] [Fixed] - Patch AnimatedStyle to avoid discarding the initial style info Pull Request resolved: facebook#35198 Test Plan: Run `yarn jest Animated` and ensure CI is green ![image](https://user-images.githubusercontent.com/11707729/199869612-4f2302da-5791-492f-83a7-683305757c23.png) Reviewed By: necolas Differential Revision: D41379826 Pulled By: javache fbshipit-source-id: 7e16726828b98def14847ec3499ff93777a9cbfb

…41176) Summary: #35198 introduced a regression where if an `{transform: undefined}` style is provided to an Animated View a `Cannot read property 'map' of undefined` type error is thrown <img src="https://github.com/facebook/react-native/assets/11707729/bb87781e-1ba7-40ec-879d-a57cef3e10d9" height="200" /> ## Changelog: [GENERAL] [FIXED] - Fix `createAnimatedStyle` when providing an undefined transform style Pull Request resolved: #41176 Test Plan: <details> <summary>Render an `Animated.View` passing `style={{transform: undefined}}`</summary> E.g. ``` const UndefinedTransform = () => { return ( <View> <Animated.View style={{transform: undefined}} /> </View> ); }; ``` </details> ### RNTester 1. Open the RNTester app and navigate to the Animated page 2. Navigate to the Transform Styles page 3. App should not throw any errors <table> <tr><th>Before</th><th>After</th></tr> <tr> <td><video src="https://github.com/facebook/react-native/assets/11707729/92ba9c3b-60b0-4805-8080-0e7fb7c00345"/></td> <td><video src="https://github.com/facebook/react-native/assets/11707729/80e2bba8-6ff6-4cf5-bcb8-26de0b869036"/></td> </tr> </table> Reviewed By: fabriziocucci Differential Revision: D50638415 Pulled By: javache fbshipit-source-id: 0ee949f019a77b8bef557888694e0e8404810105

…acebook#41176) Summary: facebook#35198 introduced a regression where if an `{transform: undefined}` style is provided to an Animated View a `Cannot read property 'map' of undefined` type error is thrown <img src="https://github.com/facebook/react-native/assets/11707729/bb87781e-1ba7-40ec-879d-a57cef3e10d9" height="200" /> ## Changelog: [GENERAL] [FIXED] - Fix `createAnimatedStyle` when providing an undefined transform style Pull Request resolved: facebook#41176 Test Plan: <details> <summary>Render an `Animated.View` passing `style={{transform: undefined}}`</summary> E.g. ``` const UndefinedTransform = () => { return ( <View> <Animated.View style={{transform: undefined}} /> </View> ); }; ``` </details> ### RNTester 1. Open the RNTester app and navigate to the Animated page 2. Navigate to the Transform Styles page 3. App should not throw any errors <table> <tr><th>Before</th><th>After</th></tr> <tr> <td><video src="https://github.com/facebook/react-native/assets/11707729/92ba9c3b-60b0-4805-8080-0e7fb7c00345"/></td> <td><video src="https://github.com/facebook/react-native/assets/11707729/80e2bba8-6ff6-4cf5-bcb8-26de0b869036"/></td> </tr> </table> Reviewed By: fabriziocucci Differential Revision: D50638415 Pulled By: javache fbshipit-source-id: 0ee949f019a77b8bef557888694e0e8404810105

…41176) Summary: #35198 introduced a regression where if an `{transform: undefined}` style is provided to an Animated View a `Cannot read property 'map' of undefined` type error is thrown <img src="https://github.com/facebook/react-native/assets/11707729/bb87781e-1ba7-40ec-879d-a57cef3e10d9" height="200" /> ## Changelog: [GENERAL] [FIXED] - Fix `createAnimatedStyle` when providing an undefined transform style Pull Request resolved: #41176 Test Plan: <details> <summary>Render an `Animated.View` passing `style={{transform: undefined}}`</summary> E.g. ``` const UndefinedTransform = () => { return ( <View> <Animated.View style={{transform: undefined}} /> </View> ); }; ``` </details> ### RNTester 1. Open the RNTester app and navigate to the Animated page 2. Navigate to the Transform Styles page 3. App should not throw any errors <table> <tr><th>Before</th><th>After</th></tr> <tr> <td><video src="https://github.com/facebook/react-native/assets/11707729/92ba9c3b-60b0-4805-8080-0e7fb7c00345"/></td> <td><video src="https://github.com/facebook/react-native/assets/11707729/80e2bba8-6ff6-4cf5-bcb8-26de0b869036"/></td> </tr> </table> Reviewed By: fabriziocucci Differential Revision: D50638415 Pulled By: javache fbshipit-source-id: 0ee949f019a77b8bef557888694e0e8404810105

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 4, 2022

react-native-bot added the Bug label Nov 4, 2022

necolas reviewed Nov 4, 2022

View reviewed changes

necolas mentioned this pull request Nov 4, 2022

[w3c] ☂️ Web Styles (Part 1) umbrella issue #34425

Open

74 tasks

gabrieldonadel force-pushed the fix/patch-animated-style branch from bdc9819 to c47a9f4 Compare November 8, 2022 03:53

gabrieldonadel requested a review from necolas November 8, 2022 03:54

gabrieldonadel force-pushed the fix/patch-animated-style branch from ba6f59d to c45bf58 Compare November 8, 2022 12:24

necolas mentioned this pull request Nov 8, 2022

☂️ npm packages for react-native-web #35263

Open

13 tasks

gabrieldonadel added 4 commits November 8, 2022 20:30

fix: Patch AnimatedStyle to avoid discarding the initial style info

8172721

chore: Update AnimatedStyle __getValue to return an array of objects

8d4717e

chore: Fix unit tests

986a0c2

chore: Add Platform.OS check to AnimatedStyle

b45837a

gabrieldonadel force-pushed the fix/patch-animated-style branch from 92af6c4 to b45837a Compare November 8, 2022 23:30

necolas reviewed Nov 9, 2022

View reviewed changes

Libraries/Animated/__tests__/Animated-web-test.js Outdated Show resolved Hide resolved

Libraries/Animated/nodes/AnimatedStyle.js Outdated Show resolved Hide resolved

Libraries/Animated/nodes/AnimatedStyle.js Outdated Show resolved Hide resolved

chore: Update AnimatedStyle and Animated tests

45e8c02

gabrieldonadel force-pushed the fix/patch-animated-style branch from 6999082 to 45e8c02 Compare November 11, 2022 03:43

gabrieldonadel requested a review from necolas November 11, 2022 03:43

necolas approved these changes Nov 14, 2022

View reviewed changes

chore: Remove unnecessary mock from test

5b6fede

chore: Update Animated web tests

650c45e

gabrieldonadel requested a review from javache November 17, 2022 04:56

javache approved these changes Nov 17, 2022

View reviewed changes

Libraries/Animated/nodes/AnimatedStyle.js Outdated Show resolved Hide resolved

chore: Update AnimatedStyle constructor

11daeda

facebook-github-bot closed this in c06323f Nov 18, 2022

gabrieldonadel deleted the fix/patch-animated-style branch November 18, 2022 15:44

gabrieldonadel mentioned this pull request Oct 24, 2023

Fix createAnimatedStyle when providing undefined transform style #41176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Patch AnimatedStyle to avoid discarding the initial style info #35198

fix: Patch AnimatedStyle to avoid discarding the initial style info #35198

gabrieldonadel commented Nov 4, 2022

analysis-bot commented Nov 4, 2022 •

edited

Loading

analysis-bot commented Nov 4, 2022 •

edited

Loading

pull-bot commented Nov 4, 2022

javache commented Nov 4, 2022

gabrieldonadel commented Nov 4, 2022

necolas Nov 4, 2022 •

edited

Loading

necolas Nov 4, 2022

necolas commented Nov 4, 2022

pull-bot commented Nov 8, 2022

pull-bot commented Nov 8, 2022

pull-bot commented Nov 9, 2022

javache commented Nov 10, 2022

necolas commented Nov 10, 2022

pull-bot commented Nov 11, 2022

gabrieldonadel commented Nov 12, 2022

necolas Nov 14, 2022

gabrieldonadel Nov 17, 2022

pull-bot commented Nov 16, 2022

pull-bot commented Nov 17, 2022

pull-bot commented Nov 17, 2022

gabrieldonadel commented Nov 17, 2022

facebook-github-bot commented Nov 17, 2022

fix: Patch AnimatedStyle to avoid discarding the initial style info #35198

fix: Patch AnimatedStyle to avoid discarding the initial style info #35198

Conversation

gabrieldonadel commented Nov 4, 2022

Summary

Changelog

Test Plan

analysis-bot commented Nov 4, 2022 • edited Loading

analysis-bot commented Nov 4, 2022 • edited Loading

pull-bot commented Nov 4, 2022

javache commented Nov 4, 2022

gabrieldonadel commented Nov 4, 2022

necolas Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

necolas Nov 4, 2022

Choose a reason for hiding this comment

necolas commented Nov 4, 2022

pull-bot commented Nov 8, 2022

pull-bot commented Nov 8, 2022

pull-bot commented Nov 9, 2022

javache commented Nov 10, 2022

necolas commented Nov 10, 2022

pull-bot commented Nov 11, 2022

gabrieldonadel commented Nov 12, 2022

necolas Nov 14, 2022

Choose a reason for hiding this comment

gabrieldonadel Nov 17, 2022

Choose a reason for hiding this comment

pull-bot commented Nov 16, 2022

pull-bot commented Nov 17, 2022

pull-bot commented Nov 17, 2022

gabrieldonadel commented Nov 17, 2022

facebook-github-bot commented Nov 17, 2022

analysis-bot commented Nov 4, 2022 •

edited

Loading

analysis-bot commented Nov 4, 2022 •

edited

Loading

necolas Nov 4, 2022 •

edited

Loading