Fix contentInsetAdjustmentBehavior set to automatic on ScrollView in the new architecture #34217

grahammendick · 2022-07-18T20:35:49Z

Summary

Fixes #34165 and Large title fails 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) 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 #34165 and Large title fails 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

Test Plan

I checked the fix in both the repros in #34165 and Large title fails. Here is a video of the fix running with large titles in the new architecture.

Screen.Recording.2022-07-18.at.21.31.11.mov

iOS doesn't like reusing the underlying UIScrollView. It won't reset the content insets because it think it's already done it. So recreated the scroll view when the component is recycled. Also had to trigger a contentSize update after recycling otherwise the scroll view started with no inset and wouldn't scroll.

The underlying UIScrollView is create new each time so don't want to use default props instead of current props for comparison. Backed out the reset of other scroll props because not needed now it's created new each time

Previous solution of recreating scroll view each time didn't work for the large-title-bug. Go to large title, scroll to collapse, go back then go to large title and it would be collapsed - it would scroll slightly so could see the first line of content but it would be collapsed. Stumbled across changing frame from 0 then back to old value and it fixes all the inset behavior auto bugs. Guess it resets everything - without resetting the frame it remembered some settings and made it go through a completely different path when remounting from when recycling

analysis-bot · 2022-07-18T21:05:50Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,820,484	+0
android	hermes	armeabi-v7a	7,214,372	+0
android	hermes	x86	8,134,204	+0
android	hermes	x86_64	8,112,309	+0
android	jsc	arm64-v8a	9,698,003	+0
android	jsc	armeabi-v7a	8,454,127	+0
android	jsc	x86	9,649,800	+0
android	jsc	x86_64	10,248,023	+0

Base commit: e1d17c8
Branch: main

analysis-bot · 2022-07-18T22:58:46Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 3188727
Branch: main

facebook-github-bot · 2022-07-19T08:14:02Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi

Hi @grahammendick, thanks for the PR.
I left some suggestions that could simplify the code. Thanks to @sammy-SC for them.

If you could try the last change, it could make the code much easier.

React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm

As per review comments, there's no need to delay the reset. Can do it all in when recycling

cipolleschi

Sorry to bother you again but, could you rebase using main, please?
The PR looks good, but there are some failures in CI that should go away after a Rebase

facebook-github-bot · 2022-07-20T16:30:55Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-07-20T16:34:43Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

react-native-bot · 2022-07-20T20:55:04Z

This pull request was successfully merged by @grahammendick in 27fe6f1.

^{When will my fix make it into a release? | Upcoming Releases}

sammy-SC · 2022-07-22T11:21:06Z

@grahammendick thank you for fixing this!

grahammendick · 2022-07-22T17:35:36Z

@sammy-SC it's a crucial fix for my Navigation router so thanks for merging it so quickly 👍

…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

…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 <[email protected]>

…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 <[email protected]>

grahammendick added 7 commits July 17, 2022 14:43

Reset props back to default when recycled

36ab76d

The underlying UIScrollView is create new each time so don't want to use default props instead of current props for comparison. Backed out the reset of other scroll props because not needed now it's created new each time

Reset event throttle for consistency

0877c74

Removed old scroll view before adding new

2b1be32

Fixed typo

06e6c8f

Started as null - only need update after a recycle

a81587f

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jul 18, 2022

react-native-bot added Bug Platform: iOS iOS applications. labels Jul 18, 2022

cipolleschi requested changes Jul 19, 2022

View reviewed changes

React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm Outdated Show resolved Hide resolved

React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm Outdated Show resolved Hide resolved

Changed frame to 0 then back again on recycle

bba6de8

As per review comments, there's no need to delay the reset. Can do it all in when recycling

grahammendick requested a review from cipolleschi July 19, 2022 16:15

cipolleschi approved these changes Jul 20, 2022

View reviewed changes

Merge branch 'facebook:main' into issue-34165

0274549

facebook-github-bot closed this in 27fe6f1 Jul 20, 2022

react-native-bot added the Merged This PR has been merged. label Jul 20, 2022

grahammendick referenced this pull request in grahammendick/navigation Jul 28, 2022

Merge pull request #613 from grahammendick/tabbar-appearance

18c2b2a

grahammendick mentioned this pull request Mar 21, 2024

UIView properties are not reset to default values when recycled #42732

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix contentInsetAdjustmentBehavior set to automatic on ScrollView in the new architecture #34217

Fix contentInsetAdjustmentBehavior set to automatic on ScrollView in the new architecture #34217

grahammendick commented Jul 18, 2022

analysis-bot commented Jul 18, 2022 •

edited

Loading

analysis-bot commented Jul 18, 2022 •

edited

Loading

facebook-github-bot commented Jul 19, 2022

cipolleschi left a comment

cipolleschi left a comment

facebook-github-bot commented Jul 20, 2022

facebook-github-bot commented Jul 20, 2022

react-native-bot commented Jul 20, 2022

sammy-SC commented Jul 22, 2022

grahammendick commented Jul 22, 2022

Fix contentInsetAdjustmentBehavior set to automatic on ScrollView in the new architecture #34217

Fix contentInsetAdjustmentBehavior set to automatic on ScrollView in the new architecture #34217

Conversation

grahammendick commented Jul 18, 2022

Summary

Changelog

Test Plan

analysis-bot commented Jul 18, 2022 • edited Loading

analysis-bot commented Jul 18, 2022 • edited Loading

facebook-github-bot commented Jul 19, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 20, 2022

facebook-github-bot commented Jul 20, 2022

react-native-bot commented Jul 20, 2022

sammy-SC commented Jul 22, 2022

grahammendick commented Jul 22, 2022

analysis-bot commented Jul 18, 2022 •

edited

Loading

analysis-bot commented Jul 18, 2022 •

edited

Loading