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

Rendering issue of components with border style - components appear stretched while rendering a new screen #42604

Closed
jankosecki opened this issue Jan 22, 2024 · 13 comments
Labels
Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@jankosecki
Copy link

jankosecki commented Jan 22, 2024

Description

After upgrading to 0.73.2 we've started observing a rendering issue of components that contain some border styles - when screens are rendered components like that are temporarily unnaturally stretched as the new screen enters the view area.
We didn't see issues like that using 0.72.6.

Sometimes it's the border, sometimes the background of the component and sometime the issue cannot be observed at all

In our app we have some other components, which would sometimes appear as stretched when entering new screen, but the sample project, with a list of components was the easiest way to reproduce it

Steps to reproduce

  1. Install pods with yarn setup
  2. Install the app on iOS emulator with yarn ios
  3. Press Reload button on main screen or go to the next screen using + button in the top right corner
  4. Observe how elements with border are initially stretched when a screen is rendering and then shows correctly

React Native Version

0.73.2

Affected Platforms

Runtime - iOS

Output of npx react-native info

System:
  OS: macOS 14.2.1
  CPU: (8) arm64 Apple M2
  Memory: 97.02 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 21.3.0
    path: /opt/homebrew/bin/node
  Yarn:
    version: 1.22.21
    path: /opt/homebrew/bin/yarn
  npm:
    version: 10.2.4
    path: /opt/homebrew/bin/npm
  Watchman:
    version: 2023.12.04.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.12.1
    path: /Users/jkosecki/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.0
      - iOS 17.0
      - macOS 14.0
      - tvOS 17.0
      - watchOS 10.0
  Android SDK: Not Found
IDEs:
  Android Studio: 2023.1 AI-231.9392.1.2311.11255304
  Xcode:
    version: 15.0.1/15A507
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.9
    path: /usr/bin/javac
  Ruby:
    version: 2.7.6
    path: /Users/jkosecki/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.73.2
    wanted: 0.73.2
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

None

Reproducer

https://github.com/jankosecki/rn-border-issue-repro/tree/main

Screenshots and Videos

image1 image2
recording.mov
recording2.mov
@jankosecki
Copy link
Author

jankosecki commented Feb 1, 2024

I had a look how borders are rendered and did some modifications to invalidateLayer of RCTViewComponentView.mm and discovered some things:

  • setting useCoreAnimationBorderRendering to hard-coded true fixes the rendering issue of smudged components with borderWidth but it leads Separator component to disappear
  • changing logic to const bool useCoreAnimationBorderRendering = !(borderMetrics.borderWidths.top == 0 && borderMetrics.borderWidths.bottom == 1); (matches exactly the style of Separator), renders separators correctly + smudging doesn't happen

From the above, looks like the else block of condition on useCoreAnimationBorderRendering is required for some specific cases but it seems to be the reason for rendering artifacts to appear. I wonder if the observed smudging comes then from _borderLayer being invalidated and re-rendered constantly as the view is re-rendered.

I don't see any direct changes in this part of the file that happened between v0.72.10 and 0.73.10 so possibly root cause lies somewhere else

diff --git a/node_modules/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm b/node_modules/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm
index d06b0aa..fad933a 100644
--- a/node_modules/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm
+++ b/node_modules/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm
@@ -588,16 +588,17 @@ - (void)invalidateLayer
   }
 
   // Stage 2. Border Rendering
