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

Safe areas are not handled correctly on iPhone X #3066

Open
borisyankov opened this issue Oct 20, 2018 · 8 comments · May be fixed by #4893
Open

Safe areas are not handled correctly on iPhone X #3066

borisyankov opened this issue Oct 20, 2018 · 8 comments · May be fixed by #4893

Comments

@borisyankov
Copy link
Contributor

borisyankov commented Oct 20, 2018

We are currently handling, but not 100% correctly, the 'screen border offsets' known as 'safe areas' on iOS. One such bug: iPhone X with a popped keyboard has some extra space below the compose box

borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue Oct 21, 2018
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue Oct 21, 2018
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue Oct 21, 2018
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
@borisyankov borisyankov self-assigned this Oct 22, 2018
@borisyankov borisyankov changed the title Safe areas are not handled correctly on iPhone X and Androids with notches Safe areas are not handled correctly on iPhone X Oct 22, 2018
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue Oct 24, 2018
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue Nov 2, 2018
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
@gnprice
Copy link
Member

gnprice commented Dec 3, 2018

Blocked on the React Navigation v2 upgrade #2702 .

@gnprice gnprice added the blocked on other work To come back to after another related PR, or some other task. label Dec 3, 2018
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue May 29, 2019
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue May 29, 2019
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue May 29, 2019
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue May 30, 2019
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
@gnprice
Copy link
Member

gnprice commented Jun 4, 2019

@jackrzhang provided (on #3389) some handy screenshots that very clearly demonstrate one of the symptoms of this issue:

On any iPhone X model, the upper edge of the screen covers part of the application UI in landscape orientation:

[...]

I don't have a physical iPhone X device to use for testing, so it's also possible that this reproduces only on a simulator.

@gnprice
Copy link
Member

gnprice commented Jun 4, 2019

I think #3370 is probably also a symptom, indirectly:

[From a video by @armaanahluwalia to demonstrate a different issue:]

A blank white band between the compose box and the keyboard.

Didn't repro for me on Android, and @jackrzhang reported it repro'd for him on a simulated iPhone XR but not on an iPhone 7. So that's consistent with it happening only on iOS devices that have a display cutout.

I suspect it's caused by the logic we have for attempting to avoid the cutout, i.e. by a bug in that logic.

@gnprice
Copy link
Member

gnprice commented Jun 6, 2019

Additional info from the dupe issue #3064 (filed just before this one), quoting @borisyankov:

When iPhone X was released, new functionality to iOS was added to indicate safe area - to help with apps handling optional notches, rounded corners or the ...

A good article explaining all this here:
https://medium.com/rosberryapps/ios-safe-area-ca10e919526f

We started using a component react-native-safe-area to handle this.

It isn't working perfectly and we need to fix it.
Testing on iOS X or Simulator with it makes few layout issues obvious.

Better yet, use the now built-in component:
https://facebook.github.io/react-native/docs/safeareaview

@gnprice gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Aug 12, 2019
chrisbobbe pushed a commit to borisyankov/zulip-mobile that referenced this issue Sep 23, 2020
Fixes zulip#3066

Previous approach almost worked correctly, but there was one
case it didn't - when the keyboard did pop on an iPhone, the
bottom space was sill there, though it shouldn't, as the keyboard
is located over the `Home Indicator`.
@chrisbobbe
Copy link
Contributor

#4315 would have us using a more maintained thing for getting the safe-area insets.

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 7, 2021

OK, now that #4315 is merged, we're using react-native-safe-area-context instead of react-native-safe-area to provide the safe-area insets. We've stopped storing the values in Redux; the new module uses React Context.

As discussed around here, the new module is not without flaws, though.

It offers a useSafeAreaInsets hook and a withSafeAreaInsets HOC, which will provide the insets in a live-updating way, so that the values are even correct when switching between screen orientations. There are some hiccups in that live-updating, though; see #4315 (comment) for some observations.

The preferred solution to try will be to try to use SafeAreaView, a React component provided by the module, instead of useSafeAreaInsets/withSafeAreaInsets. It's specifically recommended because it doesn't have any flickers when changing screen orientations.

If that doesn't work and we have to use useSafeAreaInsets/withSafeAreaInsets, we'll need to do some debugging about those flickers.

@chrisbobbe
Copy link
Contributor

I plan to make a focused PR to fix this issue after #4468 is in.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 12, 2021
Each of these UI elements is a row that has meaningful content that
we need to keep within the safe areas, but the rest of the row (its
distinct background color, its touchable area, etc.) is meant to
extend through the insets to the extreme left and right of the
screen. See example screenshots at
  zulip#4683 (comment).

So, make that happen. We do so by giving the rows left and right
padding. This is easy and makes the row elements' styles pretty
intuitive, but it does mean we'll have to take care, in the next few
commits, not to add any padding to the elements that contain the
rows -- we don't want to double up the padding by mistake.

(This requirement ends up being kind of annoying on screens that
have a mix of these kinds of rows and regular elements. Possibly
there's a better design pattern we haven't yet considered.)

Related: zulip#3066
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 12, 2021
This isn't ideal; it just puts the whole message list in a box
that's contained within the safe area. The message list was designed
to span the entire width of the screen, and some things look pretty
bad when the insets are nonzero [1]: the timestamp pills, for one
example, look like they get cut off artificially instead of being
tucked away somewhere offscreen. Things like that would best be
solved with a more in-depth design discussion.

A more complete solution might start with the "env" CSS function
[2], or, less optimally, a new WebView inbound event to be fired
whenever the insets change.

At least this solution lets things be visible and interactable that
weren't before.

I think this completes zulip#3066 (modulo this message-list issues
discussed here), which is our current audit for safe-area handling.

