UIView properties are not reset to default values when recycled

[j-piasecki]

Description

Some properties of UIView are not reset when the views are recycled. The field that caused a problem in our case is autoresizingMask. By default its set to UIViewAutoresizingNone but if it gets changed to other values in any way, it stays this way on a view that gets recycled and put in different places of the application causing weird layout issues and it diverges from Yoga.

The linked repro works by implementing a custom view that sets the mask to UIViewAutoresizingNone or UIViewAutoresizingWidth | UIViewAutoresizingHeight depending on a prop value (Toggle auto W+H button) on its children. Then, when the views are recycled (Toggle ReproView button, which changes the hierarchy that gets rendered), the autoresizingMask stays on the recycled view it was set on.

I guess the big question here is: should the library be responsible for resetting the changed values since the problem doesn't occur when it's disabled?

If the answer is yes, how would the approach be when the OS changes the value? It's happening in case of react-native-pager-view here: https://github.com/callstack/react-native-pager-view/blob/7c30957352687906921795c8dd48ed18db9fdad0/ios/Fabric/RNCPagerViewComponentView.mm#L224.

Steps to reproduce

1. Install the application with the reproduction
2. Click on Toggle auto W+H, set it to true
3. Click on Toggle ReproView, set it to false
4. Click on Toggle wide and/or Toggle tall
5. Observe the bounds of the inner view change when it should not

React Native Version

0.73.3

Affected Platforms

Runtime - iOS

Areas

Fabric - The New Renderer

Output of npx react-native info

System:
  OS: macOS 14.2.1
  CPU: (8) arm64 Apple M1 Pro
  Memory: 85.38 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.10.0
    path: ~/.nvm/versions/node/v20.10.0/bin/node
  Yarn:
    version: 1.22.19
    path: /opt/homebrew/bin/yarn
  npm:
    version: 10.2.3
    path: ~/.nvm/versions/node/v20.10.0/bin/npm
  Watchman:
    version: 2023.01.16.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.14.3
    path: /Users/jakubpiasecki/.rvm/gems/ruby-2.7.5/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.2
      - iOS 17.2
      - macOS 14.2
      - tvOS 17.2
      - watchOS 10.2
  Android SDK:
    API Levels:
      - "24"
      - "26"
      - "28"
      - "29"
      - "30"
      - "31"
      - "32"
      - "33"
      - "34"
    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
      - 33.0.1
      - 34.0.0
    System Images:
      - android-28 | Google ARM64-V8a Play ARM 64 v8a
      - android-33 | Google APIs ARM 64 v8a
      - android-34 | Google Play ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2023.1 AI-231.9392.1.2311.11255304
  Xcode:
    version: 15.1/15C65
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.2
    path: /usr/bin/javac
  Ruby:
    version: 2.7.5
    path: /Users/jakubpiasecki/.rvm/rubies/ruby-2.7.5/bin/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

```text
This is a visual bug.

Reproducer

https://github.com/j-piasecki/rn-view-recycling-repro

Screenshots and Videos

Screen.Recording.2024-01-30.at.11.49.10.mov

[github-actions]

⚠️

Newer Version of React Native is Available!

ℹ️

You are on a supported minor version, but it looks like there's a newer patch available - 0.73.3. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.

[zhongwuzw]

I think you can override - (void)prepareForRecycle of RCTViewComponentView to do the cleanup ?

[j-piasecki]

The problem here is that it's not a custom component inheriting after it but RCTViewComponentView itself, so I can't do it easily.

[zhongwuzw]

Fabric already supports custom view recycled optional in #35378, but it's only in main branch and not released, after that released, seems we can define a category of RCTViewComponentView, which override + (bool)shouldBeRecycled method to disable recycle for a temporary fix.

I can make a PR to add an interface for Fabric built-in components to make recycling optional.

[cipolleschi]

The problem here is that it's not a custom component inheriting after it but RCTViewComponentView itself, so I can't do it easily.

I looked into the example a bit more, but your ReproViewComponentView is a subclass of RCTViewComponentView itself.

So, in ReproViewComponentView you should be able to do:

- (void)prepareForRecycle 
{
  [super prepareForRecycle];
  _autoSize = NO;
  self.autoresizingMask = UIViewAutoresizingNone;
}

Resetting the autoresizingMask in the base component is probably not what we want it might lead to inconsistent behaviors.

Looking at this comment can you provide a reproducer with react-native-pager-view? We might be able to find where to put the reset code in the library.

[j-piasecki]

@cipolleschi Here it is: https://github.com/j-piasecki/autoresizingmask-repro. Just add a breakpoint inside the setAutoresizingMask of RCTViewComponentView and press the button.

[cipolleschi]

@j-piasecki I'm making some tests with the code you provided.
One thing I noticed is that, yes, the prepareForRecycle is invoked but you are not getting a recycled view. They are always created anew. So resetting the autoresizeMask will not fix the problem in this specific example

1. Creating a simple UIPageViewController

Notice that this is something defined in an extension in the library

UIPageViewController* tst = [[UIPageViewController alloc] initWithView:[[UIView alloc] init]];`