-  const bool useCoreAnimationBorderRendering =
-      borderMetrics.borderColors.isUniform() && borderMetrics.borderWidths.isUniform() &&
-      borderMetrics.borderStyles.isUniform() && borderMetrics.borderRadii.isUniform() &&
-      borderMetrics.borderStyles.left == BorderStyle::Solid &&
-      (
-          // iOS draws borders in front of the content whereas CSS draws them behind
-          // the content. For this reason, only use iOS border drawing when clipping
-          // or when the border is hidden.
-          borderMetrics.borderWidths.left == 0 ||
-          colorComponentsFromColor(borderMetrics.borderColors.left).alpha == 0 || self.clipsToBounds);
+  // const bool useCoreAnimationBorderRendering =
+  //     borderMetrics.borderColors.isUniform() && borderMetrics.borderWidths.isUniform() &&
+  //     borderMetrics.borderStyles.isUniform() && borderMetrics.borderRadii.isUniform() &&
+  //     borderMetrics.borderStyles.left == BorderStyle::Solid &&
+  //     (
+  //         // iOS draws borders in front of the content whereas CSS draws them behind
+  //         // the content. For this reason, only use iOS border drawing when clipping
+  //         // or when the border is hidden.
+  //         borderMetrics.borderWidths.left == 0 ||
+  //         colorComponentsFromColor(borderMetrics.borderColors.left).alpha == 0 || self.clipsToBounds);
+  const bool useCoreAnimationBorderRendering = !(borderMetrics.borderWidths.top == 0 && borderMetrics.borderWidths.bottom == 1);
 
   if (useCoreAnimationBorderRendering) {
     layer.mask = nil;

@cortinico any suggestions where to look further into that? I tried to add RCTLog to understand better values being used in invalidateLayer but it's called way too many times to learn anything from there

@cortinico
Copy link
Contributor

@cortinico any suggestions where to look further into that? I tried to add RCTLog to understand better values being used in invalidateLayer but it's called way too many times to learn anything from there

Is this happening also on Android by any chance?
I see you have the New Architecture enabled on iOS. Does the issue reproduces also on Old Architecture?

@jankosecki
Copy link
Author

jankosecki commented Feb 2, 2024

No, the issue has never been observed on Android.

I can confirm that the issue is not reproducible on old architecture. And the issue started happening only after upgrading to 0.73 - on 0.72 (with New Architecture) everything was working fine.

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Feb 2, 2024
@cortinico cortinico added Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) and removed Needs: Attention Issues where the author has responded to feedback. labels Feb 3, 2024
@RalissonMattias
Copy link

@jankosecki Did you find any quick solution for this?

@dioi2000
Copy link

dioi2000 commented Feb 8, 2024

suffer from the same problem

@flodlc
Copy link

flodlc commented Feb 8, 2024

Same here on 0.73.4 :/

@dioi2000
Copy link

any update?

@DorianMazur
Copy link

We have the same problem

@jankosecki
Copy link
Author

jankosecki commented Feb 20, 2024

Following the stack trace of RCTViewComponentView#invalidateLayer led me to RCTMountingManager#RCTPerformMountInstructions and to this change: #38234

Adding back at the start of RCTPerformMountInstructions:

[CATransaction begin];
[CATransaction setValue:(id)kCFBooleanTrue forKey:kCATransactionDisableActions];

and at the end of the block:

[CATransaction commit];

seems to resolve the issue in the reproduction repository.

Is it possible that the expected change was to just remove the feature flag but maintain CATransaction?
From Apple Docs:

CATransaction
A mechanism for grouping multiple layer-tree operations into atomic updates to the render tree

which is probably desired behaviour in RCTPerformMountInstructions as it iterates over list of mutations to the view and they all should be applied atomically to the render tree.

@sammy-SC

Patch to apply with patch-package for others to confirm if it solves the issue:

diff --git a/node_modules/react-native/React/Fabric/Mounting/RCTMountingManager.mm b/node_modules/react-native/React/Fabric/Mounting/RCTMountingManager.mm
index b4cfb3d..7aa00e5 100644
--- a/node_modules/react-native/React/Fabric/Mounting/RCTMountingManager.mm
+++ b/node_modules/react-native/React/Fabric/Mounting/RCTMountingManager.mm
@@ -49,6 +49,9 @@ static void RCTPerformMountInstructions(
 {
   SystraceSection s("RCTPerformMountInstructions");
 
+  [CATransaction begin];
+  [CATransaction setValue:(id)kCFBooleanTrue forKey:kCATransactionDisableActions];
+
   for (const auto &mutation : mutations) {
     switch (mutation.type) {
       case ShadowViewMutation::Create: {
@@ -147,6 +150,7 @@ static void RCTPerformMountInstructions(
       }
     }
   }
+  [CATransaction commit];
 }
 
 @implementation RCTMountingManager {

@Francesco-Voto
Copy link

I can cofnrim this seems to fix the issue also for us

@ikevin127
Copy link

Great work @jankosecki 🥇
I can confirm that the patch fixes our issue! 🎉

iOS: Native (before / after)
Before After
before.mov
after.mov

@jankosecki
Copy link
Author

Fixed properly in 0.73.6 (d979491) instead of a patch which just reverts the changes.

We've been using the version for a while and no issues have been spotted so far.

@ikevin127 my patch simply reverts some unwanted changes but the code is considered to be computation heavy (that's why it was removed in the first place). You might want to upgrade to 0.73.6 instead of using my patch to bring an actual fix to the issue.

@ikevin127
Copy link

@jankosecki Sure, Expensify already has an open PR to upgrade to v0.74 which once merged will remove the patch.

I noticed that with the patch, first paint is not instant - but it fixes out issue for now so all good. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

8 participants