react-native icon indicating copy to clipboard operation
react-native copied to clipboard

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

Open jankosecki opened this issue 1 year ago • 10 comments

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

https://github.com/facebook/react-native/assets/41573247/04c69f21-a532-40eb-adc7-0249a6bd31fd

https://github.com/facebook/react-native/assets/41573247/b2cf12c6-50e0-4d34-94f7-1cd104ae03e1

jankosecki avatar Jan 22 '24 18:01 jankosecki

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

jankosecki avatar Feb 01 '24 22:02 jankosecki

@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?

cortinico avatar Feb 02 '24 11:02 cortinico

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.

jankosecki avatar Feb 02 '24 17:02 jankosecki

@jankosecki Did you find any quick solution for this?

RalissonMattias avatar Feb 07 '24 17:02 RalissonMattias

suffer from the same problem

dioi2000 avatar Feb 08 '24 18:02 dioi2000

Same here on 0.73.4 :/

flodlc avatar Feb 08 '24 18:02 flodlc

any update?

dioi2000 avatar Feb 19 '24 11:02 dioi2000

We have the same problem

DorianMazur avatar Feb 20 '24 09:02 DorianMazur

Following the stack trace of RCTViewComponentView#invalidateLayer led me to RCTMountingManager#RCTPerformMountInstructions and to this change: https://github.com/facebook/react-native/pull/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 {

jankosecki avatar Feb 20 '24 20:02 jankosecki

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

Francesco-Voto avatar Feb 21 '24 11:02 Francesco-Voto

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

iOS: Native (before / after)
Before After

ikevin127 avatar Apr 11 '24 22:04 ikevin127

Fixed properly in 0.73.6 (https://github.com/facebook/react-native/commit/d9794916b0549330513fe530abf090fa48b8a776) 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.

jankosecki avatar Apr 17 '24 13:04 jankosecki

@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!

ikevin127 avatar Apr 17 '24 14:04 ikevin127