Skip to content

lightbox: Fix lack of access to close button on some iOS devices.#4268

Closed
chrisbobbe wants to merge 2 commits intozulip:masterfrom
chrisbobbe:pr-safe-area
Closed

lightbox: Fix lack of access to close button on some iOS devices.#4268
chrisbobbe wants to merge 2 commits intozulip:masterfrom
chrisbobbe:pr-safe-area

Conversation

@chrisbobbe
Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe commented Sep 23, 2020

An instance of #3066.

Even though we've been passing the hidden prop for the lightbox's
ZulipStatusBar since the beginning (3f8ad4a), evidently it
sometimes still appears. I don't have a clear answer for why, but I
suspect it might have to do with a particular subtlety in
react-navigation. From their docs [1]:

"""
If you're using a tab or drawer navigator, it's a bit more complex
because all of the screens in the navigator might be rendered at
once and kept rendered - that means that the last StatusBar config
you set will be used (likely on the final tab of your tab navigator,
not what the user is seeing).
"""

We do use a tab navigator (MainTabs), so this isn't implausible on
its face.

When the status bar appears, it's been causing #4267: the close
button appears behind the status bar.

The ZulipStatusBar component has been conscripted into doing part
of the work of React Native's [2], or React Navigation's [3],
SafeAreaView component, which we haven't started using yet. (#3067
is open for using it all over the app.) In particular, as long as
the hidden prop is true, a View with the height of the top inset
of the safe area (wrapping the status bar) prevents the rest of the
screen's content from "unsafely" rendering in that area.

When this screen's ZulipStatusBar has its hidden prop passed as
true, however, it doesn't defend the safe-area view at the top,
whether or not a status bar is actually showing. That's because the
wrapping View gets its height set to zero in the hidden case.

So, without answering why a status bar might actually be showing
when we tell it not to, remove the wrapping View's height
difference between hidden being true and false, conceding that
ZulipStatusBar should be the defender of the top safe area, and in
particular that it should still do so when it's been marked as
hidden. At least until we move on the sweeping changes of #3067.

Also, we've got a sliding animation for the header, and the distance
it needs to travel has increased by the height of the safe area. So,
account for that by adding safeAreaInsets.top to NAVBAR_SIZE in
the appropriate place. (Without that addition, the header just
retreats into the top inset instead of leaving the screen entirely.)

[1] https://reactnavigation.org/docs/4.x/status-bar/#tabs-and-drawer
[2] https://reactnative.dev/docs/0.62/safeareaview
[3] https://reactnavigation.org/docs/4.x/handling-iphonex/

Fixes: #4267

Even though we've been passing the `hidden` prop for the lightbox's
ZulipStatusBar since the beginning (3f8ad4a), evidently it
sometimes still appears. I don't have a clear answer for why, but I
suspect it might have to do with a particular subtlety in
react-navigation. From their docs [1]:

"""
If you're using a tab or drawer navigator, it's a bit more complex
because all of the screens in the navigator might be rendered at
once and kept rendered - that means that the last `StatusBar` config
you set will be used (likely on the final tab of your tab navigator,
not what the user is seeing).
"""

We do use a tab navigator (`MainTabs`), so this isn't implausible on
its face.

When the status bar appears, it's been causing zulip#4267: the close
button appears behind the status bar.

The `ZulipStatusBar` component has been conscripted into doing part
of the work of React Native's [2], or React Navigation's [3],
`SafeAreaView` component, which we haven't started using yet. (zulip#3067
is open for using it all over the app.) In particular, as long as
the `hidden` prop is true, a `View` with the height of the top inset
of the safe area (wrapping the status bar) prevents the rest of the
screen's content from "unsafely" rendering in that area.

When this screen's `ZulipStatusBar` has its `hidden` prop passed as
`true`, however, it doesn't defend the safe-area view at the top,
whether or not a status bar is actually showing. That's because the
wrapping `View` gets its height set to zero in the `hidden` case.

So, without answering why a status bar might actually be showing
when we tell it not to, remove the wrapping `View`'s height
difference between `hidden` being true and false, conceding that
`ZulipStatusBar` should be the defender of the top safe area, and in
particular that it should still do so when it's been marked as
`hidden`. At least until we make the sweeping changes of zulip#3067.

Also, we've got a sliding animation for the header, and the distance
it needs to travel has increased by the height of the safe area. So,
account for that by adding `safeAreaInsets.top` to `NAVBAR_SIZE` in
the appropriate place. (Without that addition, the header just
retreats into the top inset instead of leaving the screen entirely.)

There are no other places where we pass `hidden` as `true` for
`ZulipStatusBar`, so changing its behavior in that situation
shouldn't have any nasty side effects.

[1] https://reactnavigation.org/docs/4.x/status-bar/#tabs-and-drawer
[2] https://reactnative.dev/docs/0.62/safeareaview
[3] https://reactnavigation.org/docs/4.x/handling-iphonex/

Fixes: zulip#4267
@chrisbobbe
Copy link
Copy Markdown
Contributor Author

Tested on iOS (iPhone 6s physical device and iPhone XR simulator) and Android; looks fine.

@chrisbobbe chrisbobbe marked this pull request as draft January 26, 2021 01:36
@chrisbobbe
Copy link
Copy Markdown
Contributor Author

Converting this to a draft as I work on a more thorough solution for #3066.

@gnprice
Copy link
Copy Markdown
Member

gnprice commented Jan 26, 2021

Hmm, I neglected to look at this PR sooner, oops. >_>

This looks like a fine workaround, and the testing you did sounds like it should be good for confirming it didn't accidentally break anything. (I see that LightboxScreen is the only place we pass that hidden prop to ZulipStatusBar, so the lightbox is the only screen it could potentially affect.) I'm not particularly convinced that the logic around that hidden ? 0 : … conditional is all that coherent or correct in the first place.

If you think this would make your eventual more thorough fix more complicated, then holding off is a good strategy. But if not, then it probably makes sense to go ahead and merge this fix now.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 27, 2021
…g in.

This fixes the reportedly most annoying part of zulip#4268. In the next
commit, we'll also horizontally pad the contents of the header and
footer so they don't stray outside the safe areas when in a
landscape orientation.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jan 27, 2021
@chrisbobbe
Copy link
Copy Markdown
Contributor Author

chrisbobbe commented Jan 27, 2021

If you think this would make your eventual more thorough fix more complicated, then holding off is a good strategy.

Sure, makes sense. I also discovered a few things that this PR neglected. 🙂

Closing as superseded by #4440.

@chrisbobbe chrisbobbe closed this Jan 27, 2021
@chrisbobbe chrisbobbe deleted the pr-safe-area branch November 5, 2021 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to close image preview dialog on iphone

2 participants