Make SafeAreaView to work on iOS < 11

[vovkasm]

Currently SafeAreaView works only on iOS 11, because implemented in terms of native safeArea API, that not exists in older iOS versions. But this make it hard to use the component in real applications, because content will be under top bars on older versions of iOS and no reliable way to workaround this in js. More motivation in #17638

This changeset emulate safe area in terms of UIViewController layout guides API if safeArea not available.

Fixes #17638, #18255

Test Plan

I run RNTester with these simulators: iPhone6 (9.3), iPhone6 (10.0), iPhone6 (11.2), iPhoneX (11.2)

• Start RNTester application
• Look on top header, it should not overlap status bar
• Go to the <SafeAreaView> example, open modal
• Modal area should not overlap status bar

RNTester before change

RNTester after change

Release Notes

[IOS] [BUGFIX] [SafeAreaView] - Make SafeAreaView to work on iOS < 11

[vovkasm]

I'am not sure about proper release notes categorization because SafeAreaView undocumented currently at http://facebook.github.io/react-native/docs . Should it be enhancement or fix, or may be internal?

[satya164]

I think it would be nice to document if you can :)
It's by no means an internal component and it sucks to have to read third party blog posts to know how to use it instead of official docs.

[satya164]

cc @grabbou @brentvatne

[vovkasm]

@satya164 I agree. But I think documentation should be distinct PR. And I'm not native speaker, so it would be good, if anyone else create documentation patch.

[vovkasm]

@grabbou What is progress on testing?

On my side, I integrated this patch to one project and it works. If anyone needs to test it, your can specify this in package.json:

    "react-native": "https://github.com/vovkasm/react-native.git#v0.54.3-woof.2",

Also this branch has other non related modifications, they should not break anything. If you worry, here is the full list of changes:

1. Disable packager connection on device (until we will find better mechanism to disable infinity connection attempts to packager)
2. Make DevTools configurable #17617 - Make DevTools configurable from menu
3. Make SafeAreaView to work on iOS < 11 #18534 - Make SafeAreaView to work on iOS < 11

[vovkasm]

After self review yersterday I found that documentation for SafeAreaView exists in current docs, after analyzing, I found that this was done 7 days ago by commit facebook/react-native-website@415e003.
This commit adds description to SafeAreaView also to versions 0.52, 0.53, 0.54.
The documentation does not mention any restrictions on iOS versions, so I think that this change is bug fix and will fix it in original pull request description as:

[IOS] [BUGFIX] [SafeAreaView] - Make SafeAreaView to work on iOS < 11

[vovkasm]

@shergin Sorry to bother you, but could you please review this changes.

[satya164]

I think it'll also be nice to have something like SafeAreaView.isAvailable() to detect if SafeAreaView is supported in the current environment. It'll allow libraries to support multiple versions of React Native and will also keep it compatible in case SafeAreaView is supported in other platforms like Android.

[grabbou]

@satya164 Generally speaking, I think we should start thinking about creating an Android implementation of this component too (instead of isAvailable() API). It's quite handy to stop thinking about StatusBar height and other constraints when laying out your container, especially now that notches are becoming even more popular.

I might be missing something, but a simple padding/margin with StatusBar height on Android would do the trick, right? At least that's what I am usually doing inside my apps.

[satya164]

@grabbou: I think we should start thinking about creating an Android implementation of this component too (instead of isAvailable() API)

Sure. But that's not why I'm proposing isAvailable API. Libraries need to support multiple versions of React Native. There's no way to do that if SafeAreaView supports different platforms and different OS versions across different React Native version. Once SafeAreaView is supported on all platforms and OS versions, the API can be deprecated and eventually removed. With the API, libraries can know when SafeAreaView handles things automatically and when they need to handle it manually.

I might be missing something, but a simple padding/margin with StatusBar height on Android would do the trick, right? At least that's what I am usually doing inside my apps.

The way SafeAreaView currently works is that it adds the padding when it detects that your component is under elements such as status bar. Hardcoding a padding won't work, because it's always going to apply the padding regardless of if the component is under the status bar. A proper implementation would need to measure the coordinates of the component to detect if there's an overlap and apply the needed amount of padding. This cannot be done in JS since measuring is async and has to be done natively.

[vovkasm]

What is the progress on this? Can I help to land this code faster?

[kelset]

Looks like one of the tests failed with a build error

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring root project 'react-native'.
> Could not resolve all dependencies for configuration ':classpath'.
   > Could not download bcpkix-jdk15on.jar (org.bouncycastle:bcpkix-jdk15on:1.48)
      > Could not get resource 'https://jcenter.bintray.com/org/bouncycastle/bcpkix-jdk15on/1.48/bcpkix-jdk15on-1.48.jar'.
         > java.lang.NullPointerException (no error message)

Which (my best guess) a simple rerun would fix.

I'll try to bring this up to the core, it seems like it's a good feature to have.

Regarding the documentation, since there is already a section, if this PR lands I think you should do a small PR to the dedicated md on the docs repo to let people know how to use / that it works with iOS < 11.

[grabbou]

Rerun will do - that error happens quite often.

…

