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

ScrollView doesn't reset inset when contentInsetAdjustmentBehavior is automatic #34165

Closed
grahammendick opened this issue Jul 9, 2022 · 0 comments · Fixed by microsoft/react-native-macos#1484
Labels
Component: ScrollView Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@grahammendick
Copy link
Contributor

Description

This is similar to but NOT the same as the bug I raised in the Working Group. The difference here is that the ScrollView has scrollable content.

I've started updating my navigation library to work with the new architecture and I've run into a problem on iOS. The ScrollView doesn't work when contentInsetAdjustmentBehavior is set to automatic. You can see what I mean in the first video below. The first time you navigate to the second screen then the ScrollView behaves correctly. But the second time you navigate you can see that the ScrollView isn't adjusting for the navigation bar. The second video is the same example but with the old architecture. You can see that the ScrollView behaves correctly.

All these bugs are because the new architecture is recycling the ScrollView. I turned off the recycling of the ScrollView and the bug went away. Would you be open to a PR that turns off view recycling for ScrollViews with adjustment behaviour set to automatic?

// In _enqueueComponentViewWithComponentHandle of RCTComponentViewRegistry.mm
BOOL recycle = ![componentViewDescriptor.view isKindOfClass:[RCTScrollViewComponentView class]]
    || ((RCTScrollViewComponentView *) componentViewDescriptor.view).scrollView.contentInsetAdjustmentBehavior == UIScrollViewContentInsetAdjustmentAutomatic;
if (recycle)
  recycledViews.push_back(componentViewDescriptor);

It should be possible to keep recycling on for the RCTSCrollViewComponent and just create the UIScrollView new each time. But I tried it and couldn't get it to work.

Version

0.69.1

Output of npx react-native info

System:
OS: macOS 12.4
CPU: (8) arm64 Apple M1 Pro
Memory: 461.39 MB / 16.00 GB
Shell: 5.8.1 - /bin/zsh
Binaries:
Node: 17.7.1 - /opt/homebrew/bin/node
Yarn: 1.22.18 - /opt/homebrew/bin/yarn
npm: 8.5.2 - /opt/homebrew/bin/npm
Watchman: 2022.03.14.00 - /opt/homebrew/bin/watchman
Managers:
CocoaPods: 1.11.3 - /usr/local/bin/pod
SDKs:
iOS SDK:
Platforms: DriverKit 21.4, iOS 15.5, macOS 12.3, tvOS 15.4, watchOS 8.5
Android SDK:
API Levels: 29, 30, 31, 32
Build Tools: 29.0.2, 30.0.2, 31.0.0, 32.0.0, 32.1.0
System Images: android-30 | Intel x86 Atom_64, android-30 | Google Play ARM 64 v8a, android-32 | Google APIs ARM 64 v8a
Android NDK: Not Found
IDEs:
Android Studio: 2021.1 AI-211.7628.21.2111.8193401
Xcode: 13.4.1/13F100 - /usr/bin/xcodebuild
Languages:
Java: 11.0.11 - /usr/bin/javac
npmPackages:
@react-native-community/cli: Not Found
react: 18.0.0 => 18.0.0
react-native: 0.69.1 => 0.69.1
react-native-macos: Not Found
npmGlobalPackages:
react-native: Not Found

Steps to reproduce

I've created a repository with a reproduction of the bug. The README in the repo contains the steps needed to recreate the problem. There are two apps in the repo, one built with the old architecture and the other built with the new architecture. You can see that the bug only happens on the new architecture. The only dependencies in the apps are the 3 packages that make up my navigation library.

The ScrollView misbehaving on the new architecture

Screen.Recording.2022-07-09.at.14.20.00.mov

The ScrollView behaving correctly on the old architecture

Screen.Recording.2022-07-09.at.14.18.11.mov

Snack, code example, screenshot, or link to a repository

https://github.com/grahammendick/content-inset-adjustment-auto-scroll-bug

@grahammendick grahammendick added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Jul 9, 2022
facebook-github-bot pushed a commit that referenced this issue Jul 28, 2022
…34254)

Summary:
Fixes #34120

The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In #34120 there are videos comparing the positioning of a native action view in the old and the new architecture.

This PR passes the parent tag to the `updateLayout` method of the `SurfaceMountingManager`. The `SurfaceMountingManager` calls `needsCustomLayoutForChildren` on the parent view manager (copied the code from the `NativeViewHierarchyManager` in the old architecture).

**NOTE** - I wasn't sure where to get the parent shadow view from so I've put in my best guesses where I could and left it as `{}` otherwise.

## Changelog

[Android] [Fixed] - Migrate `needsCustomLayoutForChildren` check to the new architecture

Pull Request resolved: #34254

Test Plan:
I checked the fix in the repro from #34165. Here is a video of the action view closing using the native button that is now visible in the new architecture.

https://user-images.githubusercontent.com/1761227/180607896-35bf477f-4552-4b8a-8e09-9e8c49122c0c.mov

Reviewed By: cipolleschi

Differential Revision: D38153924

Pulled By: javache

fbshipit-source-id: e2c77fa70d725a33ce73fe4a615f6d884312580c
roryabraham pushed a commit to Expensify/react-native that referenced this issue Aug 17, 2022
…acebook#34254)

Summary:
Fixes facebook#34120

The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In facebook#34120 there are videos comparing the positioning of a native action view in the old and the new architecture.

This PR passes the parent tag to the `updateLayout` method of the `SurfaceMountingManager`. The `SurfaceMountingManager` calls `needsCustomLayoutForChildren` on the parent view manager (copied the code from the `NativeViewHierarchyManager` in the old architecture).

**NOTE** - I wasn't sure where to get the parent shadow view from so I've put in my best guesses where I could and left it as `{}` otherwise.

## Changelog

[Android] [Fixed] - Migrate `needsCustomLayoutForChildren` check to the new architecture

Pull Request resolved: facebook#34254

Test Plan:
I checked the fix in the repro from facebook#34165. Here is a video of the action view closing using the native button that is now visible in the new architecture.

https://user-images.githubusercontent.com/1761227/180607896-35bf477f-4552-4b8a-8e09-9e8c49122c0c.mov

Reviewed By: cipolleschi

Differential Revision: D38153924

Pulled By: javache

fbshipit-source-id: e2c77fa70d725a33ce73fe4a615f6d884312580c
roryabraham pushed a commit to Expensify/react-native that referenced this issue Aug 17, 2022
…acebook#34254)

Summary:
Fixes facebook#34120

The new React Native architecture doesn't check `needsCustomLayoutForChildren` so it wrongly positions native views on Android. In facebook#34120 there are videos comparing the positioning of a native action view in the old and the new architecture.

This PR passes the parent tag to the `updateLayout` method of the `SurfaceMountingManager`. The `SurfaceMountingManager` calls `needsCustomLayoutForChildren` on the parent view manager (copied the code from the `NativeViewHierarchyManager` in the old architecture).

**NOTE** - I wasn't sure where to get the parent shadow view from so I've put in my best guesses where I could and left it as `{}` otherwise.

## Changelog

[Android] [Fixed] - Migrate `needsCustomLayoutForChildren` check to the new architecture

Pull Request resolved: facebook#34254

Test Plan:
I checked the fix in the repro from facebook#34165. Here is a video of the action view closing using the native button that is now visible in the new architecture.

https://user-images.githubusercontent.com/1761227/180607896-35bf477f-4552-4b8a-8e09-9e8c49122c0c.mov

Reviewed By: cipolleschi

Differential Revision: D38153924

Pulled By: javache

fbshipit-source-id: e2c77fa70d725a33ce73fe4a615f6d884312580c
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this issue Nov 11, 2022
…the new architecture (facebook#34217)

Summary:
Fixes facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) on the new React Native architecture.

There are problems with setting `contentInsetAdjustmentBehavior` to `automatic` on the `ScrollView` component in the new React Native architecture. The `automatic` setting matters to navigation libraries (like [my Navigation router](https://github.com/grahammendick/navigation)) because it stops the `ScrollView` from overlapping the `UINavigationBar`. The setting also powers important native features like large titles and search bars on iOS.

The `automatic` setting works fine on the old architecture. It doesn’t work on the new architecture because React Native is recycling views. In facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) there are videos comparing the setting in the old and the new architecture.

## Changelog

[iOS] [Fixed] - Fix `contentInsetAdjustmentBehavior` set to `automatic` on `ScrollView` in the new architecture

Pull Request resolved: facebook#34217

Test Plan:
I checked the fix in both the repros in facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43). Here is a video of the fix running with large titles in the new architecture.

https://user-images.githubusercontent.com/1761227/179612188-162b896b-82c5-45de-bb5a-ba80f452fbee.mov

Reviewed By: sammy-SC

Differential Revision: D37952506

Pulled By: cipolleschi

fbshipit-source-id: 6cff6c85aa33b579405fe34a9e36c8630f4c24bd
christophpurrer pushed a commit to christophpurrer/react-native-macos that referenced this issue Nov 11, 2022
…the new architecture (facebook#34217)

Summary:
Fixes facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) on the new React Native architecture.

There are problems with setting `contentInsetAdjustmentBehavior` to `automatic` on the `ScrollView` component in the new React Native architecture. The `automatic` setting matters to navigation libraries (like [my Navigation router](https://github.com/grahammendick/navigation)) because it stops the `ScrollView` from overlapping the `UINavigationBar`. The setting also powers important native features like large titles and search bars on iOS.

The `automatic` setting works fine on the old architecture. It doesn’t work on the new architecture because React Native is recycling views. In facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) there are videos comparing the setting in the old and the new architecture.

## Changelog

[iOS] [Fixed] - Fix `contentInsetAdjustmentBehavior` set to `automatic` on `ScrollView` in the new architecture

Pull Request resolved: facebook#34217

Test Plan:
I checked the fix in both the repros in facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43). Here is a video of the fix running with large titles in the new architecture.

https://user-images.githubusercontent.com/1761227/179612188-162b896b-82c5-45de-bb5a-ba80f452fbee.mov

Reviewed By: sammy-SC

Differential Revision: D37952506

Pulled By: cipolleschi

fbshipit-source-id: 6cff6c85aa33b579405fe34a9e36c8630f4c24bd
Saadnajmi pushed a commit to microsoft/react-native-macos that referenced this issue Nov 11, 2022
…the new architecture (facebook#34217) (#1484)

Summary:
Fixes facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) on the new React Native architecture.

There are problems with setting `contentInsetAdjustmentBehavior` to `automatic` on the `ScrollView` component in the new React Native architecture. The `automatic` setting matters to navigation libraries (like [my Navigation router](https://github.com/grahammendick/navigation)) because it stops the `ScrollView` from overlapping the `UINavigationBar`. The setting also powers important native features like large titles and search bars on iOS.

The `automatic` setting works fine on the old architecture. It doesn’t work on the new architecture because React Native is recycling views. In facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) there are videos comparing the setting in the old and the new architecture.

## Changelog

[iOS] [Fixed] - Fix `contentInsetAdjustmentBehavior` set to `automatic` on `ScrollView` in the new architecture

Pull Request resolved: facebook#34217

Test Plan:
I checked the fix in both the repros in facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43). Here is a video of the fix running with large titles in the new architecture.

https://user-images.githubusercontent.com/1761227/179612188-162b896b-82c5-45de-bb5a-ba80f452fbee.mov

Reviewed By: sammy-SC

Differential Revision: D37952506

Pulled By: cipolleschi

fbshipit-source-id: 6cff6c85aa33b579405fe34a9e36c8630f4c24bd

Co-authored-by: Graham Mendick <graham.mendick@gmail.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this issue Feb 13, 2023
…the new architecture (facebook#34217) (microsoft#1484)

Summary:
Fixes facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) on the new React Native architecture.

There are problems with setting `contentInsetAdjustmentBehavior` to `automatic` on the `ScrollView` component in the new React Native architecture. The `automatic` setting matters to navigation libraries (like [my Navigation router](https://github.com/grahammendick/navigation)) because it stops the `ScrollView` from overlapping the `UINavigationBar`. The setting also powers important native features like large titles and search bars on iOS.

The `automatic` setting works fine on the old architecture. It doesn’t work on the new architecture because React Native is recycling views. In facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) there are videos comparing the setting in the old and the new architecture.

## Changelog

[iOS] [Fixed] - Fix `contentInsetAdjustmentBehavior` set to `automatic` on `ScrollView` in the new architecture

Pull Request resolved: facebook#34217

Test Plan:
I checked the fix in both the repros in facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43). Here is a video of the fix running with large titles in the new architecture.

https://user-images.githubusercontent.com/1761227/179612188-162b896b-82c5-45de-bb5a-ba80f452fbee.mov

Reviewed By: sammy-SC

Differential Revision: D37952506

Pulled By: cipolleschi

fbshipit-source-id: 6cff6c85aa33b579405fe34a9e36c8630f4c24bd

Co-authored-by: Graham Mendick <graham.mendick@gmail.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this issue Mar 10, 2023
…the new architecture (facebook#34217) (microsoft#1484)

Summary:
Fixes facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) on the new React Native architecture.

There are problems with setting `contentInsetAdjustmentBehavior` to `automatic` on the `ScrollView` component in the new React Native architecture. The `automatic` setting matters to navigation libraries (like [my Navigation router](https://github.com/grahammendick/navigation)) because it stops the `ScrollView` from overlapping the `UINavigationBar`. The setting also powers important native features like large titles and search bars on iOS.

The `automatic` setting works fine on the old architecture. It doesn’t work on the new architecture because React Native is recycling views. In facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) there are videos comparing the setting in the old and the new architecture.

## Changelog

[iOS] [Fixed] - Fix `contentInsetAdjustmentBehavior` set to `automatic` on `ScrollView` in the new architecture

Pull Request resolved: facebook#34217

Test Plan:
I checked the fix in both the repros in facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43). Here is a video of the fix running with large titles in the new architecture.

https://user-images.githubusercontent.com/1761227/179612188-162b896b-82c5-45de-bb5a-ba80f452fbee.mov

Reviewed By: sammy-SC

Differential Revision: D37952506

Pulled By: cipolleschi

fbshipit-source-id: 6cff6c85aa33b579405fe34a9e36c8630f4c24bd

Co-authored-by: Graham Mendick <graham.mendick@gmail.com>
shwanton pushed a commit to shwanton/react-native-macos that referenced this issue Mar 10, 2023
…the new architecture (facebook#34217) (microsoft#1484)

Summary:
Fixes facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) on the new React Native architecture.

There are problems with setting `contentInsetAdjustmentBehavior` to `automatic` on the `ScrollView` component in the new React Native architecture. The `automatic` setting matters to navigation libraries (like [my Navigation router](https://github.com/grahammendick/navigation)) because it stops the `ScrollView` from overlapping the `UINavigationBar`. The setting also powers important native features like large titles and search bars on iOS.

The `automatic` setting works fine on the old architecture. It doesn’t work on the new architecture because React Native is recycling views. In facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43) there are videos comparing the setting in the old and the new architecture.

## Changelog

[iOS] [Fixed] - Fix `contentInsetAdjustmentBehavior` set to `automatic` on `ScrollView` in the new architecture

Pull Request resolved: facebook#34217

Test Plan:
I checked the fix in both the repros in facebook#34165 and [Large title fails](reactwg/react-native-new-architecture#43). Here is a video of the fix running with large titles in the new architecture.

https://user-images.githubusercontent.com/1761227/179612188-162b896b-82c5-45de-bb5a-ba80f452fbee.mov

Reviewed By: sammy-SC

Differential Revision: D37952506

Pulled By: cipolleschi

fbshipit-source-id: 6cff6c85aa33b579405fe34a9e36c8630f4c24bd

Co-authored-by: Graham Mendick <graham.mendick@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment