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

[0.74][iOS] Wrong state of Touchable components with React.Suspense on New Architecture #44044

Closed
tboba opened this issue Apr 11, 2024 · 14 comments
Assignees
Labels
API: Animated Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. p: Software Mansion Partner: Software Mansion Partner Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@tboba
Copy link

tboba commented Apr 11, 2024

Description

In react-freeze, we're using a component that wraps content into React.Suspense. However, since 0.74 RC's we can observe that some of the Touchable components are staying in a focused state, after suspensing them. It seems that after re-render, all of the native views are being created anew, leading to pass the state from the previous to the new instances of components. In our case, React tries to pass the props, but it updates the component that doesn't already exist, and it passes the unmounted state to the new one. You can see on the repro, that on re-render React loses the information about the scroll position of ScrollView, whereas the Touchable keeps the focused state on the new instance.

Steps to reproduce

  1. Create Touchable component and wrap it into React.Suspense.
  2. Add logic that will suspense the element (e.g. setTimeout)
  3. The element should be blocked in the focused state.
  4. You can additionally create a ScrollView to observe, that all of the components are being recreated during the re-render.

React Native Version

0.74.0-rc.8

Affected Platforms

Runtime - Android, Runtime - iOS

Areas

Fabric - The New Renderer

Output of npx react-native info

System:
  OS: macOS 14.4.1
  CPU: (10) arm64 Apple M2 Pro
  Memory: 294.63 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.16.0
    path: ~/.nvm/versions/node/v18.16.0/bin/node
  Yarn:
    version: 1.22.21
    path: ~/.nvm/versions/node/v18.16.0/bin/yarn
  npm:
    version: 9.5.1
    path: ~/.nvm/versions/node/v18.16.0/bin/npm
  Watchman:
    version: 2024.03.25.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.15.2
    path: /usr/local/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.4
      - iOS 17.4
      - macOS 14.4
      - tvOS 17.4
      - visionOS 1.1
      - watchOS 10.4
  Android SDK:
    Android NDK: 22.1.7171670
IDEs:
  Android Studio: Not Found
  Xcode:
    version: 15.3/15E204a
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.9
    path: /usr/bin/javac
  Ruby:
    version: 2.6.10
    path: /usr/bin/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.73.6
    wanted: 0.73.6
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: true
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

Unfortunately, there's no specific stacktrace that is shown during during that process.

[BUNDLE] ./index.js 

[(NOBRIDGE) LOG] Bridgeless mode is enabled
[(NOBRIDGE) LOG] Running "FabricTestExample" with {"rootTag":1,"initialProps":{"concurrentRoot":true},"fabric":true}

Reproducer

https://github.com/tboba/RN-Animated-Reproducer

Screenshots and Videos

0.74 - Paper
Components are not being recreated; thus, touchable has unfocused state after re-render.

Screen.Recording.2024-04-12.at.11.47.11.mov

0.74 - Fabric
Components are being recreated (ScrollView forgot its position), but touchable has lost information about untouching.

Screen.Recording.2024-04-12.at.11.44.21.mov
@tboba tboba added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Apr 11, 2024
@rickhanlonii
Copy link
Member

What was the last known version that this worked in the "before" case?

@kmagiera
Copy link
Contributor

We investigated this a bit more, but it was omitted in the original description but can be seen on the videos. The root cause of this issue is that with Fabric the native components are recreated after being suspended. This can be seen on the video where scroll view is being rendered that has a state (scroll position) which isn't saved into the react component. When the scrollview is suspended and unsuspended, the scroll position is reset. This is because a new view is created and replaces the previous instance of the UIScrollView that was there before suspense.

The issue with button is basically the same. The button that appears after suspense is a new button. It is in the pressed state, because that's the opacity of the button at the moment when it gets suspended. In the meantime animated runs the animation that brings the opacity back to 1, but it applies that to the "old button" and when the new one is attached back it uses that old state.

@tboba
Copy link
Author

tboba commented Apr 11, 2024

Hey @rickhanlonii, I've already tested this scenario on RN 0.72 and 0.73 on Fabric, but I couldn't reproduce that bug there 😕 So I can assume this was working properly up to version 0.73!
As @kmagiera said, the root cause of this bug is how React Native recreates components after being suspended. I'll correct the description to describe this issue more briefly. Thanks!

@tboba tboba changed the title [0.74] Different behavior of using Animated with React.Suspense on New Architecture [0.74] Wrong state of Touchable components with React.Suspense on New Architecture Apr 12, 2024
@rickhanlonii
Copy link
Member

OK thanks for confirming that this is new in 0.74.

the root cause of this bug is how React Native recreates components after being suspended

If this is happening, it sounds like a bug. When React suspends an already committed tree, we put it into an offscreen Activity. That should maintain the state and native views while it's suspended, and when the tree-recommits, we re-use the previous state and views to commit the updated tree. So if the native views are being re-created, that's a bug. And if it was working in 0.73, that means it was previously working with the React 18 behavior that does this, so my guess is something changed in RN.

@kmagiera
Copy link
Contributor

what's important to note is that it doesn't work on the new architecture only. With the old architecture and on the same version of react it works ok.

@cortinico
Copy link
Contributor

cortinico commented Apr 15, 2024

@tboba @kmagiera I've spent some time investigating this. I believe that this issue is affecting iOS only at the moment (I'd like to get a confirmation from you to rule out Android though).

Specifically, here is a run of your reproducer with the JVM debugger attached at the ReactRootView.

Specifically, I'm inspecting the 4 childs of ReactRootView (the textview, the scrollview, the button and the debug overlay) to make sure that the class instances are not recreated (you can see that the @ numbers are not changing between invocations).

Screen.Recording.2024-04-15.at.15.46.06.mov

Moreover, the ScrollView is correctly rendered at the right state and the button is also not affected. If you confirm this, we'll focus our attentions on iOS as this seems to be a regression introduced in the latest versions.

@cortinico cortinico self-assigned this Apr 15, 2024
@cortinico cortinico added Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. and removed Needs: Triage 🔍 labels Apr 15, 2024
@tboba
Copy link
Author

tboba commented Apr 16, 2024

Hey @cortinico, thanks for checking this! Yeah I can confirm this affects only iOS - as I can observe, components seems to be not recreated on Android.

@cortinico cortinico added the Platform: iOS iOS applications. label Apr 16, 2024
@cortinico cortinico changed the title [0.74] Wrong state of Touchable components with React.Suspense on New Architecture [0.74][iOS] Wrong state of Touchable components with React.Suspense on New Architecture Apr 16, 2024
@cipolleschi
Copy link
Contributor

@tboba @kmagiera I'm investigating the issue and I wanted to provide an update.

While debugging, I could see that on iOS, as much as on Android, ScrollView instances are not created anew. The iOS object reference (the memory address) of RCTScrollViewComponentView stays the same.

Our current working hypothesis is that:

  • scrolling updates the Native state, changing the content offset. React does not know about this change as it happens in Native.
  • When the reload is triggered, Suspense stores the node somewhere and Fabric, in native, unmount the view and prepare it for recycle
  • When suspense is done, React restores the node in JS from the point before the Native State update happened, as it was not aware of it, thus losing the scroll positioning.

We need to first verify that this is the case, and secondly look for a possible solution.

I'll try to keep you updated as much as possible.

@cipolleschi
Copy link
Contributor

cipolleschi commented Apr 17, 2024

Also... Just tested the same repro in 0.73 and it is broken in the same way! cc. @cortinico


I went further and test the following configurations:

Configuration \ Version 0.73 0.74
Bridge
Bridgeless

@kmagiera
Copy link
Contributor

Thanks for taking a look @cipolleschi. This is an interesting finding as I remember we've been also checking this with debugger and by taking native hierarchy snapshot, and in our tests we've been seeing that all view instances in suspended subtree were different before and after suspense. I'll double check that again if that wasn't a mistake.

@kmagiera
Copy link
Contributor

Following up here again as I tested this once more with RC9. I can confirm that on the provided repro those objects are of the same instances. However, it turns out that this is due to the view recycling pool. So the view in fact gets recreated, but since fabric has a recycling pool, it just picks the same instance.

I modified the repro app such that there is another button that I press while the scroll view is away this way, it uses that old scroll view from the pool. After that operation the scroll under suspended tree is a completely new instance:

Code of the modified version is here: https://gist.github.com/kmagiera/89dec0e535f293fe0bd613765bfa4dc5

Also attaching the video:

Area.mp4

@cipolleschi
Copy link
Contributor

Thanks for the repro @kmagiera! I'll look into it.
To set expectation, given that it is not a regression and it was broken also on 0.73, we are going to release 0.74.0 as it is today and fix this in 0.74.1. Rest assured that we want to get to the bottom of this!

@cortinico cortinico assigned cipolleschi and unassigned cortinico Apr 19, 2024
sammy-SC added a commit to sammy-SC/react-native that referenced this issue Apr 22, 2024
Summary:
## Changelog:

[iOS] [Fixed] - Fixed stale state on TouchableOpacity and TouchableBounce

When TouchableOpacity and TouchableBounce are unmounted, we need to reset their state. This includes animation state. If we don't do that, view is unmounted on the mounting layer and animation will not be applied. This leaves view in undefined state. In TouchableOpacity, it is view with reduced opacity. TouchableBounce that is view with applied transform.

This was reported in facebook#44044

Reviewed By: rubennorte, cipolleschi

Differential Revision: D56416571
sammy-SC added a commit to sammy-SC/react-native that referenced this issue Apr 22, 2024
…ok#44182)

Summary:

## Changelog:

[iOS] [Fixed] - Fixed stale state on TouchableOpacity and TouchableBounce

When TouchableOpacity and TouchableBounce are unmounted, we need to reset their state. This includes animation state. If we don't do that, view is unmounted on the mounting layer and animation will not be applied. This leaves view in undefined state. In TouchableOpacity, it is view with reduced opacity. TouchableBounce that is view with applied transform.

This was reported in facebook#44044

Reviewed By: rubennorte, cipolleschi

Differential Revision: D56416571
facebook-github-bot pushed a commit that referenced this issue Apr 23, 2024
Summary:
Pull Request resolved: #44182

## Changelog:

[iOS] [Fixed] - Fixed stale state on TouchableOpacity and TouchableBounce

When TouchableOpacity and TouchableBounce are unmounted, we need to reset their state. This includes animation state. If we don't do that, view is unmounted on the mounting layer and animation will not be applied. This leaves view in undefined state. In TouchableOpacity, it is view with reduced opacity. TouchableBounce that is view with applied transform.

This was reported in #44044

Reviewed By: rubennorte, cipolleschi

Differential Revision: D56416571

fbshipit-source-id: 01214ec8a5e07c80a609e082b955a30305ad8396
@cortinico cortinico added the Resolution: PR Submitted A pull request with a fix has been provided. label Apr 23, 2024
@cipolleschi cipolleschi removed the Resolution: PR Submitted A pull request with a fix has been provided. label Apr 24, 2024
@cipolleschi
Copy link
Contributor

@cortinico I removed the label, because the PR fixes only half of the problem. We are still iterating to figure out the other half!

cipolleschi pushed a commit that referenced this issue May 1, 2024
Summary:
Pull Request resolved: #44182

## Changelog:

[iOS] [Fixed] - Fixed stale state on TouchableOpacity and TouchableBounce

When TouchableOpacity and TouchableBounce are unmounted, we need to reset their state. This includes animation state. If we don't do that, view is unmounted on the mounting layer and animation will not be applied. This leaves view in undefined state. In TouchableOpacity, it is view with reduced opacity. TouchableBounce that is view with applied transform.

This was reported in #44044

Reviewed By: rubennorte, cipolleschi

Differential Revision: D56416571

fbshipit-source-id: 01214ec8a5e07c80a609e082b955a30305ad8396
kosmydel pushed a commit to kosmydel/react-native that referenced this issue May 6, 2024
…ok#44182)

Summary:
Pull Request resolved: facebook#44182

## Changelog:

[iOS] [Fixed] - Fixed stale state on TouchableOpacity and TouchableBounce

When TouchableOpacity and TouchableBounce are unmounted, we need to reset their state. This includes animation state. If we don't do that, view is unmounted on the mounting layer and animation will not be applied. This leaves view in undefined state. In TouchableOpacity, it is view with reduced opacity. TouchableBounce that is view with applied transform.

This was reported in facebook#44044

Reviewed By: rubennorte, cipolleschi

Differential Revision: D56416571

fbshipit-source-id: 01214ec8a5e07c80a609e082b955a30305ad8396
@cipolleschi
Copy link
Contributor

Both fixed landed and this issue is fixed in 0.74.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. p: Software Mansion Partner: Software Mansion Partner Platform: iOS iOS applications. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

6 participants