[1] So far, I think this is just landscape mode on newer iOS
    devices, but I might be missing something.

[2] https://webkit.org/blog/7929/designing-websites-for-iphone-x/

Fixes: zulip#3066
@chrisbobbe chrisbobbe linked a pull request Jul 12, 2021 that will close this issue
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 16, 2021
Each of these UI elements is a row that has meaningful content that
we need to keep within the safe areas, but the rest of the row (its
distinct background color, its touchable area, etc.) is meant to
extend through the insets to the extreme left and right of the
screen. See example screenshots at
  zulip#4683 (comment).

So, make that happen. We do so by giving the rows left and right
padding. This is easy and makes the row elements' styles pretty
intuitive, but it does mean we'll have to take care, in the next few
commits, not to add any padding to the elements that contain the
rows -- we don't want to double up the padding by mistake.

(This requirement ends up being kind of annoying on screens that
have a mix of these kinds of rows and regular elements. But this is
unfortunately the workable design we've found; see our architecture
doc on handling safe areas.)

Related: zulip#3066
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 16, 2021
This isn't ideal; it just puts the whole message list in a box
that's contained within the safe area. The message list was designed
to span the entire width of the screen, and some things look pretty
bad when the insets are nonzero [1]: the timestamp pills, for one
example, look like they get cut off artificially instead of being
tucked away somewhere offscreen. Things like that would best be
solved with a more in-depth design discussion.

A more complete solution might start with the "env" CSS function
[2], or, less optimally, a new WebView inbound event to be fired
whenever the insets change.

At least this solution lets things be visible and interactable that
weren't before.

I think this completes zulip#3066 (modulo this message-list issue
discussed here), which is our current audit for safe-area handling.

[1] So far, I think this is just landscape mode on newer iOS
    devices, but I might be missing something.

[2] https://webkit.org/blog/7929/designing-websites-for-iphone-x/

Fixes: zulip#3066
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
Each of these UI elements is a row that has meaningful content that
we need to keep within the safe areas, but the rest of the row (its
distinct background color, its touchable area, etc.) is meant to
extend through the insets to the extreme left and right of the
screen. See example screenshots at
  zulip#4683 (comment).

So, make that happen. We do so by giving the rows left and right
padding. This is easy and makes the row elements' styles pretty
intuitive, but it does mean we'll have to take care, in the next few
commits, not to add any padding to the elements that contain the
rows -- we don't want to double up the padding by mistake.

(This requirement ends up being kind of annoying on screens that
have a mix of these kinds of rows and regular elements. But this is
unfortunately the workable design we've found; see our architecture
doc on handling safe areas.)

Related: zulip#3066
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 29, 2021
This isn't ideal; it just puts the whole message list in a box
that's contained within the safe area. The message list was designed
to span the entire width of the screen, and some things look pretty
bad when the insets are nonzero [1]: the timestamp pills, for one
example, look like they get cut off artificially instead of being
tucked away somewhere offscreen. Things like that would best be
solved with a more in-depth design discussion.

A more complete solution might start with the "env" CSS function
[2], or, less optimally, a new WebView inbound event to be fired
whenever the insets change.

At least this solution lets things be visible and interactable that
weren't before.

I think this completes zulip#3066 (modulo this message-list issue
discussed here), which is our current audit for safe-area handling.

[1] So far, I think this is just landscape mode on newer iOS
    devices, but I might be missing something.

[2] https://webkit.org/blog/7929/designing-websites-for-iphone-x/

Fixes: zulip#3066
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
Each of these UI elements is a row that has meaningful content that
we need to keep within the safe areas, but the rest of the row (its
distinct background color, its touchable area, etc.) is meant to
extend through the insets to the extreme left and right of the
screen. See example screenshots at
  zulip#4683 (comment).

So, make that happen. We do so by giving the rows left and right
padding. This is easy and makes the row elements' styles pretty
intuitive, but it does mean we'll have to take care, in the next few
commits, not to add any padding to the elements that contain the
rows -- we don't want to double up the padding by mistake.

(This requirement ends up being kind of annoying on screens that
have a mix of these kinds of rows and regular elements. But this is
unfortunately the workable design we've found; see our architecture
doc on handling safe areas.)

Related: zulip#3066
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 13, 2021
This isn't ideal; it just puts the whole message list in a box
that's contained within the safe area. The message list was designed
to span the entire width of the screen, and some things look pretty
bad when the insets are nonzero [1]: the timestamp pills, for one
example, look like they get cut off artificially instead of being
tucked away somewhere offscreen. Things like that would best be
solved with a more in-depth design discussion.

A more complete solution might start with the "env" CSS function
[2], or, less optimally, a new WebView inbound event to be fired
whenever the insets change.

At least this solution lets things be visible and interactable that
weren't before.

I think this completes zulip#3066 (modulo this message-list issue
discussed here), which is our current audit for safe-area handling.

[1] So far, I think this is just landscape mode on newer iOS
    devices, but I might be missing something.

[2] https://webkit.org/blog/7929/designing-websites-for-iphone-x/

Fixes: zulip#3066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants