-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Closed
ScrollView doesn't reset inset when contentInsetAdjustmentBehavior is automatic #34165
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
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
Labels
Component: ScrollView
Needs: Triage 🔍
Type: New Architecture
Issues and PRs related to new architecture (Fabric/Turbo Modules)
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 whencontentInsetAdjustmentBehavior
is set toautomatic
. 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 theScrollView
isn't adjusting for the navigation bar. The second video is the same example but with the old architecture. You can see that theScrollView
behaves correctly.All these bugs are because the new architecture is recycling the
ScrollView
. I turned off the recycling of theScrollView
and the bug went away. Would you be open to a PR that turns off view recycling forScrollViews
with adjustment behaviour set to automatic?It should be possible to keep recycling on for the
RCTSCrollViewComponent
and just create theUIScrollView
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
The text was updated successfully, but these errors were encountered: