From e405e84fc35923888442df748757787698040010 Mon Sep 17 00:00:00 2001 From: scisci Date: Thu, 31 Jan 2019 01:33:38 -0800 Subject: [PATCH] Fix Native Rotation Android (#18872) Summary: Fixes #14161 Android crashes in some cases if an animated transform config contains a string value, like a rotation. This PR fixes that by ensuring all values sent to the native side are doubles. It adds `__transformDataType` to AnimatedTransform.js. Added integration test `ReactAndroid/src/androidText/js/AnimatedTransformTestModule.js` This test fails with the following error `INSTRUMENTATION_RESULT: longMsg=java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Double`, if the changes to AnimatedTransform.js are reverted. [Android] [Fixed] - Fixes Android crash on animated style with string rotation Pull Request resolved: https://github.com/facebook/react-native/pull/18872 Differential Revision: D13894676 Pulled By: cpojer fbshipit-source-id: 297e8132563460802e53f3ac551c3ba9ed943736 --- .../Animated/src/NativeAnimatedHelper.js | 17 ++++ .../src/nodes/AnimatedInterpolation.js | 16 +--- .../Animated/src/nodes/AnimatedTransform.js | 2 +- .../react/tests/AnimatedTransformTest.java | 55 +++++++++++++ .../js/AnimatedTransformTestModule.js | 81 +++++++++++++++++++ ReactAndroid/src/androidTest/js/TestBundle.js | 5 ++ 6 files changed, 160 insertions(+), 16 deletions(-) create mode 100644 ReactAndroid/src/androidTest/java/com/facebook/react/tests/AnimatedTransformTest.java create mode 100644 ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js diff --git a/Libraries/Animated/src/NativeAnimatedHelper.js b/Libraries/Animated/src/NativeAnimatedHelper.js index 79e76db4cf3d5c..2ac39f024ab63a 100644 --- a/Libraries/Animated/src/NativeAnimatedHelper.js +++ b/Libraries/Animated/src/NativeAnimatedHelper.js @@ -260,6 +260,22 @@ function shouldUseNativeDriver(config: AnimationConfig | EventConfig): boolean { return config.useNativeDriver || false; } +function transformDataType(value: any): number { + // Change the string type to number type so we can reuse the same logic in + // iOS and Android platform + if (typeof value !== 'string') { + return value; + } + if (/deg$/.test(value)) { + const degrees = parseFloat(value) || 0; + const radians = (degrees * Math.PI) / 180.0; + return radians; + } else { + // Assume radians + return parseFloat(value) || 0; + } +} + module.exports = { API, addWhitelistedStyleProp, @@ -272,6 +288,7 @@ module.exports = { generateNewAnimationId, assertNativeAnimatedModule, shouldUseNativeDriver, + transformDataType, get nativeEventEmitter() { if (!nativeEventEmitter) { nativeEventEmitter = new NativeEventEmitter(NativeAnimatedModule); diff --git a/Libraries/Animated/src/nodes/AnimatedInterpolation.js b/Libraries/Animated/src/nodes/AnimatedInterpolation.js index d426a2931defe2..1ba304d331ad5f 100644 --- a/Libraries/Animated/src/nodes/AnimatedInterpolation.js +++ b/Libraries/Animated/src/nodes/AnimatedInterpolation.js @@ -347,21 +347,7 @@ class AnimatedInterpolation extends AnimatedWithChildren { } __transformDataType(range: Array) { - // Change the string array type to number array - // So we can reuse the same logic in iOS and Android platform - return range.map(function(value) { - if (typeof value !== 'string') { - return value; - } - if (/deg$/.test(value)) { - const degrees = parseFloat(value) || 0; - const radians = (degrees * Math.PI) / 180.0; - return radians; - } else { - // Assume radians - return parseFloat(value) || 0; - } - }); + return range.map(NativeAnimatedHelper.transformDataType); } __getNativeConfig(): any { diff --git a/Libraries/Animated/src/nodes/AnimatedTransform.js b/Libraries/Animated/src/nodes/AnimatedTransform.js index 703fa51ceb4e1b..e3b3034675055c 100644 --- a/Libraries/Animated/src/nodes/AnimatedTransform.js +++ b/Libraries/Animated/src/nodes/AnimatedTransform.js @@ -103,7 +103,7 @@ class AnimatedTransform extends AnimatedWithChildren { transConfigs.push({ type: 'static', property: key, - value, + value: NativeAnimatedHelper.transformDataType(value), }); } } diff --git a/ReactAndroid/src/androidTest/java/com/facebook/react/tests/AnimatedTransformTest.java b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/AnimatedTransformTest.java new file mode 100644 index 00000000000000..f2ddc0d2e9ed37 --- /dev/null +++ b/ReactAndroid/src/androidTest/java/com/facebook/react/tests/AnimatedTransformTest.java @@ -0,0 +1,55 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.react.tests; + +import android.view.View; + + +import com.facebook.react.testing.ReactInstanceSpecForTest; +import com.facebook.react.testing.StringRecordingModule; +import com.facebook.react.bridge.JavaScriptModule; +import com.facebook.react.testing.ReactAppInstrumentationTestCase; +import com.facebook.react.testing.ReactTestHelper; + +/** + * Integration test for {@code removeClippedSubviews} property that verify correct scrollview + * behavior + */ +public class AnimatedTransformTest extends ReactAppInstrumentationTestCase { + + private StringRecordingModule mStringRecordingModule; + + @Override + protected String getReactApplicationKeyUnderTest() { + return "AnimatedTransformTestApp"; + } + + @Override + protected ReactInstanceSpecForTest createReactInstanceSpecForTest() { + mStringRecordingModule = new StringRecordingModule(); + return super.createReactInstanceSpecForTest() + .addNativeModule(mStringRecordingModule); + } + + public void testAnimatedRotation() { + waitForBridgeAndUIIdle(); + + View button = ReactTestHelper.getViewWithReactTestId( + getActivity().getRootView(), + "TouchableOpacity"); + + // Tap the button which triggers the animated transform containing the + // rotation strings. + createGestureGenerator().startGesture(button).endGesture(); + waitForBridgeAndUIIdle(); + + // The previous cast error will prevent it from getting here + assertEquals(2, mStringRecordingModule.getCalls().size()); + } + +} diff --git a/ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js b/ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js new file mode 100644 index 00000000000000..f5c9e863d31336 --- /dev/null +++ b/ReactAndroid/src/androidTest/js/AnimatedTransformTestModule.js @@ -0,0 +1,81 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @providesModule AnimatedTransformTestModule + */ + +'use strict'; + +var BatchedBridge = require('BatchedBridge'); +var React = require('React'); +var StyleSheet = require('StyleSheet'); +var View = require('View'); +var TouchableOpacity = require('TouchableOpacity'); +var Text = require('Text'); +var RecordingModule = require('NativeModules').Recording; + +const styles = StyleSheet.create({ + base: { + width: 200, + height: 200, + backgroundColor: 'red', + transform: [{rotate: '0deg'}], + }, + transformed: { + transform: [{rotate: '45deg'}], + }, +}); + +/** + * This app presents a TouchableOpacity which was the simplest way to + * demonstrate this issue. Tapping the TouchableOpacity causes an animated + * transform to be created for the rotation property. Since the property isn't + * animated itself, it comes through as a static property, but static properties + * can't currently handle strings which causes a string->double cast exception. + */ +class AnimatedTransformTestApp extends React.Component { + constructor(props) { + super(props); + this.toggle = this.toggle.bind(this); + } + + state = { + flag: false, + }; + + toggle() { + this.setState({ + flag: !this.state.flag, + }); + } + + render() { + // Using this to verify if its fixed. + RecordingModule.record('render'); + + return ( + + + TouchableOpacity + + + ); + } +} + +var AnimatedTransformTestModule = { + AnimatedTransformTestApp: AnimatedTransformTestApp, +}; + +BatchedBridge.registerCallableModule( + 'AnimatedTransformTestModule', + AnimatedTransformTestModule +); + +module.exports = AnimatedTransformTestModule; diff --git a/ReactAndroid/src/androidTest/js/TestBundle.js b/ReactAndroid/src/androidTest/js/TestBundle.js index 6fb617821b6754..6e910054742650 100644 --- a/ReactAndroid/src/androidTest/js/TestBundle.js +++ b/ReactAndroid/src/androidTest/js/TestBundle.js @@ -35,6 +35,11 @@ require('TimePickerDialogTestModule'); const AppRegistry = require('AppRegistry'); const apps = [ + { + appKey: 'AnimatedTransformTestApp', + component: () => + require('AnimatedTransformTestModule').AnimatedTransformTestApp, + }, { appKey: 'CatalystRootViewTestApp', component: () =>