On Fri, 6 Apr 2018 at 09:55 Lorenzo Sciandra ***@***.***> wrote: Looks like one of the tests failed with a build error FAILURE: Build failed with an exception. * What went wrong: A problem occurred configuring root project 'react-native'. > Could not resolve all dependencies for configuration ':classpath'. > Could not download bcpkix-jdk15on.jar (org.bouncycastle:bcpkix-jdk15on:1.48) > Could not get resource 'https://jcenter.bintray.com/org/bouncycastle/bcpkix-jdk15on/1.48/bcpkix-jdk15on-1.48.jar'. > java.lang.NullPointerException (no error message) Which (my best guess) a simple rerun would fix. I'll try to bring this up to the core, it seems like it's a good feature to have. Regarding the documentation, since there is already a section, if this PR lands I think you should do a small PR to the dedicated md on the docs repo <https://github.com/facebook/react-native-website/blob/master/docs/safeareaview.md> to let people know how to use / that it works with iOS < 11. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18534 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACWcxhKBGvZOMzv4YtmtbiVyAVH3l-Yaks5tlx9jgaJpZM4S5ofz> .

[grabbou]

I think it's time to move ahead with this PR - where we are?

[grabbou]

Sure. But that's not why I'm proposing isAvailable API. Libraries need to support multiple versions of React Native. There's no way to do that if SafeAreaView supports different platforms and different OS versions across different React Native version

I think more standard approach, in my opinion, would be to just rename it to SafeAreaViewIOS until it's landing on Android and problem solved. That would be similar to what we were doing from the very beginning at the Alert / AlertIOS and StatusBar / StatusBarIOS examples. The isAvailable() API is, in my opinion, redundant given that we already have tools to handle cross-platform code. All in all, you will still have to check whether isAvailable function / constant is present under SafeAreaView which is same as checking whether it exists at all.

[satya164]

Can't just rename it. It'll break existing libraries. Need to have a deprecation path to rename. And when you add it back later to support Android, it would be pretty confusing for libraries. isAvailable seems to be a much simpler and straightforward way right now imo. The thing with isAvailable is that it doesn't break any existing code, while makes it easier for future usage and new additions to the API.

[facebook-github-bot]

@vovkasm I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[vovkasm]

Will not rebase, because current master totally broken with babel 7 transition (

Potential reviewer is @shergin as the author of the original code.

[satya164]

@vovkasm can you add an isAvailable method?

[vovkasm]

@satya164 I can, but prefer to do it in distinct PR because:

• I don't see consensus on it (@grabbou ?)
• I don't sure about name isAvailable, perhaps isSupported sounds better
• This PR is simple bugfix, but isAvailable is a feature, new API. It can be easily added without this PR and vice versa

But if most people vote for it, then I will add.

[vovkasm]

Just publish RN v0.55.3 with this patch (we use this version at work, see #18534 (comment)):

• url for package.json: https://github.com/vovkasm/react-native.git#v0.55.3-woof.0
• for browser: https://github.com/vovkasm/react-native/tree/v0.55.3-woof.0

[Bonobomagno]

If using expo, we can still use something like
STATUS_BAR_HEIGHT: parseInt(Platform.Version) < 11 || Platform.OS !== 'ios' ? Constants.statusBarHeight : 0

[vovkasm]

@Bonobomagno Strictly speaking, no... using Constants.statusBarHeight is incorrect, because status bar height is not a constant :-)

[vovkasm]

I'm seriously starting to think that publishing of an independent component to npm is a proper way to go with this issue. It seems that at the moment RN team focuses on hard rewriting core, not in stabilizing the code.

[kelset]

RN team focuses on hard rewriting core, not in stabilizing the code.

I can understand your frustration but I can assure we are mostly focusing (at least, the OSS-side of core team) on stabilizing the codebase. Literally, our efforts at the moment are around the CI and fixing issues not strictly related to this feature.

Feel free to publish it as a separate package - it's a free world after all - what I can do is try to bring it once more to the attention of the core team.

[hramos]

We encourage people to go the third party module route whenever possible. As @kelset said, we're focusing on core stability at the moment. As long as our tests are not all green, landing PRs is not the top priority.

[hramos]

Now that tests are green, I'm going through PRs. I'm hesitant about merging this PR. We have been using SafeAreaView in Facebook's family of apps for a while now, and it does not appear we have run into this issue. @shergin?

[vovkasm]

@hramos I think that "Test Plan" in the PR clearly shows the issue, isn't it? It exists in RNTester.

We have been using SafeAreaView in Facebook's family of apps for a while now, and it does not appear we have run into this issue.

So... :-)

[charpeni]

Once this PR is merged, we would have to revert this PR: facebook/react-native-website#429.

[facebook-github-bot]

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

[hramos]

This has been accepted. I'll go through the motions of landing the internal diff soon.

[react-native-bot]

This pull request was closed by @vovkasm in 3949e93.

Once this commit is added to a release, you will see the corresponding version tag below the description at 3949e93. If the commit has a single master tag, it is not yet part of a release.

[vovkasm]

@hramos, can you provide some info? Why this deoptimization was done? Is it because some technical reasons or code style or something another?

[vovkasm]

Does this change come from some internal conversations? What purpose?

I wonder because I intentionally moved relative slow call of respondsToSelector method out of "critical path" code in layoutSubviews. It was tradeoff speed/size, cost of this optimization was 1 byte stored in ivars area.

[hramos]

This was one of the changes requested as part of the internal review (cc @PeteTheHeat).