Set collapse property before starting an animation that uses the native driver

[cabelitos]

Summary

Depending on the style props of an Animated.View it may be optimised away
by the NativeViewHierarchyOptimizer, which will make the animation to
fail, because the native view is virtual (it does not exists
in the native view hierarchy).
Although the createAnimatedComponent already sets the collapsable property
based on the this._propsAnimated.__isNative flag, it won't work on all
cases, since the __isNative flag is only set when one starts the animation.
Which won't cause a re-render to occuor, thus not setting the collapsable
property to false.
In order to prevent this issue the HOC will just set the collapsable property
to false.

Changelog

[Javascript] [Fixed] - Properly set collapsable to false before starting a nativeDriver animation

Test Plan

Without this patch:

Run the following App on an Android device without this patch and click start.
Outcome: The animation will not make the text invisible.

With this patch:

Run the following App on an Android device with this patch and click start.
Outcome: The animation will make the text invisible.

import React, { Component, ReactNode } from 'react';
import { View, Text, TouchableOpacity, Animated, StyleSheet, Easing } from 'react-native';

interface Props { }

const Constants = {
  animation: {
    duration: 500,
  },
};

const text =
  'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nulla sed orci erat. Suspendisse feugiat elit gravida elit consequat ultrices. Sed sollicitudin ullamcorper molestie. Mauris a diam neque. Vivamus in lectus.';

class App extends Component<Props> {
  anim: any;

  constructor(props: Props) {
    super(props);
    this.anim = new Animated.Value(0);
  }

  handleStartPress = () => {
    this.anim.setValue(0);
    console.log('start');
    Animated.timing(this.anim, {
      duration: Constants.animation.duration,
      toValue: 1,
      easing: Easing.linear(),
      useNativeDriver: true,
    }).start();
  };

  render(): ReactNode {
    return (
      <View style={styles.container}>
        <Animated.View
          style={{
            opacity: this.anim.interpolate({
              inputRange: [0, 1],
              outputRange: [1, 0],
            }),
          }}>
          <Text>{text}</Text>
        </Animated.View>
        <TouchableOpacity
          style={styles.startButton}
          onPress={this.handleStartPress}>
          <Text style={styles.startButtonText}>START</Text>
        </TouchableOpacity>
      </View>
    );
  }
}


const styles = StyleSheet.create({
  container: {
    alignItems: 'center',
    backgroundColor: 'white',
    flex: 1,
  },
  description: {
    marginTop: 20,
    paddingHorizontal: 10,
  },
  startButton: {
    alignItems: 'center',
    aspectRatio: 1,
    backgroundColor: 'yellow',
    borderRadius: 100,
    height: 50,
    justifyContent: 'center',
  },
  startButtonText: {
    fontSize: 10,
    fontWeight: 'bold',
  },
});

export default App;

Closes #25318

[janicduplessis]

@cabelitos I think we can make this simpler and just always set collapsable to true instead of checking __isNative

react-native/Libraries/Animated/src/createAnimatedComponent.js

Line 159 in da903c1

collapsable={

.

Animated views should never really be collapsable since they will end up with some props that make them non-collapsable when an animation starts.

[cabelitos]

Hello and thanks for you review @janicduplessis.

At first I thought about harding coding the collapsable to false which avoids this NativeViewHierarchyOptimizer optimization. However I did not want to break the existing behavior or making performance worse in some cases. But I'm happy to just hard code the collapsable to false if you guys prefer.

[janicduplessis]

I don't think it will have a perf impact since usually animated views will have a non-collapsable prop. Also there will be an overhead anyway when the animation starts when the view gets marked as non-collapsable and we have to update the view again.

[cabelitos]

@janicduplessis ok then. I'll update the PR hardcoding the collapsable prop. Thank you!

[cabelitos]

@janicduplessis updated the PR, please let me know what do you think.
thank you.

[cpojer]

👍

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cpojer]

Just want to double check here: This changes collapsible to always be false but @janicduplessis was saying in his original comment that we should set it to true. I'm a bit confused here, please help.

[janicduplessis]

@cpojer Oups my bad I meant false

[cabelitos]

Hey guys, thank you for the approval.
@cpojer about the collapsible being hardcoded to false. If we set it to true the NativeViewHierarchyOptimizer would not create the backing native view, which would cause the native to won't work with the native driver. So I assumed that @janicduplessis did a typo in his comment and he meant to say false and not true.

Thank you very much for the support!

[react-native-bot]

This pull request was successfully merged by @cabelitos in df50eee.

^{When will my fix make it into a release? | Upcoming Releases}

[cpojer]

Unfortunately this did cause issues in production on Android where things didn't appear properly so I had to revert this change for now :(

[sintylapse]

@cpojer could you please specify in which circumstances this fix cause issues? maybe not everyone will have those issues but animation will be fixed for them

[cabelitos]

Good morning @cpojer,

if possible could you please share what's the problem and how I can trigger it. I'm glad to send a fix for the problem that is affecting you.

Thank you very much and sorry for any problem that I have caused.

[vdmtrv]

@cpojer as @cabelitos has mentioned, can you elaborate on the problem, please? This still seems to be an issue on 0.60.4