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

UI updates made from layout effect are flushed in separate UI transaction #44111

Closed
kmagiera opened this issue Apr 16, 2024 · 6 comments
Closed
Labels
Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. p: Software Mansion Partner: Software Mansion Partner Resolution: Fixed A PR that fixes this issue has been merged. Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@kmagiera
Copy link
Contributor

kmagiera commented Apr 16, 2024

Description

This issue describes a change in behavior when UI updates are made from useLayoutEffect calls between old and new architecture.

The issue surfaced when trying expo router with the new architecture and is due to the code in Screen.tsx that calls a method that updates a state in useLayoutEffect hook.

In this scenario, we have two main components being mounted at the same time: a parent and child component. The parent component renders with some default set of attributes, but then the child component uses useLayoutEffect hook that gets triggered immediately upon mounting to update the parent state and as a consequence make parent re-render with some of the default attributes changed. A simple version of this scenario is implemented in the reproducer app: https://github.com/kmagiera/use-layout-effect-ui-flash/blob/main/ReproducerApp/App.tsx

In the above scenario, with the old architecture, the parent component receives both initial and updates attributes in a single UI transaction, meaning that they will be flushed onto screen in one go and you'll never see the intermediate state before the initial attributes are set and the updates ones are applied.
However, with the new architecture, the two updates come in separate transactions that are scheduled to run on UI thread.
When the two transactions are executed in between frames, you'll see the intermediate UI state flushed briefly as presented on the video attached later on.

Here is a screenshot from the reproducer app that presents three consecutive frames after pressing "toggle" button. In the reproducer app, the parent component renders with red background at first, and then the child component updates the background to blue.
frames

With the old architecture, you never get the frame with red background as all the updates happen in a single UI thread loop run.

Note: I am unsure whether it is a bug or intended behavior. There is a possibility that React or React Native doesn't want to guarantee for such updates to happen in single UI transaction and that expo router shouldn't rely in this side effect. That being said, despite the fact expo router's approach adds an additional render pass, it makes its API much more elegant.

Steps to reproduce

  1. Clone reproducer app https://github.com/kmagiera/use-layout-effect-ui-flash
  2. Build fabric version for iOS
  3. Notice that when pressing "toggle" button the background color of the new element shortly flashes red before turning blue

Notice that this issue doesn't happen when app is running on the old architecture.

The reproducer app uses ALotOfViews components in order to make the bug reproduce more reliably. The same issue happens even without these additional views, but it reproduces only a fraction of times from my testing. The root cause of the problem seem to be that finalizeUpdates gets called twice, while on Paper, both updates happen in the same UIManager batch.

Another way to reproduce the issue is to remove ALotOfViews component and put breakpoint in the finalizeUpdates method and notice that once it hits the breakpoint for the second time, the view is already created and its background color is red. On paper, you can add didSetProps to RCTView and observe it gets triggered only once after receiving both background color updates.

React Native Version

0.73.6

Affected Platforms

Runtime - iOS
Runtime - Android

Areas

Fabric - The New Renderer

Output of npx react-native info

info Fetching system and libraries information...
System:
  OS: macOS 13.5.2
  CPU: (10) arm64 Apple M2 Pro
  Memory: 81.53 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.17.1
    path: ~/.nvm/versions/node/v18.17.1/bin/node
  Yarn:
    version: 1.22.21
    path: ~/.nvm/versions/node/v18.17.1/bin/yarn
  npm:
    version: 9.6.7
    path: ~/.nvm/versions/node/v18.17.1/bin/npm
  Watchman: Not Found
Managers:
  CocoaPods:
    version: 1.15.2
    path: /Users/mdk/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.2
      - iOS 17.2
      - macOS 14.2
      - tvOS 17.2
      - visionOS 1.0
      - watchOS 10.2
  Android SDK: Not Found
IDEs:
  Android Studio: 2023.2 AI-232.10300.40.2321.11567975
  Xcode:
    version: 15.2/15C500b
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.9
    path: /usr/bin/javac
  Ruby:
    version: 3.3.0
    path: /Users/mdk/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.74.0-rc.9
    wanted: 0.74.0-rc.9
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

n/a

Reproducer

https://github.com/kmagiera/use-layout-effect-ui-flash

Screenshots and Videos

Reproducer app running on fabric (see red background flashes):

fabric.mp4

Reproducer app running on Paper (no red background flashes):

paper.mp4
@kmagiera kmagiera added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Apr 16, 2024
Copy link

⚠️ Add or Reformat Version Info
ℹ️ We could not find or parse the version number of React Native in your issue report. Please use the template, and report your version including major, minor, and patch numbers - e.g. 0.70.2

@cortinico cortinico added Issue: Author Provided Repro This issue can be reproduced in Snack or an attached project. and removed Needs: Author Feedback Needs: Version Info labels Apr 16, 2024
@kmagiera
Copy link
Contributor Author

I just tested this on Android and there's also similar difference on Android with and w/o Fabric. Attaching video from Android with fabric where you can also see red background flashes:
android-fabric.webm

@cortinico
Copy link
Contributor

Thanks for the extremely detailed report @kmagiera

So there are a number of things at play here. We're aware that useLayoutEffect doesn't work correctly in the New Architecture till 0.74 (included).

The reason behind this is that in order for useLayoutEffect to effectively be blocking the rendering on native, we needed a series of changes inside the runtime scheduler (i.e. we needed an API to synchronously block the rendering on native).

Those changes have now landed on main and we expect useLayoutEffect to work correctly in 0.75. Ideally, you should be able to test your changes on the latest react-native@nightly and it should work correctly.

However, I've tested it and I've realized the current implementation is broken, and we'll have to fix it (thanks to your report we were able to spot this early on!). I'm coordinating with @sammy-SC on how we can fix this thing forward, and ideally have a nightly version where useLayoutEffect works correctly.

I haven't fully investigated why it seems to work correctly on old arch as useLayoutEffect should have been broken on both archs.

javache added a commit to javache/react-native that referenced this issue Apr 18, 2024
…nabled

Summary:
When using `batchRenderingUpdatesInEventLoop` the Fabric Differentiator may more a newly created ShadowNode and subsequent update (eg coming from a layout effect) into a single MountingTransaction. This enables us to correctly implement layoutEffect in the new architecture and prevent flickering of intermediate states.

On Android, we also have a view preallocation mechanism, which will create native views for ShadowNodes as they're created by React. This mechanism is incompatible with `batchRenderingUpdatesInEventLoop` as we will ignore the `create` instruction in the MountingTransaction since we already have a preallocated view. However the props we used for preallocation will be outdated/different compared with the ones in the `MountingTransaction`, which represents a severe regression to the issue described in facebook#44111.

Changelog: [Android][Fixed] Mutations to newly created views from useLayoutEffect were not always applied.

Differential Revision: D56301318
javache added a commit to javache/react-native that referenced this issue Apr 18, 2024
…nabled (facebook#44144)

Summary:

When using `batchRenderingUpdatesInEventLoop` the Fabric Differentiator may more a newly created ShadowNode and subsequent update (eg coming from a layout effect) into a single MountingTransaction. This enables us to correctly implement layoutEffect in the new architecture and prevent flickering of intermediate states.

On Android, we also have a view preallocation mechanism, which will create native views for ShadowNodes as they're created by React. This mechanism is incompatible with `batchRenderingUpdatesInEventLoop` as we will ignore the `create` instruction in the MountingTransaction since we already have a preallocated view. However the props we used for preallocation will be outdated/different compared with the ones in the `MountingTransaction`, which represents a severe regression to the issue described in facebook#44111.

Changelog: [Android][Fixed] Mutations to newly created views from useLayoutEffect were not always applied.

Differential Revision: D56301318
@cortinico
Copy link
Contributor

Hey all,
I'd like to provide a small update here.

As mentioned in my previous update, the useLayoutEffect is known to be broken in 0.74 RCs. The correct behavior is fixed on main for iOS and we're discussing about including it in 0.74.1 to unblock libraries and partners.

So if you test your reproducer against react-native@nightly, you will see that it's fixed.

However, I've tested it and I've realized the current implementation is broken, and we'll have to fix it (thanks to your report we were able to spot this early on!).

For Android the situation is more complicated, as we realized that view preallocation is conflicting with the recent changes that were supposed to fix useLayoutEffect in the New Architecture. We do have a couple of alternatives here (see #44144 for more context) and we're discussing which one to go forward and include in 0.74.1 to fix this behavior for Android as well.

@brentvatne
Copy link
Collaborator

hey folks, I just wanted to post the videos I've previously shared with you on other platforms for reference on this issue for the broader audience to understand how this issue might manifest in their apps.

New arch enabled

Screen_Recording_2024-04-04_at_12.12.44_PM.mov

Old arch enabled

Screen_Recording_2024-04-04_at_12.18.11_PM.mov

@cortinico
Copy link
Contributor

Closing as this has been fixed for good in 0.74.1 🎉

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

No branches or pull requests

4 participants