From 406a474e52fa515fa60187f1a2ff5ede2a6fdcd0 Mon Sep 17 00:00:00 2001
From: Pieter De Baets <pieterdb@fb.com>
Date: Mon, 23 May 2022 07:57:42 -0700
Subject: [PATCH] Correctly batch AnimatedColor updates

Summary:
Call `setWaitingForIdentifier` before we do any `setValue` calls to make sure we execute them together (and only call start/finish batch once). Only calll `updateAnimatedNodeConfig` conditionally when we changed nativeColor.

Changelog: [Internal]

Reviewed By: JoshuaGross

Differential Revision: D36517302

fbshipit-source-id: ecbae2d1df69e4456620c58a922275406e22a2f8
---
 Libraries/Animated/nodes/AnimatedColor.js | 60 +++++++++++++++--------
 Libraries/Animated/nodes/AnimatedValue.js |  6 +--
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/Libraries/Animated/nodes/AnimatedColor.js b/Libraries/Animated/nodes/AnimatedColor.js
index 8901a97d0d7..8bb31844b22 100644
--- a/Libraries/Animated/nodes/AnimatedColor.js
+++ b/Libraries/Animated/nodes/AnimatedColor.js
@@ -24,7 +24,9 @@ import type {ProcessedColorValue} from '../../StyleSheet/processColor';
 export type AnimatedColorConfig = $ReadOnly<{
   useNativeDriver: boolean,
 }>;
-type ColorListenerCallback = (value: string) => mixed;
+
+type ColorListenerCallback = (value: ColorValue) => mixed;
+
 export type RgbaValue = {
   +r: number,
   +g: number,
@@ -32,6 +34,7 @@ export type RgbaValue = {
   +a: number,
   ...
 };
+
 type RgbaAnimatedValue = {
   +r: AnimatedValue,
   +g: AnimatedValue,
@@ -40,6 +43,8 @@ type RgbaAnimatedValue = {
   ...
 };
 
+const NativeAnimatedAPI = NativeAnimatedHelper.API;
+
 const defaultColor: RgbaValue = {r: 0, g: 0, b: 0, a: 1.0};
 let _uniqueId = 1;
 
@@ -161,34 +166,43 @@ export default class AnimatedColor extends AnimatedWithChildren {
    * and update all the bound properties.
    */
   setValue(value: RgbaValue | ColorValue): void {
-    this.nativeColor = null;
+    let shouldUpdateNodeConfig = false;
+    if (this.__isNative) {
+      const nativeTag = this.__getNativeTag();
+      NativeAnimatedAPI.setWaitingForIdentifier(nativeTag.toString());
+    }
 
     const processedColor: RgbaValue | NativeColorValue =
       processColor(value) ?? defaultColor;
     if (isRgbaValue(processedColor)) {
-      // $FlowIgnore[incompatible-cast] - Type is verified above
-      const rgbaValue: RgbaValue = (processedColor: RgbaValue);
+      // $FlowIgnore[incompatible-type] - Type is verified above
+      const rgbaValue: RgbaValue = processedColor;
       this.r.setValue(rgbaValue.r);
       this.g.setValue(rgbaValue.g);
       this.b.setValue(rgbaValue.b);
       this.a.setValue(rgbaValue.a);
+      if (this.nativeColor != null) {
+        this.nativeColor = null;
+        shouldUpdateNodeConfig = true;
+      }
     } else {
-      // $FlowIgnore[incompatible-cast] - Type is verified above
-      this.nativeColor = (processedColor: NativeColorValue);
+      // $FlowIgnore[incompatible-type] - Type is verified above
+      const nativeColor: NativeColorValue = processedColor;
+      if (this.nativeColor !== nativeColor) {
+        this.nativeColor = nativeColor;
+        shouldUpdateNodeConfig = true;
+      }
     }
 
-    if (this.nativeColor) {
-      if (!this.__isNative) {
-        this.__makeNative();
-      }
-
+    if (this.__isNative) {
       const nativeTag = this.__getNativeTag();
-      NativeAnimatedHelper.API.setWaitingForIdentifier(nativeTag.toString());
-      NativeAnimatedHelper.API.updateAnimatedNodeConfig(
-        nativeTag,
-        this.__getNativeConfig(),
-      );
-      NativeAnimatedHelper.API.unsetWaitingForIdentifier(nativeTag.toString());
+      if (shouldUpdateNodeConfig) {
+        NativeAnimatedAPI.updateAnimatedNodeConfig(
+          nativeTag,
+          this.__getNativeConfig(),
+        );
+      }
+      NativeAnimatedAPI.unsetWaitingForIdentifier(nativeTag.toString());
     }
   }
 
@@ -275,7 +289,7 @@ export default class AnimatedColor extends AnimatedWithChildren {
    * final value after stopping the animation, which is useful for updating
    * state to match the animation position with layout.
    */
-  stopAnimation(callback?: (value: string) => void): void {
+  stopAnimation(callback?: ColorListenerCallback): void {
     this.r.stopAnimation();
     this.g.stopAnimation();
     this.b.stopAnimation();
@@ -286,7 +300,7 @@ export default class AnimatedColor extends AnimatedWithChildren {
   /**
    * Stops any animation and resets the value to its original.
    */
-  resetAnimation(callback?: (value: string) => void): void {
+  resetAnimation(callback?: ColorListenerCallback): void {
     this.r.resetAnimation();
     this.g.resetAnimation();
     this.b.resetAnimation();
@@ -294,8 +308,12 @@ export default class AnimatedColor extends AnimatedWithChildren {
     callback && callback(this.__getValue());
   }
 
-  __getValue(): string {
-    return `rgba(${this.r.__getValue()}, ${this.g.__getValue()}, ${this.b.__getValue()}, ${this.a.__getValue()})`;
+  __getValue(): ColorValue {
+    if (this.nativeColor != null) {
+      return this.nativeColor;
+    } else {
+      return `rgba(${this.r.__getValue()}, ${this.g.__getValue()}, ${this.b.__getValue()}, ${this.a.__getValue()})`;
+    }
   }
 
   __attach(): void {
diff --git a/Libraries/Animated/nodes/AnimatedValue.js b/Libraries/Animated/nodes/AnimatedValue.js
index 0625c83687a..9530e090f92 100644
--- a/Libraries/Animated/nodes/AnimatedValue.js
+++ b/Libraries/Animated/nodes/AnimatedValue.js
@@ -134,9 +134,9 @@ class AnimatedValue extends AnimatedWithChildren {
       !this.__isNative /* don't perform a flush for natively driven values */,
     );
     if (this.__isNative) {
-      _executeAsAnimatedBatch(this.__getNativeTag().toString(), () => {
-        NativeAnimatedAPI.setAnimatedNodeValue(this.__getNativeTag(), value);
-      });
+      _executeAsAnimatedBatch(this.__getNativeTag().toString(), () =>
+        NativeAnimatedAPI.setAnimatedNodeValue(this.__getNativeTag(), value),
+      );
     }
   }