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

Add 'contentInsetAdjustmentBehavior' (new in iOS 11) to ScrollView #15023

Closed

Conversation

skevy
Copy link
Contributor

@skevy skevy commented Jul 14, 2017

In iOS11, Apple added a new layout feature called "Safe Areas" (this blog post talks a bit about it: https://www.bignerdranch.com/blog/wwdc-2017-large-titles-and-safe-area-layout-guides/).

UIScrollView is one component that is affected by this change in Apple's API. When the contentInsetAdjustmentBehavior is set to automatic, for example, it will adjust the insets (and override any manually set insets) automatically based on whether or not there's a UINavigationBar, a UITabBar, a visible status bar, etc on the screen. Frustratingly, Apple decided to default to Automatic for this behavior, which will cause any apps that set contentInset/contentContainerStyle padding to have their values offset by, at the very least, the size of the status bar, when they compile their app for iOS 11. Here's more information about this behavior: https://developer.apple.com/documentation/uikit/uiscrollview/2902261-contentinsetadjustmentbehavior?language=objc

Mostly, this is a really straightforward change -- it simply adds a new iOS-only prop to ScrollView that allows setting contentInsetAdjustmentBehavior. But I did decide to default the behavior to never, so that it mimics the behavior we've seen in iOS < 11. I think it's good to keep something as crucial as scrollview content insets non-magical, and also keep it behaving similarly between platforms.

@facebook-github-bot 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. GH Review: review-needed labels Jul 14, 2017
@skevy skevy requested a review from shergin July 14, 2017 23:36
@shergin
Copy link
Contributor

shergin commented Jul 15, 2017

I think we have to just default this prop to never (and do not expose it to RN), because:

  • RCTScrollView is not ready to randomly changing contentInsets;
  • It probably will not work with RN wrappers around UINavigationBar and UITabBar anyways.

@skevy Do you really use auto value in your code?

@skevy
Copy link
Contributor Author

skevy commented Jul 15, 2017

I don’t today, but I might! :p

I think it will actually work with RN wrappers around UINavigationController and UITabBarController, as iOS makes it pretty automagical.

But honestly, I think you’re first point is the killer here. That makes sense. It probably would break in weird cases.

I’m fine with setting it to never. I will probably never use this property. :p

@skevy
Copy link
Contributor Author

skevy commented Jul 15, 2017

I could of course add support for an event when content insets are adjusted – but that would be weird when thinking about cross platform support (cuz android would never randomly change your content insets).

@shergin
Copy link
Contributor

shergin commented Jul 15, 2017

@javache Pieter, what do you think, should we expose it?

@javache
Copy link
Member

javache commented Jul 17, 2017

Given that may want to set this when integrating with other UIKit hierarchies, I think we should.

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 18, 2017
@facebook-github-bot
Copy link
Contributor

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

@shergin
Copy link
Contributor

shergin commented Jul 18, 2017

@skevy Could you please wrap all native usages of new UIScrollViewContentInsetAdjustment enum using:

#if __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_11_0
  ...
#endif

?

@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jul 18, 2017
@skevy
Copy link
Contributor Author

skevy commented Jul 19, 2017

@shergin np. Sorry for the late reply, am on vacation!

@skevy skevy requested a review from javache as a code owner July 19, 2017 20:31
@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 20, 2017
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@pull-bot
Copy link

pull-bot commented Jul 25, 2017

Warnings
⚠️

📋 Test Plan - This PR appears to be missing a Test Plan.

This PR has been submitted by a core contributor.

Generated by 🚫 dangerJS

@facebook-github-bot
Copy link
Contributor

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

facebook-github-bot pushed a commit that referenced this pull request Jul 31, 2017
Summary:
In iOS11, Apple added a new layout feature called "Safe Areas" (this blog post talks a bit about it: https://www.bignerdranch.com/blog/wwdc-2017-large-titles-and-safe-area-layout-guides/).

UIScrollView is one component that is affected by this change in Apple's API. When the `contentInsetAdjustmentBehavior` is set to `automatic`, for example, it will adjust the insets (and override any manually set insets) automatically based on whether or not there's a UINavigationBar, a UITabBar, a visible status bar, etc on the screen. Frustratingly, Apple decided to default to `Automatic` for this behavior, which will cause any apps that set contentInset/contentContainerStyle padding to have their values offset by, at the very least, the size of the status bar, when they compile their app for iOS 11. Here's more information about this behavior: https://developer.apple.com/documentation/uikit/uiscrollview/2902261-contentinsetadjustmentbehavior?language=objc

Mostly, this is a really straightforward change -- it simply adds a new iOS-only prop to ScrollView that allows setting `contentInsetAdjustmentBehavior`. But I did decide to default the behavior to `never`, so that it mimics the behavior we've seen in iOS < 11. I think it's good to keep something as crucial as scrollview content insets non-magical, and also keep it behaving similarly between platforms.
Closes #15023

Reviewed By: javache

Differential Revision: D5517552

Pulled By: hramos

fbshipit-source-id: c9ce4bf331b3d243228268d826fdd4dcee99981d
ide pushed a commit to expo/react-native that referenced this pull request Aug 1, 2017
Summary:
In iOS11, Apple added a new layout feature called "Safe Areas" (this blog post talks a bit about it: https://www.bignerdranch.com/blog/wwdc-2017-large-titles-and-safe-area-layout-guides/).

UIScrollView is one component that is affected by this change in Apple's API. When the `contentInsetAdjustmentBehavior` is set to `automatic`, for example, it will adjust the insets (and override any manually set insets) automatically based on whether or not there's a UINavigationBar, a UITabBar, a visible status bar, etc on the screen. Frustratingly, Apple decided to default to `Automatic` for this behavior, which will cause any apps that set contentInset/contentContainerStyle padding to have their values offset by, at the very least, the size of the status bar, when they compile their app for iOS 11. Here's more information about this behavior: https://developer.apple.com/documentation/uikit/uiscrollview/2902261-contentinsetadjustmentbehavior?language=objc

Mostly, this is a really straightforward change -- it simply adds a new iOS-only prop to ScrollView that allows setting `contentInsetAdjustmentBehavior`. But I did decide to default the behavior to `never`, so that it mimics the behavior we've seen in iOS < 11. I think it's good to keep something as crucial as scrollview content insets non-magical, and also keep it behaving similarly between platforms.
Closes facebook#15023

Reviewed By: javache

Differential Revision: D5517552

Pulled By: hramos

fbshipit-source-id: c9ce4bf331b3d243228268d826fdd4dcee99981d
ide pushed a commit to expo/react-native that referenced this pull request Aug 4, 2017
Summary:
In iOS11, Apple added a new layout feature called "Safe Areas" (this blog post talks a bit about it: https://www.bignerdranch.com/blog/wwdc-2017-large-titles-and-safe-area-layout-guides/).

UIScrollView is one component that is affected by this change in Apple's API. When the `contentInsetAdjustmentBehavior` is set to `automatic`, for example, it will adjust the insets (and override any manually set insets) automatically based on whether or not there's a UINavigationBar, a UITabBar, a visible status bar, etc on the screen. Frustratingly, Apple decided to default to `Automatic` for this behavior, which will cause any apps that set contentInset/contentContainerStyle padding to have their values offset by, at the very least, the size of the status bar, when they compile their app for iOS 11. Here's more information about this behavior: https://developer.apple.com/documentation/uikit/uiscrollview/2902261-contentinsetadjustmentbehavior?language=objc

Mostly, this is a really straightforward change -- it simply adds a new iOS-only prop to ScrollView that allows setting `contentInsetAdjustmentBehavior`. But I did decide to default the behavior to `never`, so that it mimics the behavior we've seen in iOS < 11. I think it's good to keep something as crucial as scrollview content insets non-magical, and also keep it behaving similarly between platforms.
Closes facebook#15023

Reviewed By: javache

Differential Revision: D5517552

Pulled By: hramos

fbshipit-source-id: c9ce4bf331b3d243228268d826fdd4dcee99981d
facebook-github-bot pushed a commit that referenced this pull request Oct 31, 2017
…mentBehavior

Summary:
Same IOS 11 issue as #15023
Fixes the annoying 20px content insert on top.
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Fix the behaviour of WebView when building the iOS project with Xcode 9 due to the `contentInsetAdjustmentBehavior`.

![expected](https://user-images.githubusercontent.com/5630513/32207551-b7789668-bdca-11e7-840a-f325b2767d08.jpg)
![iphone_x_after](https://user-images.githubusercontent.com/5630513/32207557-beb8fd6e-bdca-11e7-87d0-18d533f20125.jpg)

![issue_normal_iphone_ios11](https://user-images.githubusercontent.com/5630513/32207572-d773be5c-bdca-11e7-8e28-8f0783eef1cd.jpg)
![iphone_x_before](https://user-images.githubusercontent.com/5630513/32207581-e3e93234-bdca-11e7-847d-f801bbf05d55.jpg)

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAl  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
 [IOS] [BUGFIX] [WebView] - Fix extra white space added to webView due to iOS 11 contentInsetAdjustmentBehavior
Closes #16519

Differential Revision: D6195670

Pulled By: shergin

fbshipit-source-id: 08d4d4dc700059bb7707e038dc4f592af0275896
@ide ide deleted the @skevy/add-ios11-scrollview-opt-upstream branch April 25, 2020 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants