lightbox: Fix SafeAreaInsets padding is not applied.

[jainkuniya]

Wrap screen content with SafeAreaView from react-native instead of
View.

References: https://facebook.github.io/react-native/docs/safeareaview

Fixes: #3291

[jainkuniya]

@gnprice @borisyankov Let's take this small fix too!

[gnprice]

Very interesting, thanks!

Is the lightbox the only screen this is applicable to? What's the reason for that?

The linked docs say this only applies on iOS. There are Android devices with a notch in the screen -- what about them? We already have a safeAreaInsets mechanism which we use in some places; should we use that here instead of SafeAreaView?

[gnprice]

There are Android devices with a notch in the screen -- what about them?

BTW, for investigating this kind of question: on Android P, the developer settings have an option to simulate a notch (or two!):

[jainkuniya]

I have a notch in my phone, so I can test without any simulator 😅. Actually on Android, we can't paint/overlap app on status bar by default so it works fine. But on iOS by default we can. Other screen uses Screen component, so it is taken care by it :)

…

On Tue, Feb 26, 2019, 4:41 AM Greg Price ***@***.*** wrote: There are Android devices with a notch in the screen -- what about them? BTW, for investigating this kind of question: on Android P, the developer settings have an option to simulate a notch (or two!): [image: image] <https://user-images.githubusercontent.com/28173/53374943-ba557980-390e-11e9-8ad6-dd7bc4678125.png> [image: image] <https://user-images.githubusercontent.com/28173/53375274-a65e4780-390f-11e9-849b-19debe6fdfd0.png> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3292 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARp1SY_QlPUTqbRRl_q6F3_Z8APtvAuSks5vRG22gaJpZM4aGctP> .

[gnprice]

Thanks for the reply! Just to copy to this thread from our PM discussion yesterday:

• Any time I ask a "why" question about a PR, we'll want the answer to make it into the actual commits. Or if I find something confusing, we'll want it to be clear in the actual commits -- ideally the code itself make it clear, or if not then comments or the commit messages explaining.

  So e.g. your answer about Android vs. iOS is a perfect example -- that's great information to put in the commit message to explain what's going on.

• There's a question I asked that I'm still curious about:

  We already have a safeAreaInsets mechanism which we use in some places; should we use that here instead of SafeAreaView?

  And then you answered that in our PM thread -- that answer is also prime information to explain in the commit message 🙂

• One other, followup question: you said we don't need this on Android because Android already keeps us out of the status bar.

  But there are several possible kinds of notch, at least that you can test in developer mode (with the setting I posted a screenshot of). Including having one at the bottom, and having an extra-tall one at the top. Do things already work fine on those?

  Probably screenshots will be helpful in explaining exactly how the app behaves in those cases.

[borisyankov]

Related: #3067

SafeAreaView is iOS-specific.

The extra-messy thing is that iPhones do not have physical keys anymore, so they not only need offset for the notch (optionally) but for the UI element at the bottom that acts as Home button.

Android does have this:
https://developer.android.com/guide/topics/display-cutout

The Andoird story is simpler and we barely care about the notch.
We probably want to set LAYOUT_IN_DISPLAY_CUTOUT_MODE_NEVER and that should be enough.

[jainkuniya]

The extra-messy thing is that iPhones do not have physical keys anymore, so they not only need offset for the notch (optionally) but for the UI element at the bottom that acts as Home button.

In my phone too there are are no physical buttons at bottom, edges are curved little bit (one plus six).
But simpler than iOS 😝

We probably want to set LAYOUT_IN_DISPLAY_CUTOUT_MODE_NEVER and that should be enough.

We don't really need to set because by default we are only allowed to paint in safe areas.
https://developer.android.com/guide/topics/display-cutout#default_behavior

[jainkuniya]

Android screenshots: (in order)

• corner display cutout
• double display cutout
• tall display cutout
  
  
  

[jainkuniya]

@gnprice Updated 👍

commit 6b26ab6 (HEAD -> issue-3291, origin/issue-3291)
Author: Vishwesh Jainkuniya gitvishwesh@gmail.com
Date: Thu Jan 17 23:34:36 2019 +0530

ios, lightbox: Fix `SafeAreaInsets` padding is not applied.

Wrap screen content with `SafeAreaView` from `react-native` instead of
`View`.

In all other screens we are manually handeling safe area by applying
margins/padding. This custom handeling is dependent on third party
dependency to calculate offsets. Now RN also comes up with this new
component `SafeAreaView`. Plan is to use it instead of handeling
manually across the app, and remove external dependency.

This issue of safe area is only for iOS, becuase by defualt it allows us
to paint whole screen including notch area and bottom.

On Android by defualt we are not allowed to paint statusbar (notch area)
so this is not a issue there.

I have tested on Android by simulating all types of display cutout
present in the developer option, and works fine even without this
change.

References: https://facebook.github.io/react-native/docs/safeareaview
https://developer.android.com/guide/topics/display-cutout#default_behavior

Fixes: #3291

[gnprice]

Cool, that Android docs link is very helpful.

I agree that the default behavior on Android is good for us, and we don't need LAYOUT_IN_DISPLAY_CUTOUT_MODE_NEVER. @borisyankov the docs are pretty confusing here -- they say on the one hand

[In] the default behavior [...] [c]ontent renders into the cutout area while in portrait mode

which sounds bad... but then later they say

By default, in portrait mode with no special flags set, the status bar on a device with a cutout is resized to be at least as tall as the cutout, and your content displays in the area below.

which sounds perfectly fine, and matches @jainkuniya 's screenshots above that show our current behavior (under that default).

I think what's happening is that in the earlier quotation, "content" includes the status bar (which after all we do have some control over). We don't want our actual content rendering into the cutout area, but it's fine that the status bar is there.

[gnprice]

(In those screenshots, is there a duplicate? I see two that both look like double cutouts, and none that look like a tall cutout.)

[jainkuniya]

This is for tall display cut (there was issue while uploading files :()

[gnprice]

Copying from a question I asked @jainkuniya in chat the other day:

You wrote

In all other screens we are manually handeling safe area by applying margins/padding

and earlier you wrote

Other screen uses Screen component, so it is taken care by it :)

But it takes some work to go and confirm that that's really true. After all, there are a lot of screens in the app! Do we really use Screen on all of them... with this one exception? Where there's one exception, there are often more exceptions.

It turns out in fact this is not 100% true. 😛 You replied in chat with some more details to help explain why this is nevertheless the right change -- please reply here with those details, and then we can discuss.

[jainkuniya]

Hey, I have updated in the commit message.

In all other screens we are manually handeling safe area by applying
margins/padding as we are using `Screen`. In 4 screens we aren't
using `Screen`:

$ git ls-files src/ | grep Screen | xargs grep -wL Screen
- src/chat/ChatScreen.js
We have custom handeling here, see ComposeBox and ChatScreen.

- src/lightbox/LightboxScreen.js
This change will take care for this as well.

- src/main/MainScreenWithTabs.js
`TabBarBottom` is taking care for this
see https://github.com/react-navigation/react-navigation/blob/v1.5.11/src/views/TabView/TabBarBottom.js#L234

- src/start/CompatibilityScreen.js
- src/start/LoadingScreen.js
For both the above screens, content is very less and aligned vertically
 center, so no scope for content touching top/bottom.

All This custom handeling is dependent on third party
dependency to calculate offsets. Now RN also comes up with this new
component `SafeAreaView`. Plan is to use it instead of handeling
manually across the app, and remove external dependency.

This issue of safe area is only for iOS, becuase by defualt it allows us
to paint whole screen including notch area and bottom.

On Android by defualt we are not allowed to paint statusbar (notch area)
so this is not a issue there.

I have tested on Android by simulating all types of display cutout
present in the developer option, and works fine even without this
change.

References: https://facebook.github.io/react-native/docs/safeareaview
https://developer.android.com/guide/topics/display-cutout#default_behavior

[gnprice]

Thanks @jainkuniya for all your work on this!

After reading this explanation, and then rereading recently #3067, I've determined the plan I want to take is to merge #3067. That will move us systematically onto SafeAreaView, so we don't have a mixture of two different ways to do the same thing. (Which would tend to be super confusing for people, including me 😉 , trying to understand exactly how the code works.) It also answers a lot of the questions I was asking above, some of them by fixing bugs where in fact we weren't correctly handling the notch on some other screens, i.e. #3066 .