In this case, [tst.view autoresizingMask] is nil;

2. Creating a Scroll, Horizontal UIPageViewControllerm with no options

UIPageViewController* tst = [[UIPageViewController alloc]
                                 initWithTransitionStyle:UIPageViewControllerTransitionStyleScroll
                                 navigationOrientation:UIPageViewControllerNavigationOrientationHorizontal 
                                 options:@{}];

In this case, [tst.view autoresizingMask] is 18;

3. Creating a PageCurl, Horizontal UIPageViewControllerm with no options

UIPageViewController* tst = [[UIPageViewController alloc]
                                 initWithTransitionStyle:UIPageViewControllerTransitionStylePageCurl
                                 navigationOrientation:UIPageViewControllerNavigationOrientationHorizontal 
                                 options:@{}];

In this case, [tst.view autoresizingMask] is 18;

4. Creating a PageCurl, Vertical UIPageViewControllerm with no options

UIPageViewController* tst = [[UIPageViewController alloc]
                                 initWithTransitionStyle:UIPageViewControllerTransitionStylePageCurl
                                 navigationOrientation:UIPageViewControllerNavigationOrientationVertical 
                                 options:@{}];

In this case, [tst.view autoresizingMask] is 18;

5. Creating a Scroll, Horizontal UIPageViewControllerm with no options

UIPageViewController* tst = [[UIPageViewController alloc]
                                 initWithTransitionStyle:UIPageViewControllerTransitionStyleScroll
                                 navigationOrientation:UIPageViewControllerNavigationOrientationVertical 
                                 options:@{}];

In this case, [tst.view autoresizingMask] is 18;

6. Tested outside react native

I created a new iOS native project, no react native involved and added:

UIPageViewController* tst2 = [[UIPageViewController alloc]
                                   initWithTransitionStyle:UIPageViewControllerTransitionStyleScroll
                                   navigationOrientation:UIPageViewControllerNavigationOrientationHorizontal
                                   options:@{}];
  UIPageViewController* tst3 = [[UIPageViewController alloc]
                                   initWithTransitionStyle:UIPageViewControllerTransitionStylePageCurl
                                   navigationOrientation:UIPageViewControllerNavigationOrientationHorizontal
                                   options:@{}];
  UIPageViewController* tst4 = [[UIPageViewController alloc]
                                   initWithTransitionStyle:UIPageViewControllerTransitionStyleScroll
                                   navigationOrientation:UIPageViewControllerNavigationOrientationVertical
                                   options:@{}];
  UIPageViewController* tst5 = [[UIPageViewController alloc]
                                   initWithTransitionStyle:UIPageViewControllerTransitionStylePageCurl
                                   navigationOrientation:UIPageViewControllerNavigationOrientationVertical
                                   options:@{}];
  
  NSLog(@"autoresizeMask for tst2 is %u", [tst2.view autoresizingMask]);
  NSLog(@"autoresizeMask for tst3 is %u", [tst3.view autoresizingMask]);
  NSLog(@"autoresizeMask for tst4 is %u", [tst4.view autoresizingMask]);
  NSLog(@"autoresizeMask for tst5 is %u", [tst5.view autoresizingMask]);

And the console log is

So, I think that the behavior is correct. Fabric is not involved in setting/resetting the autoresizeMask, and resetting it on recycling is not the right thing. This is how UIKit is supposed to work and if we reset the autoresizeMask on the base RCTViewComponentView, we might invalidate some UIKit assumptions and create incidents down the line.

