From dccb57fb50874e31dd0d3f6e39666e4a5b9a079d Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Tue, 29 Nov 2022 07:52:46 -0800 Subject: [PATCH] Fix Animated.Color callbacks when switching between native colors Summary: Animated.Color was never calling `flush`, and thus didn't trigger any animated views to update when the Animated.Color had (a native-only) color change when using the JS driver. Changelog: [General][Fixed] Improved handling of native colors in Animated.Colors Reviewed By: mdvacca Differential Revision: D41519965 fbshipit-source-id: 52f78460cf67ab9260d3868b0d27692b64fc3c58 --- Libraries/Animated/__tests__/Animated-test.js | 39 +++++- Libraries/Animated/nodes/AnimatedColor.js | 127 +++++++----------- Libraries/Animated/nodes/AnimatedNode.js | 3 +- Libraries/Animated/nodes/AnimatedValue.js | 20 ++- .../Animated/nodes/AnimatedWithChildren.js | 2 +- 5 files changed, 92 insertions(+), 99 deletions(-) diff --git a/Libraries/Animated/__tests__/Animated-test.js b/Libraries/Animated/__tests__/Animated-test.js index d02ffa8aad1f8d..eca451af1ba4dc 100644 --- a/Libraries/Animated/__tests__/Animated-test.js +++ b/Libraries/Animated/__tests__/Animated-test.js @@ -10,9 +10,10 @@ import * as React from 'react'; +const {PlatformColor} = require('../../StyleSheet/PlatformColorValueTypes'); let Animated = require('../Animated').default; -let AnimatedProps = require('../nodes/AnimatedProps').default; -let TestRenderer = require('react-test-renderer'); +const AnimatedProps = require('../nodes/AnimatedProps').default; +const TestRenderer = require('react-test-renderer'); jest.mock('../../BatchedBridge/NativeModules', () => ({ NativeAnimatedModule: {}, @@ -1039,10 +1040,10 @@ describe('Animated tests', () => { }); node.__attach(); - expect(callback.mock.calls.length).toBe(0); + expect(callback).not.toHaveBeenCalled(); color.setValue({r: 11, g: 22, b: 33, a: 0.5}); - expect(callback.mock.calls.length).toBe(4); + expect(callback).toHaveBeenCalledTimes(5); expect(node.__getValue()).toEqual({ style: { backgroundColor: 'rgba(11, 22, 33, 0.5)', @@ -1052,7 +1053,7 @@ describe('Animated tests', () => { node.__detach(); color.setValue({r: 255, g: 0, b: 0, a: 1.0}); - expect(callback.mock.calls.length).toBe(4); + expect(callback).toHaveBeenCalledTimes(5); }); it('should track colors', () => { @@ -1088,5 +1089,33 @@ describe('Animated tests', () => { jest.runAllTimers(); expect(color2.__getValue()).toEqual('rgba(44, 55, 66, 0)'); }); + + it('should provide updates for native colors', () => { + const color = new Animated.Color('red'); + + const listener = jest.fn(); + color.addListener(listener); + + const callback = jest.fn(() => { + console.log('callback', color.__getValue()); + }); + const node = new AnimatedProps({style: {color}}, callback); + node.__attach(); + + color.setValue('blue'); + expect(callback).toHaveBeenCalledTimes(5); + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith({value: 'rgba(0, 0, 255, 1)'}); + expect(color.__getValue()).toEqual('rgba(0, 0, 255, 1)'); + + callback.mockClear(); + listener.mockClear(); + + color.setValue(PlatformColor('bar')); + expect(callback).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledTimes(1); + expect(listener).toHaveBeenCalledWith({value: PlatformColor('bar')}); + expect(color.__getValue()).toEqual(PlatformColor('bar')); + }); }); }); diff --git a/Libraries/Animated/nodes/AnimatedColor.js b/Libraries/Animated/nodes/AnimatedColor.js index d6fba11337f76b..389a4a82ffe009 100644 --- a/Libraries/Animated/nodes/AnimatedColor.js +++ b/Libraries/Animated/nodes/AnimatedColor.js @@ -18,7 +18,7 @@ import type {PlatformConfig} from '../AnimatedPlatformConfig'; import normalizeColor from '../../StyleSheet/normalizeColor'; import {processColorObject} from '../../StyleSheet/PlatformColorValueTypes'; import NativeAnimatedHelper from '../NativeAnimatedHelper'; -import AnimatedValue from './AnimatedValue'; +import AnimatedValue, {flushValue} from './AnimatedValue'; import AnimatedWithChildren from './AnimatedWithChildren'; export type AnimatedColorConfig = $ReadOnly<{ @@ -43,10 +43,11 @@ type RgbaAnimatedValue = { ... }; +export type InputValue = ?(RgbaValue | RgbaAnimatedValue | ColorValue); + const NativeAnimatedAPI = NativeAnimatedHelper.API; const defaultColor: RgbaValue = {r: 0, g: 0, b: 0, a: 1.0}; -let _uniqueId = 1; /* eslint no-bitwise: 0 */ function processColor( @@ -113,22 +114,12 @@ export default class AnimatedColor extends AnimatedWithChildren { b: AnimatedValue; a: AnimatedValue; nativeColor: ?NativeColorValue; - _listeners: { - [key: string]: { - r: string, - g: string, - b: string, - a: string, - ... - }, - ... - } = {}; - - constructor( - valueIn?: ?(RgbaValue | RgbaAnimatedValue | ColorValue), - config?: ?AnimatedColorConfig, - ) { + + _suspendCallbacks: number = 0; + + constructor(valueIn?: InputValue, config?: ?AnimatedColorConfig) { super(); + let value: RgbaValue | RgbaAnimatedValue | ColorValue = valueIn ?? defaultColor; if (isRgbaAnimatedValue(value)) { @@ -156,7 +147,8 @@ export default class AnimatedColor extends AnimatedWithChildren { this.b = new AnimatedValue(initColor.b); this.a = new AnimatedValue(initColor.a); } - if (this.nativeColor || (config && config.useNativeDriver)) { + + if (config?.useNativeDriver) { this.__makeNative(); } } @@ -174,25 +166,27 @@ export default class AnimatedColor extends AnimatedWithChildren { const processedColor: RgbaValue | NativeColorValue = processColor(value) ?? defaultColor; - if (isRgbaValue(processedColor)) { - // $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-type] - Type is verified above - const nativeColor: NativeColorValue = processedColor; - if (this.nativeColor !== nativeColor) { - this.nativeColor = nativeColor; - shouldUpdateNodeConfig = true; + this._withSuspendedCallbacks(() => { + if (isRgbaValue(processedColor)) { + // $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-type] - Type is verified above + const nativeColor: NativeColorValue = processedColor; + if (this.nativeColor !== nativeColor) { + this.nativeColor = nativeColor; + shouldUpdateNodeConfig = true; + } } - } + }); if (this.__isNative) { const nativeTag = this.__getNativeTag(); @@ -203,7 +197,12 @@ export default class AnimatedColor extends AnimatedWithChildren { ); } NativeAnimatedAPI.unsetWaitingForIdentifier(nativeTag.toString()); + } else { + flushValue(this); } + + // $FlowFixMe[incompatible-call] + this.__callListeners(this.__getValue()); } /** @@ -240,50 +239,6 @@ export default class AnimatedColor extends AnimatedWithChildren { this.a.extractOffset(); } - /** - * Adds an asynchronous listener to the value so you can observe updates from - * animations. This is useful because there is no way to synchronously read - * the value because it might be driven natively. - * - * Returns a string that serves as an identifier for the listener. - */ - addListener(callback: ColorListenerCallback): string { - const id = String(_uniqueId++); - const jointCallback = ({value: number}: any) => { - callback(this.__getValue()); - }; - this._listeners[id] = { - r: this.r.addListener(jointCallback), - g: this.g.addListener(jointCallback), - b: this.b.addListener(jointCallback), - a: this.a.addListener(jointCallback), - }; - return id; - } - - /** - * Unregister a listener. The `id` param shall match the identifier - * previously returned by `addListener()`. - */ - removeListener(id: string): void { - this.r.removeListener(this._listeners[id].r); - this.g.removeListener(this._listeners[id].g); - this.b.removeListener(this._listeners[id].b); - this.a.removeListener(this._listeners[id].a); - delete this._listeners[id]; - } - - /** - * Remove all registered listeners. - */ - removeAllListeners(): void { - this.r.removeAllListeners(); - this.g.removeAllListeners(); - this.b.removeAllListeners(); - this.a.removeAllListeners(); - this._listeners = {}; - } - /** * Stops any running animation or tracking. `callback` is invoked with the * final value after stopping the animation, which is useful for updating @@ -332,6 +287,18 @@ export default class AnimatedColor extends AnimatedWithChildren { super.__detach(); } + _withSuspendedCallbacks(callback: () => void) { + this._suspendCallbacks++; + callback(); + this._suspendCallbacks--; + } + + __callListeners(value: number): void { + if (this._suspendCallbacks === 0) { + super.__callListeners(value); + } + } + __makeNative(platformConfig: ?PlatformConfig) { this.r.__makeNative(platformConfig); this.g.__makeNative(platformConfig); diff --git a/Libraries/Animated/nodes/AnimatedNode.js b/Libraries/Animated/nodes/AnimatedNode.js index a77bba2db42549..a4fadb5ab7ecf3 100644 --- a/Libraries/Animated/nodes/AnimatedNode.js +++ b/Libraries/Animated/nodes/AnimatedNode.js @@ -42,7 +42,7 @@ export default class AnimatedNode { } __addChild(child: AnimatedNode) {} __removeChild(child: AnimatedNode) {} - __getChildren(): Array { + __getChildren(): $ReadOnlyArray { return []; } @@ -184,6 +184,7 @@ export default class AnimatedNode { 'This JS animated node type cannot be used as native animated node', ); } + toJSON(): any { return this.__getValue(); } diff --git a/Libraries/Animated/nodes/AnimatedValue.js b/Libraries/Animated/nodes/AnimatedValue.js index f20307b310e26e..207dbf69c6e12b 100644 --- a/Libraries/Animated/nodes/AnimatedValue.js +++ b/Libraries/Animated/nodes/AnimatedValue.js @@ -48,21 +48,18 @@ const NativeAnimatedAPI = NativeAnimatedHelper.API; * this two-phases process is to deal with composite props such as * transform which can receive values from multiple parents. */ -function _flush(rootNode: AnimatedValue): void { - const animatedStyles = new Set(); - function findAnimatedStyles(node: AnimatedValue | AnimatedNode) { - /* $FlowFixMe[prop-missing] (>=0.68.0 site=react_native_fb) This comment - * suppresses an error found when Flow v0.68 was deployed. To see the error - * delete this comment and run Flow. */ +export function flushValue(rootNode: AnimatedNode): void { + const leaves = new Set<{update: () => void, ...}>(); + function findAnimatedStyles(node: AnimatedNode) { + // $FlowFixMe[prop-missing] if (typeof node.update === 'function') { - animatedStyles.add(node); + leaves.add((node: any)); } else { node.__getChildren().forEach(findAnimatedStyles); } } findAnimatedStyles(rootNode); - // $FlowFixMe[prop-missing] - animatedStyles.forEach(animatedStyle => animatedStyle.update()); + leaves.forEach(leaf => leaf.update()); } /** @@ -91,7 +88,6 @@ export default class AnimatedValue extends AnimatedWithChildren { _animation: ?Animation; _tracking: ?AnimatedTracking; - // $FlowFixMe[missing-local-annot] constructor(value: number, config?: ?AnimatedValueConfig) { super(); if (typeof value !== 'number') { @@ -291,9 +287,9 @@ export default class AnimatedValue extends AnimatedWithChildren { this._value = value; if (flush) { - _flush(this); + flushValue(this); } - super.__callListeners(this.__getValue()); + this.__callListeners(this.__getValue()); } __getNativeConfig(): Object { diff --git a/Libraries/Animated/nodes/AnimatedWithChildren.js b/Libraries/Animated/nodes/AnimatedWithChildren.js index ac4ade64612f9d..93d88497648632 100644 --- a/Libraries/Animated/nodes/AnimatedWithChildren.js +++ b/Libraries/Animated/nodes/AnimatedWithChildren.js @@ -70,7 +70,7 @@ export default class AnimatedWithChildren extends AnimatedNode { } } - __getChildren(): Array { + __getChildren(): $ReadOnlyArray { return this._children; }