Handle safe areas (mostly). #4893
Conversation
There was a problem hiding this comment.
Thanks @chrisbobbe ! Excited to be closing this out soon.
I think an approach like in that commit is a good enough 80% solution, and getting the message list quite right looks like a bigger project that we don't really have time for right now.
Yeah, I agree.
Separately: it looks like this PR is entirely concerned with the left and right insets. What about the top and bottom? Are those already all handled?
One known symptom that seems probably caused by this is #3066 (comment) . Hmm, and in fact I reproduce that now on an iPhone XR (on v27.162 -- I should update that but I don't think we've merged any related changes since then -- and iOS 14.6.) The symptom is a little subtler now (#3370 (comment) ) than in that original screenshot, but still there.
OK, and that's the only vertical-related symptom I'm finding. All the top and bottom bars successfully avoid the insets with no glitches, including on the lightbox. I guess the lightbox part was #4442 🙂
src/chat/UnreadNotice.js
Outdated
| text: `{unreadCount, plural, | ||
| =0 {No unread messages} | ||
| =1 {# unread message} | ||
| other {# unread messages} | ||
| }`, |
There was a problem hiding this comment.
This makes sense, but I'm concerned about just handing this string to our translators. In order to do a successful job translating it, they really need to learn this syntax to some extent -- in particular, they need to learn which words to translate (-> the parts in the inner braces, but not "unreadCount", or "plural", or "other"), and they need to learn to use "two" or "few" etc. if their language has those categories. That's pretty complicated; we'll want to be sure translators get the guidance they need when they're trying to handle it.
Perhaps start a chat thread on #translation?
And in the meantime, to unblock the work that brought it up in this PR: can use _ directly in the render method, and pass the result to a RawLabel.
There was a problem hiding this comment.
This makes sense, but I'm concerned about just handing this string to our translators.
Oh, hmm. I guess I thought Transifex could parse the syntax and have a nice UI for getting it translated correctly. Thinking again, I don't think I've seen a special UI for the simpler uses of the syntax, like Uploading {fileName}..., so they're not likely to have one for the more complex ones like this. Hmm.
There was a problem hiding this comment.
src/chat/UnreadNotice.js
Outdated
| safeAreaWrapper: { | ||
| flex: 1, | ||
| flexDirection: 'row', | ||
| }, |
There was a problem hiding this comment.
It looks like the outer container, which used to contain this new element's children directly, has justifyContent: 'space-between'. Do we need that here?
Also we can probably delete that style prop, and the flexDirection, from the outer container's style props because it now has just the one child and so those don't mean anything.
src/chat/UnreadNotice.js
Outdated
| unreadTextWrapper: { | ||
| flex: 1, | ||
| flexDirection: 'row', | ||
| alignItems: 'center', |
There was a problem hiding this comment.
Similarly this probably no longer needs the inward-facing layout props, as it only has one child. (That's from the first commit, replacing the two labels with one.)
Hmm, in fact maybe this wrapper view is no longer needed at all?
| <SafeAreaView | ||
| mode="padding" | ||
| edges={['right', 'left']} |
There was a problem hiding this comment.
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.
So I think this isn't actually a problem: I think if you make a SafeAreaView at a place where it's actually completely inside the safe area -- for example, because it's nested other SafeAreaViews that provided the appropriate padding on each side -- then it has no effect.
This isn't clearly described in the react-native-safe-area-context package's doc. But when I look at e.g. the Android implementation, it's making calculations that are designed to do that:
https://github.com/th3rdwave/react-native-safe-area-context/blob/cd8dd60d035a44c22459b2c890e6512e5796396e/android/src/main/java/com/th3rdwave/safeareacontext/SafeAreaUtils.java#L51
On iOS... hmm actually the implementation kind of looks like it's doing the wrong thing here:
https://github.com/th3rdwave/react-native-safe-area-context/blob/cd8dd60d035a44c22459b2c890e6512e5796396e/ios/SafeAreaView/RNCSafeAreaView.m#L76
But using the appropriate iOS API method:
https://developer.apple.com/documentation/uikit/uiview/2891102-safearealayoutguide
it would definitely get the right behavior:
For [] views in the view hierarchy [other than the view controller's root view], the layout guide reflects only the portion of the view that is covered by other content. For example, if a view is entirely within the safe area of its superview, the layout guide edges are equal to the edges of the view.
There was a problem hiding this comment.
In any case, if that isn't actually a problem, then that is quite a good thing! Because it seems like in general it'd be tough to ensure that a given element either always did, or never did, appear in a position where it should be occupying the inset area. For example it's not totally clear to me that that's the case for this SectionHeader component.
And if it is empirically a problem... then that seems like a pretty fundamental bug in react-native-safe-area-context. Particularly on iOS, where the system's API is straightforward and clearly documented to do the right thing.
If you're finding that that's the case, please start a thread on #mobile-team and we'll discuss in more detail. We should probably still proceed with this PR, because even a buggy library (plus any layout glitches we get from missing something as we try to maintain this style of carefully covering each part of the tree just once) is probably better than the status quo… but we should also consider rewriting the relevant parts of the library.
| </SafeAreaView> | ||
| <SwitchRow |
There was a problem hiding this comment.
This one is a great example of where it'd be real helpful to be able to nest SafeAreaViews and not worry about it -- that is, and have the inner ones be no-ops.
The SwitchRow isn't doing anything interesting by handling the left and right padding, right? That is, it's just extending its own background -- which is the same as the background of the parent. So if we did put a SafeAreaView around this whole screen, and if that did cause the inner SafeAreaViews to be no-ops like they should be, then that would have the exact same effect as this trickier thing.
src/main/HomeScreen.js
Outdated
| </SafeAreaView> | ||
| <ServerCompatBanner /> | ||
| <LoadingBanner /> | ||
| <UnreadCards /> |
There was a problem hiding this comment.
OTOH this is an example of where we'd still need several separate SafeAreaViews on inner components, and wouldn't want to handle the padding all at once at the outside. That's because the top icon bar, these banners, and the stream headings in the unreads list, all want to separately extend to the side edges.
| // TODO: Keep the meaningful content within the safe areas, but | ||
| // let the background elements extend through the insets all the | ||
| // way to the left and right of the screen. | ||
| <SafeAreaView mode="padding" edges={['right', 'left']} style={{ flex: 1 }}> |
There was a problem hiding this comment.
Yeah, this seems like a fine compromise in that this is a big improvement over the status quo.
The timestamp pills are probably the part that comes out ugliest here. But at least they are cut off -- so it looks like a normal layout that got rendered into an awkwardly letterboxed viewport, rather than a layout that's just broken.
As Greg suggests at zulip#4893 (comment).
chrisbobbe
force-pushed
the
pr-safe-areas
branch
from
4424eb8 to
001f01c
Compare
|
Thanks for the review! Revision pushed, following our long discussion on CZO. Please feel free to rework or comment a lot on the new architecture doc; I was hoping to provide enough detail while still making it easy to see what the new requirements are, but I think it might be kind of a wall of text in this draft. I've included a fix for #3370, commented on a small layout issue with autocomplete, and changed the |
As Greg suggests at zulip#4893 (comment).
chrisbobbe
force-pushed
the
pr-safe-areas
branch
from
001f01c to
3f737f3
Compare
|
I've just rebased, resolving tons of conflicts from the recent work converting to Hooks components—hopefully I didn't mess anything up; I don't think I did. 🙂 |
The logo should be centered with respect to the entire screen, not the safe area (or worse, as it's been doing, the safe area plus three insets). It's also not big enough to risk entering the insets on any real screen, so it's fine to ignore the insets here. Also, this was using `useSafeAreaInsets`, which we try to avoid.
The new message takes further advantage of ICU Message syntax than we have before; see https://formatjs.io/docs/core-concepts/icu-syntax/. Now, translators have more complete information about the text being translated, and will be able to give better, more locale-appropriate translations. This might cause confusion if Transifex doesn't give the user a nice UI; see discussion at https://chat.zulip.org/#narrow/stream/58-translation/topic/ICU.20Message.20syntax/near/1230242. This will also be nice for the layout because the text will be in just one element, not split between two.
We'll tidy this up a bit in the next commit by removing the inner `View` (the one that uses the `unreadTextWrapper` styles).
As Greg suggests at zulip#4893 (comment).
In the next commit, note the TODOs in `EmojiAutocomplete`, `PeopleAutocomplete`, and `StreamAutocomplete` (`TopicAutocomplete isn't affected) for not double-handling these insets, which we do by inappropriately padding the item rows. The padding comes from some reusable components whose other callsites *do* need the padding. As the TODOs say, we should probably stop reusing those components, and make new ones. The use-cases are different enough that this probably makes sense to do in principle anyway. But another reason not to reuse them is that it'd just be hard to deal with this issue with the insets. `SafeAreaView` doesn't make it easy to conditionally handle no edges or some edges; see AppAndFlow/react-native-safe-area-context#204.
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
These screens don't currently have any screen-spanning "row" elements (see the previous commit for what those are), so we can just put the entire contents of each screen in a padding SafeAreaView.
Tricky because the contents of these screens are mixed between screen-spanning "row" elements (see a recent commit for what those are) and regular elements. These are all examples of "adjusting the wrapper's other children as necessary to compensate", per our rules for handling safe areas in the new doc added at the start of this series: > If a React component is meant to occupy any insets, then its > layout parent should adapt to allow it to do so, so that the inset > distance isn't covered twice. This generally means omitting a > padding `SafeAreaView` wrapper from the parent, if present, > putting one in the child, and adjusting the wrapper's other > children as necessary to compensate (e.g., by giving them > `SafeAreaView`s, preferably with margin instead of padding, to > show they don't care what fills the space). Each component touched in this commit is a "parent" in that sense, and has a jsdoc explaining which of its descendents make it necessary for the component to occupy insets.
As recommended by react-native-safe-area-context to avoid flickering when the orientation changes.
For landscape mode.
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
force-pushed
the
pr-safe-areas
branch
from
3f737f3 to
7d53cd3
Compare
Not yet perfect because some languages have categories for "two" or "few", etc., as Greg points out; see zulip#4893 (comment) So, start an experiment to see if we can do better, using ICU syntax. The ICU syntax is likely to be a barrier to translators if they have to deal with it directly. It's confusing, and more complicated than the other strings they're used to working with. Transifex has some documentation that suggests it might offer a special UI that helps the user along, if ICU syntax is used correctly, but we haven't seen such a UI in practice yet. So, add an entry to messages_en.json that Transifex will hopefully like (but don't use the message in the UI yet, just in case not). The entry will be uploaded to Transifex next time we do a sync, and then we'll see.
Not yet perfect because some languages have categories for "two" or "few", etc., as Greg points out; see zulip#4893 (comment) So, start an experiment to see if we can do better, using ICU syntax. The ICU syntax is likely to be a barrier to translators if they have to deal with it directly. It's confusing, and more complicated than the other strings they're used to working with. Transifex has some documentation that suggests it might offer a special UI that helps the user along, if ICU syntax is used correctly, but we haven't seen such a UI in practice yet. So, add an entry to messages_en.json that Transifex will hopefully like (but don't use the message in the UI yet, just in case not). The entry will be uploaded to Transifex next time we do a sync, and then we'll see.
Not yet perfect because some languages have categories for "two" or "few", etc., as Greg points out; see zulip#4893 (comment) So, start an experiment to see if we can do better, using ICU syntax. The ICU syntax is likely to be a barrier to translators if they have to deal with it directly. It's confusing, and more complicated than the other strings they're used to working with. Transifex has some documentation that suggests it might offer a special UI that helps the user along, if ICU syntax is used correctly, but we haven't seen such a UI in practice yet. So, add an entry to messages_en.json that Transifex will hopefully like (but don't use the message in the UI yet, just in case not). The entry will be uploaded to Transifex next time we do a sync, and then we'll see.
Not yet perfect because some languages have categories for "two" or "few", etc., as Greg points out; see zulip#4893 (comment) So, start an experiment to see if we can do better, using ICU syntax. The ICU syntax is likely to be a barrier to translators if they have to deal with it directly. It's confusing, and more complicated than the other strings they're used to working with. Transifex has some documentation that suggests it might offer a special UI that helps the user along, if ICU syntax is used correctly, but we haven't seen such a UI in practice yet. So, add an entry to messages_en.json that Transifex will hopefully like (but don't use the message in the UI yet, just in case not). The entry will be uploaded to Transifex next time we do a sync, and then we'll see.
After #4683.
The last commit mentions some shortcomings with the message list; screenshots below. But I think an approach like in that commit is a good enough 80% solution, and getting the message list quite right looks like a bigger project that we don't really have time for right now.
Once the rest of the work looks good, I'd like to add something to our style guide for how we'd like to maintain safe-area handling in the future.
I think the first commit ("i18n: Allow better translations for "# unread message(s)".") could be an independent cleanup, so I can move that to another PR if we want.
Message list after this PR (dark and light mode):
Fixes: #3066