Fix crash animated

[riteshshukla04]

Summary:

Fixes https://github.com/facebook/react-native/issues/51395 .

Changelog:

[GENERAL][FIXED]Crash when Animated value & primitive value used together

Test Plan:

Can be tested on RNTester with

const myAnimatedValue = useAnimatedValue(100);
  return (
    <View style={styles.main}>
      <Animated.View style={[
        {width: 100, height: 100, backgroundColor: 'red'},
        {
          transform: [
            {translateX: myAnimatedValue},
          ],
        },
        {
          transform: [
            {translateY: 100},
          ],
        },

       ]} />
    </View>
  );

[riteshshukla04]

When an array of styles are passed its get flattened here https://github.com/facebook/react-native/blob/f1697544cd720df21bd7dc8ca993d95a41321e3f/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js#L95-L97

Now if array with animated object keys is passed to flattened style

[
        {width: 100, height: 100, backgroundColor: 'red'},
        {
          transform: [
            {translateX: myAnimatedValue2},
          ],
        },
        {
          transform: [
            {translateY: 100},
          ],
        },

       ]

The result is below

{
    "width": 100,
    "height": 100,
    "backgroundColor": "red",
    "transform": [
        {
            "translateY": 100
        }
    ]
}

It can be seen that {translateX: myAnimatedValue2} disappears completely . Now when this flattened object is passed to https://github.com/facebook/react-native/blob/f1697544cd720df21bd7dc8ca993d95a41321e3f/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js#L100-L106 We don't recieve any animated nodes cause it doesn't exist in flattened array causing it to return null.

These tricks it into passing an style array(before flattening) with animated values to View component which causes this crash.
My fix here is to flatten style everytime if it is an array in animated component itself

[facebook-github-bot]

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

[riteshshukla04]

Thanks so much for the review, @javache! 🙏 . I'm a bit unsure about how to write a reliable Jest test case for this. I tried the following:

const tree =  await render.create(
        <Animated.View
          style={[
            {
              backgroundColor: 'red',
              width: 100,
              height: 100,
            },
            {transform: [{translateX: new Animated.Value(0)}]},
            [{transform: [{translateY: 100}]}],
          ]}
        />
         
      );
      const root = tree.toJSON();

      expect(root).toMatchSnapshot();
      

However, it seems to pass even in scenarios where the component crashes on device. Should I add an example in RNTester instead?

Also we need to update the snapshot for as the current fix involves flattening style arrays passed to Animated. The snapshot for LogBoxInspectorSourceMapStatus still expects a nested style array, which no longer reflects the updated structure.

[javache]

Instead of flattening, let's make sure all props passed through have values applied.

You won't see this in jest, since processTransform is only called at the mounting layer. You'd need to validate there's no more Animated.Value in the final props.

[riteshshukla04]

Thanks so much for the response, @javache! 🙏

You're absolutely right — though from what I understand, validating all the props might not be very helpful in this case, since they’ll likely be overwritten anyway during the later stages when styles are further flattened (especially in the cases that are currently crashing).

Also, to properly apply all the style values when props are passed, it seems we would need to modify createAnimatedStyle itself to support working with style arrays directly, rather than flattened style object.

https://github.com/facebook/react-native/blob/f1697544cd720df21bd7dc8ca993d95a41321e3f/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js#L96-L104

Please do correct me if I’ve misunderstood anything — happy to adjust the approach accordingly!

[riteshshukla04]

Instead of flattening, let's make sure all props passed through have values applied.

You won't see this in jest, since processTransform is only called at the mounting layer. You'd need to validate there's no more Animated.Value in the final props.

Hey @javache , I was thinking about this yesterday night and now I think we can do this too . Something like below will apply all the values and still preserve the array.

if (key === 'style' && Array.isArray(maybeNode)) {
       props[key] = maybeNode.map(style=>{
        const node = AnimatedStyle.from(style, undefined);
        if(node instanceof AnimatedStyle){
          return  node.__getValueWithStaticStyle(style);
        }
        return style;
       })
      }

Should we do something like this ? Whats your opinion?

[javache]

I think the fix you're looking for needs to be in getValueWithStaticStyle (https://github.com/facebook/react-native/blob/6399caef635b6aadc4c98ec37c9f007f81fa1f79/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js#L150)

[javache]

It's not super clear why we are flattening props there when we can see that in the end a series of unflattened props are passed through to the host component - that's incorrect.

[riteshshukla04]

Hey @javache , __getValueWithStaticStyle wont be available unless node is an instance of Animated Style (which here is not as the animated values disappear after flattening) . I can confirm that if we find a animated node then flattened props are passed to host component else non flattened. The fix I was thinking here is either converting all the props to flattened or go through each element in style array and make the animated values apply before sending it to host component

[javache]

__getValueWithStaticStyle wont be available unless node is an instance of Animated Style (which here is not as the animated values disappear after flattening) .

Good catch. Perhaps that's a bug that should be fixed here first? If there's an animated value in the unflattened style we should still allocate the Animated.Style.

cc @yungsters

[riteshshukla04]

https://github.com/facebook/react-native/blob/f1697544cd720df21bd7dc8ca993d95a41321e3f/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js#L25-L30 I believe we may need to rewrite the function (and more support functions like __getValueWithStaticTransforms) to support non-flattened styles. However, I’m not entirely sure if addressing this edge case is worth the added complexity.

[riteshshukla04]

I have updated this with new logic where I am not flattening the array but processing all the animated values (if found) . This should fix the issue without need for flattening before sending it to host component.
cc:- @javache

[javache]

This needs a unit test to demonstrate the fix.

Wouldn't a better place for this fix be inside AnimatedStyle? Maybe removing just this line? https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Animated/nodes/AnimatedStyle.js#L105

[riteshshukla04]

Thanks for the reply @javache . I initially tried removing that line but that makes style array flattened every time before sending to host component . Also some test cases like these fail on that .

it('returns original `style` if it has no nodes', () => {
    const style = {color: 'red'};
    expect(getValue({style}).style).toBe(style);
  });

Should we go ahead with the suggested approach?

[riteshshukla04]

@javache Can you please check if this can be merged . I have added test cases

[javache]

I'll import this for the test-case - I think we can find a more generic solution.

[facebook-github-bot]

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

[yungsters]

Thanks for investigating this, sharing a detailed explanation of what you've found, and contributing a unit test case.

I took a look and put up a different fix that I think will have better performance and will be a bit more intuitive for maintainers: https://github.com/facebook/react-native/pull/51719

I verified that the unit test you contributed passes with it. If you have a chance, can you try it out to see if it addresses all of the problems you encountered?

[riteshshukla04]

Thanks @yungsters . Your fix looks better . Closing this