Can you share more in which scenario you are encountering the issue and what is the actual behavior and what would be the expected behavior?

[j-piasecki]

@cipolleschi Sorry for the late reply, I've missed the notification.

One thing I noticed is that, yes, the prepareForRecycle is invoked but you are not getting a recycled view. They are always created anew. So resetting the autoresizeMask will not fix the problem in this specific example

I'm not sure what you mean by They are always created anew. If you look at the stack trace from the issue, autoresizingMask is set on a RCTViewComponentView, which then may get recycled with the mask still set, causing the layout to break.

Can you share more in which scenario you are encountering the issue and what is the actual behavior and what would be the expected behavior?

It's a bit hard to prepare the exact reproduction, since we only observed it in the Expensify app while working on adapting it to the new architecture and it didn't seem to be deterministic. If this helps, this is what we were seeing:

Those problems all were fixed by https://github.com/WoLewicki/App/blob/d6287ba33feb664d4c2aa131cb9d09b177f4bb18/patches/react-native%2B0.73.2%2B010%2BresetAutoresizingOnView.patch

We also had to disable recycling for TextInput components, since in some cases the placeholder would be moved & cut inside:

(This is from the same screen as the screenshots above)

We tried to get to the bottom of why exactly it's happening, but in the end disabling recycling was much quicker and effective approach, so I don't have any more details to share here 😞.

[grahammendick]

I had a similar problem when React Native recycled ScrollViews. I fixed it by changing the frame size to zero and back again in prepareForRecycle. Can you try this patch instead?

CGRect oldFrame = self.frame;
self.frame = CGRectZero;
self.frame = oldFrame;

[cipolleschi]

I'm not sure what you mean by They are always created anew. If you look at the stack trace from the issue, autoresizingMask is set on a RCTViewComponentView, which then may get recycled with the mask still set, causing the layout to break.

No, what I was trying to say is that even a Native app (no React Native) which uses the UIPageViewController has the autoresizeMask set like that (see example 6, here). The root cause it is not the Fabric view recycling: it is how UIKit works!

[j-piasecki]

The root cause it is not the Fabric view recycling: it is how UIKit works!

I get your point but on the other hand, UIKit doesn’t recycle views so it’s not a problem there, Fabric does recycle them. I’m not sure what would be the correct solution, but resetting autoresizingMask does the trick since RN doesn’t rely on it. Or do you see any other solutions to this?

[cipolleschi]

But I don't understand the issue:

• UIKIt do not recycle views, BUT it always create a new view with those value for autoresizingMask.
• Fabric recycle the view, BUT it doesn't change the value of autoresizingMask.

So, with vanilla iOS, every time the UIPageViewController creates a new view, you get a view with autoresizingMask equal to 18.
With Fabric, the UIPageViewController creates the view once and it sets autoresizingMask to 18. And every time Fabric returns a view for the UIPageViewController, it gives you a view with 18 for autoresizingMask, which is the same value.

The only problem might occur when Fabric returns a view that has been created by UIPageViewController for a different purpose, for example as a View that is used as container for something else.
Is that the case, perhaps?

[j-piasecki]

The only problem might occur when Fabric returns a view that has been created by UIPageViewController for a different purpose, for example as a View that is used as container for something else.
Is that the case, perhaps?

I believe that's exactly the case. Sorry if I didn't make it clear.

[cipolleschi]

I'll talk with @sammy-SC about that. What I'm confused about is that Fabric should not recycle that view, because, technically, that view is not a RCTViewComponentView as the UIPageViewController, being an Apple component, can't provide a view that conforms to a React Native protocol... so, this is something funky.

[j-piasecki]

I updated the repro linked above: j-piasecki/autoresizingmask-repro@9a554eb. Now it should be relatively easy to reproduce the issue:

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

[cipolleschi]

Thanks for updating the reproducer! I was talking with @sammy-SC about this a couple of days ago.
As I mention, Fabric is not able to recycle and manage views that are created by UIKit. So what's happening is probably that pager-view is taking a view created by Fabric and setting it as contentView of the UIPageController which triggers the setting of the autoresizingMask.

This is probably a breaking change between the old and new architecture and might require a change in the pager-view. I'll investigate it further next week, to see if that's the case.

[cipolleschi]

Closing this to clean up the issues. Let's refer to callstack/react-native-pager-view#819 as it is a library's change