Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Animated.Value resets styles when detached on RN 0.70.0 #34665

Closed
j-piasecki opened this issue Sep 12, 2022 · 22 comments
Closed

Animated.Value resets styles when detached on RN 0.70.0 #34665

j-piasecki opened this issue Sep 12, 2022 · 22 comments
Assignees
Labels
API: Animated Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Platform: Android Android applications. Platform: iOS iOS applications. Stale There has been a lack of activity on this issue and it may be closed soon.

Comments

@j-piasecki
Copy link
Collaborator

Description

When Animated.Value in styles gets replaced by a constant value, the Animated.Node gets correctly detached, however, the new value is not assigned - the prop gets restored to its default value.
It only happens when using native drivers, when running animation with useNativeDriver: false it works correctly.

Behavior on React Native 0.70.0:

rn70.webm

Correct behavior on React Native 0.69.5:

rn69.webm

Related issue: software-mansion/react-native-gesture-handler#2208

Version

0.70.0

Output of npx react-native info

System:
    OS: macOS 12.3.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 109.94 MB / 16.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 18.7.0 - /opt/homebrew/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 8.15.0 - /opt/homebrew/bin/npm
    Watchman: 2022.08.08.00 - /opt/homebrew/bin/watchman
  Managers:
    CocoaPods: 1.11.3 - /opt/homebrew/lib/ruby/gems/3.0.0/bin/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 21.4, iOS 15.4, macOS 12.3, tvOS 15.4, watchOS 8.5
    Android SDK:
      API Levels: 24, 26, 28, 29, 30, 31, 32, 33
      Build Tools: 26.0.3, 28.0.3, 29.0.2, 30.0.2, 30.0.3, 31.0.0, 32.0.0, 32.1.0, 33.0.0
      System Images: android-28 | Google ARM64-V8a Play ARM 64 v8a, android-32 | Google APIs ARM 64 v8a
      Android NDK: Not Found
  IDEs:
    Android Studio: 2021.2 AI-212.5712.43.2112.8609683
    Xcode: 13.3.1/13E500a - /usr/bin/xcodebuild
  Languages:
    Java: 11.0.14 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: 18.1.0 => 18.1.0
    react-native: 0.70.0 => 0.70.0
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Steps to reproduce

Run the code below and press the Animate button. When the animation ends the view snaps to the edge of the screen instead of staying in place.

Snack, code example, screenshot, or link to a repository

import React, {useRef, useState} from 'react';
import {View, Animated, Button} from 'react-native';

export default function App() {
  const value = useRef(new Animated.Value(0)).current;
  const [useAnimatedValue, setUseAnimatedValue] = useState(false);

  function animate() {
    value.setValue(0);
    setUseAnimatedValue(true);
    Animated.spring(value, {
      toValue: 1,
      bounciness: 3000,
      velocity: 0.5,
      useNativeDriver: true,
    }).start(({finished}) => {
      if (finished) {
        setUseAnimatedValue(false);
      }
    });
  }

  const offset = useAnimatedValue
    ? value.interpolate({
        inputRange: [0, 1],
        outputRange: [0, 250],
        extrapolate: 'clamp',
      })
    : 250;

  return (
    <View style={{flex: 1}}>
      <Animated.View
        style={{
          width: 100,
          height: 100,
          backgroundColor: 'red',
          transform: [{translateX: offset}],
        }}
      />
      <Button
        title="Animate"
        onPress={() => {
          animate();
        }}
      />
    </View>
  );
}
@lunaleaps
Copy link
Contributor

lunaleaps commented Sep 26, 2022

To clarify is this an Android only issue? Ah it seems in software-mansion/react-native-gesture-handler#2208 it appears on both platforms

@lunaleaps lunaleaps added Platform: iOS iOS applications. Platform: Android Android applications. labels Sep 26, 2022
@lunaleaps lunaleaps self-assigned this Sep 26, 2022
@lunaleaps
Copy link
Contributor

Will take a look over 0.70 PRs that may have affected later today, or if anyone has leads, please share!

@javache
Copy link
Member

javache commented Sep 27, 2022

Could this be due to d8c25ca? Could you try backing it out to validate?

@kelset
Copy link
Contributor

kelset commented Oct 3, 2022

@lunaleaps / @javache any updates on this?

@kelset kelset added Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. and removed Needs: Triage 🔍 labels Oct 3, 2022
@lunaleaps
Copy link
Contributor

lunaleaps commented Oct 4, 2022

Confirmed reverting relevant changes d8c25ca? fixes the issue and that it's only reproducible non-Fabric

@kelset
Copy link
Contributor

kelset commented Oct 5, 2022

@lunaleaps is there already a "revert commit" in main? I'd rather cherry-pick that then do a revert locally in 0.70-branch

@lunaleaps
Copy link
Contributor

I'm not sure we can easily revert, it fixes an issue on Fabric. I'm trying some superficial attempts at fixing forward as the author is not in office right now. @javache if you have more ideas?

@javache
Copy link
Member

javache commented Oct 10, 2022

#34927 attempts to resolve this issue by enabling the new behaviour only on Fabric. I verified this resolves the repro given here, but would appreciate help testing other scenarios too (eg the drawerlayout issue)

kelset pushed a commit that referenced this issue Oct 11, 2022
Summary:
Pull Request resolved: #34927

The changes made in D36902220 (a041951) and D36958882 (d8c25ca) attempted to reduce flickering and consistency issues when using Animated.

In the old renderer, we explicitly reset all animated props, and wait for the subsequent React commit to set the props to the right state, but if `initialProps` are used, the React reconciliation may not be able to identify the prop-update is required and will leave out the value. This behaviour is different in the new renderer, where we do not explicitly `restoreDefaultValues` on detaching the animated node, and instead rely on the latest state being correct(?).

Changelog:
[General][Fixed] Stop styles from being reset when detaching Animated.Values in old renderer

Fixes #34665

Reviewed By: rshest

Differential Revision: D40194072

fbshipit-source-id: 1b3fb1d1f4a39036a501a8a21e57002035dd5659

# Conflicts:
#	Libraries/Animated/createAnimatedComponent.js
@j-piasecki
Copy link
Collaborator Author

@javache Thank you for fixing this. Your PR seems to be fixing the drawer layout issue.

@j-piasecki
Copy link
Collaborator Author

@javache @lunaleaps Sorry for the ping, but I've missed a case where an ongoing animation gets interrupted and a new one starts from the position where the old one got interrupted. Here's how it looks:

device-2022-12-07-112210.mp4

If I let the animation finish it behaves correctly, however, if I tap the button twice, the view snaps to the edge of the screen instead of staying where the animation finished.

Here's the updated reproduction
import React, { useRef, useState } from 'react';
import { View, Animated, Button } from 'react-native';

export default function App() {
  const value = useRef(new Animated.Value(0)).current;
  const isMoved = useRef(false);
  const [useAnimatedValue, setUseAnimatedValue] = useState(false);

  function animate() {
    setUseAnimatedValue(true);
    isMoved.current = !isMoved.current;
    Animated.spring(value, {
      toValue: isMoved.current ? 1 : 0,
      bounciness: 3000,
      velocity: 0.5,
      useNativeDriver: true,
    }).start(({ finished }) => {
      if (finished) {
        setUseAnimatedValue(false);
      }
    });
  }

  const offset = useAnimatedValue
    ? value.interpolate({
        inputRange: [0, 1],
        outputRange: [0, 250],
        extrapolate: 'clamp',
      })
    : isMoved.current
    ? 250
    : 0;

  return (
    <View style={{ flex: 1 }}>
      <Animated.View
        style={{
          width: 100,
          height: 100,
          backgroundColor: 'red',
          transform: [{ translateX: offset }],
        }}
      />
      <Button
        title="Animate"
        onPress={() => {
          animate();
        }}
      />
    </View>
  );
}

@kelset
Copy link
Contributor

kelset commented Dec 7, 2022

I'll open the issue again, @javache can you give this a look if you have a moment?

@kelset kelset reopened this Dec 7, 2022
@lunaleaps
Copy link
Contributor

Trying to clarify the urgency of this -- @j-piasecki, @kelset this is still breaking DrawerLayout for 0.70 re: software-mansion/react-native-gesture-handler#2208?

cc @genkikondo

@j-piasecki
Copy link
Collaborator Author

@lunaleaps Yes, it's still breaking DrawerLayout

@javache
Copy link
Member

javache commented Dec 8, 2022

Does this apply to RN latest? We've landed 5cdf3cf last month which switches out the implementation of Animated to be concurrent-safe and may have different behaviour.

@j-piasecki
Copy link
Collaborator Author

If by latest you mean 0.70.6 then yes, it's also broken there, but if you mean 0.71.0-rc.3 then it works as expected.

@kelset
Copy link
Contributor

kelset commented Dec 9, 2022

yeah I think @javache means 0.71 - good to hear that it works there, I wonder if we need to attempt to cherry-pick that commit on 0.70

@j-piasecki
Copy link
Collaborator Author

If by latest you mean 0.70.6 then yes, it's also broken there, but if you mean 0.71.0-rc.3 then it works as expected.

I don't exactly remember now, but it seems like I've checked only the new architecture (where the behavior is correct), but on the old architecture it's still broken. I'm really sorry for the confusion.

@kelset
Copy link
Contributor

kelset commented Jan 13, 2023

If by latest you mean 0.70.6 then yes, it's also broken there, but if you mean 0.71.0-rc.3 then it works as expected.

I don't exactly remember now, but it seems like I've checked only the new architecture (where the behavior is correct), but on the old architecture it's still broken. I'm really sorry for the confusion.

on which version of RN did you do that test? 0.70 or 0.71? can you try with 0.71.0? We released it yesterday

@j-piasecki
Copy link
Collaborator Author

I tried it on 0.71.0, and while it works as expected on the new architecture, on the old arch the layout jump is still visible. This applies to both, Android and iOS.

OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this issue May 22, 2023
Summary:
Pull Request resolved: facebook#34927

The changes made in D36902220 (facebook@a041951) and D36958882 (facebook@d8c25ca) attempted to reduce flickering and consistency issues when using Animated.

In the old renderer, we explicitly reset all animated props, and wait for the subsequent React commit to set the props to the right state, but if `initialProps` are used, the React reconciliation may not be able to identify the prop-update is required and will leave out the value. This behaviour is different in the new renderer, where we do not explicitly `restoreDefaultValues` on detaching the animated node, and instead rely on the latest state being correct(?).

Changelog:
[General][Fixed] Stop styles from being reset when detaching Animated.Values in old renderer

Fixes facebook#34665

Reviewed By: rshest

Differential Revision: D40194072

fbshipit-source-id: 1b3fb1d1f4a39036a501a8a21e57002035dd5659
@github-actions
Copy link

github-actions bot commented Sep 5, 2023

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Sep 5, 2023
@github-actions
Copy link

This issue was closed because it has been stalled for 7 days with no activity.

@shubhamguptadream11
Copy link
Collaborator

shubhamguptadream11 commented Oct 19, 2023

It still exists on react native: 0.72.4. Any suggestions I tries useNativeAnimations={false}. This fix is working but I don't want to do this. Also if this issue is happening on non fabric variant also so why this is fixed on only on fabric part?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated Impact: Regression Describes a behavior that used to work on a prior release, but stopped working recently. Platform: Android Android applications. Platform: iOS iOS applications. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants