microsoft / react-native-windows

A framework for building native Windows apps with React.
https://microsoft.github.io/react-native-windows/
Other
16.38k stars 1.14k forks source link

Native driver persists animated values after they're no longer applied #3460

Open Thristhart opened 5 years ago

Thristhart commented 5 years ago

Environment

  1. react-native -v:
    react-native-cli: 2.0.1
    react-native: 0.60.6
  2. npm ls rnpm-plugin-windows:
    rnpm-plugin-windows@0.3.7
  3. npm ls react-native-windows:
    react-native-windows@0.60.0-vnext.37
  4. node -v:
    v10.15.0
  5. npm -v:
    6.4.1

Then, specify:

Steps to Reproduce

  1. Pass a style to an Animated.View that contains a transform or opacity set to an Animated.Value
  2. Use Animated.timing to animate that Animated.Value to a new value, with useNativeDriver: true
  3. Update the Animated.View such that it no longer has the Animated.Value in its styles

Expected Behavior

The transform or opacity would reset to initial values. A View translated to the right previously would have that translation removed.

Actual Behavior

The transform or opacity remain applied, even though the style no longer contains those values.

Reproducible Demo

image image image

import React from "react";
import { Animated, Easing, Text, TouchableOpacity } from "react-native";

export default class AnimationPersistRepro extends React.Component {
    state = { animated: false };
    translateXAnimated = new Animated.Value(0);
    onPress = () => {
        this.setState((state) => {
            const shouldAnimate = !state.animated;
            if (shouldAnimate) {
                Animated.timing(this.translateXAnimated, {
                    duration: 400,
                    toValue: 100,
                    easing: Easing.linear,
                    useNativeDriver: true,
                }).start();
            }
            return { animated: shouldAnimate };
        });
    };
    render() {
        const animatedStyle = {
            transform: [{ translateX: this.translateXAnimated }],
        };
        const nonAnimatedStyle = {
            transform: [],
        };
        return (
            <>
                <Animated.View style={this.state.animated ? animatedStyle : nonAnimatedStyle}>
                    <Text>Animates to the right</Text>
                </Animated.View>
                <TouchableOpacity onPress={this.onPress}>
                    <Text>Press to toggle animation. Currently animated: {this.state.animated.toString()} </Text>
                </TouchableOpacity>
            </>
        );
    }
}

----alternative repro as a function component-----

const AnimationPersistRepro = () => {
  const [isAnimated, setAnimated] = useState(false);
  const translateXAnimated = useRef(new Animated.Value(0));

  const animatedStyle = {
    transform: [ { translateX: translateXAnimated.current } ]
  };
  const nonAnimatedStyle = {
    transform: []
  };

  const onPress = () => {
    setAnimated(animated => {
      const shouldAnimate = !animated;
      if(shouldAnimate) {
        Animated.timing(translateXAnimated.current, {
          duration: 400,
          toValue: 100,
          easing: Easing.linear,
          useNativeDriver: true
        }).start();
      }
      return shouldAnimate;
    });
  };

  return (
    <>
      <Animated.View style={isAnimated ? animatedStyle : nonAnimatedStyle}>
        <Text>Animates to the right</Text>
      </Animated.View>
      <TouchableOpacity onPress={onPress}>
        <Text>Press to toggle animation. Currently animated: {isAnimated.toString()} </Text>
      </TouchableOpacity>
    </>
  );
};
chrisglein commented 5 years ago

Interesting scenario. It's not immediately clear whether this is or is not expected behavior, as in what sort of memory do animations have. First step would be to try this on Android/iOS to clarify that expected behavior.

kmelmon commented 4 years ago

Confirmed in snack this isn't expected behavior for other platforms.

Thristhart commented 4 years ago

I assume this is related to #3280, given that #3281 comments out target.StopAnimation()

chrisglein commented 4 years ago

Comment from @marlenecota:

Debugged issue #3460 where an animated View is not reset after the animation is removed. There’s a lot of Animated code, so it’s going to take a little time to figure out how they all work together, but it looks like there’s an unimplemented “ResetDefaultValues” function in PropsAnimatedNode that needs to be called when it is disconnected from the View. Unfortunately the original value is not stored anywhere, so I am trying to figure out where it would go. I spoke with the Xbox guys and they have a workaround for this, so it is not currently blocking and are ok waiting for a fix.

Given presence of workaround, moving this to M5.

rozele commented 2 years ago

This also blocks use of native animations in some scenarios for us as well.

rozele commented 2 years ago

Here's another repro for this: https://github.com/rozele/react-native-windows/tree/repro3460

Here's the "expected" behavior using the JS animation driver:

https://user-images.githubusercontent.com/1106239/149194070-bfb66137-07c1-484d-97da-e00963d45c31.mp4

Here's the "actual" behavior using the composition native animation driver:

https://user-images.githubusercontent.com/1106239/149194090-278cb9c9-ae1b-42bd-8aa2-3ab52f13a7eb.mp4

Notice the translateX value never resets to 0.

rozele commented 2 years ago

Re-opening, as this is technically still a bug for the composition-based NativeAnimated implementation and the tick-based implementation is opt-in.

For a temporary workaround use:

// Animated.timing is an example, same syntax for any animation driver
Animated.timing({
  /* ... */,
  useNativeDriver: true,
  platformConfig: {
    useComposition: false,
  }
);