Clean up navigation logic (4/x): Tidy up our use of ZulipStatusBar a bit more.

[chrisbobbe]

I've removed the work toward using React Navigation's header option in this revision, and changed the title.

Toward: #3066.

---

This PR is the next in the series started by #4428, #4440, and #4443.

Here, we use the header option—for the first time!—for ChatNavBar, as part of the safe-area-handling pattern I described at #4443 (comment).

I'm a bit disappointed to see that Flow isn't tracking what route we're on when we use the header option, so I have to suppress it when it says it's suspicious about grabbing specific route params to give to ChatNavBar. I'm not sure how easy this would be to fix. With React Navigation v5's component-based API, I'm not sure if it's a case where we'd need to ask more from the React types than is reasonable.

Also, on iOS, we start showing and hiding the status bar as the lightbox header/footer slide in and out, as Greg mentioned wanting to do at #4440 (comment).

I think I worked out something odd in how RN's KeyboardAvoidingView works, and I made a note on that.

[chrisbobbe]

• ZulipMobile [nfc]: Use just one ActionSheetProvider, here.
• ChatScreen [nfc]: Remove unnecessary wrapping View.

I made these two commits before I quite figured out what was going on with the KeyboardAvoidingView, so I expect they can freely be dropped if they don't seem helpful. (If dropping them, we'd want to use our new understanding of KeyboardAvoidingView when we carefully resolve rebase conflicts that'd arise from dropping them.)

[chrisbobbe]

ChatScreen: Store `editMessage` as a nav param, not React state.

The fact that `ChatScreen`'s header changes with this bit of state
is a clue that it really wants to be a route param. When we start
using the `header` option [1] for the header, instead of smushing
the header in with the rest of the screen's content, it'll be
convenient to grab `editMessage` from the route params.

[1] https://reactnavigation.org/docs/stack-navigator/#header

We could aim to reduce the contents of editMessage, probably to just the ID of the message being edited, along the lines of what React Navigation recommends should go in these params.

I think the biggest drawback to storing the whole original content and topic equally applies in the status quo (i.e., where editMessage is stored in React state): we should be getting model data from react-redux, in a way that ensures that the UI live-updates. So, perhaps a fix can be done as a followup, or we can add a TODO. (Er, or actually it might be quite easy to do in this PR; I've just assumed it might be a bit complicated because of how tricky it was to work out what editMessage does in the first place.)

The article gives a couple of other good reasons, but some of them (like gracefully handling deep-linking) don't yet really apply to us. But I think their guidance that "You can also think of the route object like a URL" makes sense. For this screen, I'm thinking of the analogy of a URL that has something like ?msg-edit-id=12345.

[chrisbobbe]

In my draft that moves ModalSearchNavBar to the header, the search term is stored in the nav state. This seems fine; I'm thinking of the analogy of a URL that has something like ?q=foo.

Also, from the doc about what should go in params:

Some examples of what should be in params are:

• [...]
• Params for sorting, filtering data etc. when you have a list of items, e.g. navigation.navigate('Feeds', { sortBy: 'latest' })

[chrisbobbe]

In my draft that moves ModalSearchNavBar to the header

While at first I thought we might not want to keep the search bar in the header/nav bar, I think it's actually a fine place to put it, at least on Android.

From an Android doc:

The key functions of the app bar are as follows:

• [...]
• Access to important actions in a predictable way, such as search.

[gnprice]

We could aim to reduce the contents of editMessage, probably to just the ID of the message being edited

I think the thing that makes that tricky is mainly just the message content. If you look at where setEditMessage gets called (after getting renamed to startEditMessage along the way >_> -- this whole edit-message path is not a model of good software design), we have to fetch the raw Markdown content from the server at that point:

  const { raw_content } = await api.getRawMessageContent(auth, message.id);
  startEditMessage({
    id: message.id,
    content: raw_content,
    topic: message.subject,
  });

We don't have any cause to keep that information beyond the scope of one edit-message operation, so I think it is best kept as local state of one form or another. If we want the message ID in the nav state, I think the least awkward thing may be for the content and topic to go there along with it, as this PR currently does.

[gnprice]

While at first I thought we might not want to keep the search bar in the header/nav bar, I think it's actually a fine place to put it, at least on Android.

Yep, this is pretty classic in Material Design and on Android.

It's not really that foreign on iOS either, I think. Open up Safari, or Photos, and start searching -- the search box is at the top in basically an equivalent of the app bar.

[chrisbobbe]

I think the thing that makes that tricky is mainly just the message content.

Ahh—right, you're right. I saw that but forgot about it when commenting.

[chrisbobbe]

editMessage: null could go here, or it could go in AppNavigator where we define the screen (doc):

<Stack.Screen name="chat" component={ChatScreen} initialParams={{ editMessage: null }} />

[gnprice]

I think I prefer it being here rather than on the navigator. Ideally it really belongs with the component definition itself -- it's basically a default value for an optional parameter -- and this feels a bit closer to the component definition than the navigator does.

[chrisbobbe]

Just rebased and resolved some conflicts.

[gnprice]

Thanks @chrisbobbe ! Sorry for the delayed review.

Some comments below. Generally this looks good except I'm not convinced the final commit is a helpful direction. (And a couple of the prep commits may not be needed if we leave that one out.)

---

Specifically: this way of specifying the header seems rather more complicated than just invoking something like ChatNavBar at the top of the component's own rendered tree. Does this buy us something that's worth the complexity?

I'm looking back at #4443 (comment) , and I see this at the endpoint:

Each screen component on a stack navigator is responsible for keeping important content out of the right, bottom, and left insets. It doesn't have to worry about the top inset because its header does that, as mentioned above.

I think that means we'll still want to be handling right, bottom, and left insets somewhere generic like Screen for the sake of screens that don't have particular other needs, as well as handling all four sides in screen-specific ways where needed.

Given that, I don't see that we necessarily benefit from handling one of the four sides in a different way. If anything, it's probably better to handle all four together, or close by each other -- it helps in spotting gaps and understanding how things fit together.

... For example. Looking at Screen, it strikes me that we handle the bottom, and then the top (via Modal(Search)?NavBar), but don't handle the left and right. That seems like it's probably the cause of this symptom: #3066 (comment) (Assuming that symptom is still present -- my test iPhone XR is out of battery, just started it charging so I can confirm.) I guess not that exact symptom because those screenshots are from MainTabsScreen, but I'd predict from that code that we have that symptom when Screen is involved too.

[gnprice]

Hmm, good point!

As written, this is a bit hard to follow because it's in terms of the underlying StatusBar. That happens to be part of the implementation of this component, but this jsdoc should describe the interface, so it should be understandable without consulting the implementation.

Quite possibly the interface should now be in part something like "This renders to an RN StatusBar with appropriate props, and nothing else." That wasn't always how this component worked, but I think probably that was the wrong design and this one (which you introduced in a previous PR in this series) is a better one. If the jsdoc says that, then I think that provides the needed background for this material to make sense too.

[gnprice]

Hmm, interesting!

[gnprice]

I think I prefer it being here rather than on the navigator. Ideally it really belongs with the component definition itself -- it's basically a default value for an optional parameter -- and this feels a bit closer to the component definition than the navigator does.

[gnprice]

We could aim to reduce the contents of editMessage, probably to just the ID of the message being edited

I think the thing that makes that tricky is mainly just the message content. If you look at where setEditMessage gets called (after getting renamed to startEditMessage along the way >_> -- this whole edit-message path is not a model of good software design), we have to fetch the raw Markdown content from the server at that point:

  const { raw_content } = await api.getRawMessageContent(auth, message.id);
  startEditMessage({
    id: message.id,
    content: raw_content,
    topic: message.subject,
  });

We don't have any cause to keep that information beyond the scope of one edit-message operation, so I think it is best kept as local state of one form or another. If we want the message ID in the nav state, I think the least awkward thing may be for the content and topic to go there along with it, as this PR currently does.

[gnprice]

While at first I thought we might not want to keep the search bar in the header/nav bar, I think it's actually a fine place to put it, at least on Android.

Yep, this is pretty classic in Material Design and on Android.

It's not really that foreign on iOS either, I think. Open up Safari, or Photos, and start searching -- the search box is at the top in basically an equivalent of the app bar.

[gnprice]

This looks like a helpful explanation of what this prop means, for reference when using this component. I.e., it's part of explaining this component's interface.

To help make that clear and make it discoverable for that purpose, let's put it where the prop appears in the Props type definition, and format it as jsdoc. When it's tucked inside the render method like this, it looks like it's an implementation comment, and would be easy to miss when scanning this component for interface documentation.

[chrisbobbe]

it looks like it's an implementation comment

It sort of is an implementation comment, in that it explains a weirdness in some code that it calls as part of doing its job, right? If for some strange reason (😬) that weirdness were common (or easily obtained) knowledge, the component's jsdoc might do OK to just say "passes some props straight through to KeyboardAvoidingView", I suppose? I guess if it were well-known, the implementation comment wouldn't be needed either, hmm.

I agree that this is easy to miss where it currently is, so it'll be smart to propagate it to the jsdoc where it'll be surer to be found.

[gnprice]

Yeah, as written it does both. But this contains some key information that the reader who's simply using this component will need to know -- so that information is part of the interface and belongs in the jsdoc.

I think one way to write this would be:

• Take the first two paragraphs and put them in jsdoc. Also s/KeyboardAvoidingView/KeyboardAvoider/ there.
  • I think that covers all the information needed for simply using this component.
• Then leave the remaining paragraphs formatted as an implementation comment -- they're really information that's not needed for using the component, but is there to explain how the implementation works for the sake of someone modifying or debugging it.

If for some strange reason () that weirdness were common (or easily obtained) knowledge, the component's jsdoc might do OK to just say "passes some props straight through to KeyboardAvoidingView", I suppose? I guess if it were well-known, the implementation comment wouldn't be needed either, hmm.

Yep and yep 🙂

In other circumstances, where we're just passing some props through but they are reasonably well documented (or at any rate we don't have any thoughts to add to what's in their docs), it can indeed make sense for our interface to just say that:

zulip-mobile/src/common/Input.js

Lines 28 to 42 in 0735ff1

	/**
	* A light abstraction over the standard TextInput component
	* that allows us to seamlessly provide internationalization
	* capabilities and also style the component depending on
	* the platform the app is running on.
	*
	* @prop [style] - Can override our default style for inputs.
	* @prop placeholder - Translated before passing to TextInput as
	* a prop of the same name.
	* @prop [textInputRef] - Passed to TextInput in `ref`. See upstream docs
	* on refs: https://reactjs.org/docs/refs-and-the-dom.html
	* @prop ...all other TextInput props - Passed through verbatim to TextInput.
	* See upstream: https://reactnative.dev/docs/textinput
	*/
	class Input extends PureComponent<Props, State> {

[gnprice]

Hmm -- it seems like a bit of a step backward for the details of this definition to appear here at the navigator, rather than next to the ChatScreen component. In particular it makes it rather indirect to be looking at ChatScreen and work out where the header is coming from.

I think one pretty reasonable solution would be to put this code over in ChatScreen.js, either as a separately exported function or perhaps better as a property on ChatScreen. Like ChatScreen.header = (params: RouteParamsOf<typeof ChatScreen>) => { … };.

Even then, though, that still seems more complicated than just invoking ChatNavBar at the top of the component's own rendered tree. Does this buy us something that's worth the complexity?

(further discussion in main thread)

[chrisbobbe]

Sorry for the delayed review.

I know you've got a lot of stuff on your list. 🙂 Thanks for the review!

It's useful to see your skepticism about React Navigation's magic here; certainly they don't get everything right.

I think we agree right away that React Navigation's default header wouldn't work out of the box, but it might be helpful to see it, for some context, if you haven't already. On master, if I just remove the headerMode="none" from AppNavigator, that looks like this on iOS:

default.react.nav.header.mp4

On iOS, the animation looks consistent with headerMode="float", which makes sense because the React Nav doc says it's a common iOS pattern. (Notice how the default header doesn't slide, while our homemade header in the content area slides. Obviously if we were going with the default header, it wouldn't coexist with the homemade header.)

The default header doesn't work for a few reasons (here's an incomplete list, and this is just from looking on iOS):

• We don't want a title and nav button on MainTabsScreen. That's easy to get rid of, but in doing so, we should also override the headerMode="float" default and use "screen" instead. That's because "float" looks wrong when different screens have differently structured headers. (An alternative would be to think harder about animations; apparently they give an escape hatch if you really want to use "float".)
• To show each screen's title, it's doing the best it can—but it doesn't know that "Variables" should be capitalized, for example. Of course that'd be easy to fix, by e.g. passing the title option.
• The background color isn't quite right, I think?
• It's way too simple for, e.g., the chat screen nav bar, which needs more than a back button.

Still, these and other issues should be fixable by customizing the header: using various options like headerLeft, headerRight, headerBackTitleVisible (hmm that one's got an interesting-looking default), headerTitleAlign—there are maybe two dozen of these options, which we haven't exercised—or just supplying our own component for the whole header, as this PR does.

Ah, looking back, I agree that my comments about handling safe-area insets don't read as an argument for setting the header React Navigation's way. I think I'd assumed that we'd be interested in doing so (with header), as a semantically correct and not-too-hard thing to do, but that it'd be chaos (specifically for our safe-area-handling plans) to do it in some places and not others, so I picked doing it it everywhere, with the current PR taking one step to get there.

So, I owe some points in favor of picking that 🙂:

---

• Being a little bit "forced" (or at least encouraged) to write our headers a certain way (e.g., if we eventually use headerLeft and those options I mentioned above) is intuitively likely to help keep a consistent look-and-feel on all screens. E.g., you've pointed out somewhere that some back buttons look like ← and some look like <.

• IIUC, we have three substantive headers (four if you count MainTabsScreen's <View style={{ height: insets.top }} />): ChatNavBar, ModalNavBar, and ModalSearchNavBar. Currently,

  • Screen switches between ModalNavBar and ModalSearchNavBar based on the search prop, since several screens use each.
  • ChatScreen, being the only screen to use ChatNavBar, chooses to eject from Screen rather than ask Screen to be aware of its special header preferences. I guess this is defensible; we wouldn't want Screen's header-rendering logic to turn into a giant switch-case block on the current screen identifier ('account-pick', 'account-details', etc.). Would we? 🤔

  But the consequences for ChatScreen are a bit unfortunate: it has to duplicate code for showing the OfflineNotice, UnreadNotice, and for avoiding the keyboard, and that invites an inconsistent look-and-feel. Possibly Screen would be easier for ChatScreen (and others) to reach for if it didn't deal with headers.

• If we want, we could "set and forget" a default header in the navigator, with screenOptions, for new screens to have. Then, screens with specific needs (like ChatScreen) could override the default with their special headers, but naturally not before checking that

  • the change is necessary and well-considered, and
  • the "specific needs" aren't actually more general needs that the default should handle.

• The header should always participate in the layout in pretty much the same way: it stays at the top. Using header gives us this guarantee, and we shouldn't hit or have to think about problems like one you've mentioned where a header was scrolling up and down along with the screen's content.

• It seems odd to use the wrong semantics for setting a header; I think my strongest thought starting out was that using header would just be "right", and that keeping the status quo would be "wrong", because we're pretending in the status quo that

  • the header is something that it's not (i.e., null), and that
  • the screen content is something that it's not (i.e., content plus header).

  Possibly this is just putting too much faith in React Navigation's ability to structure things nicely? Still, if we keep things how they are, maybe let's add a code comment about this? 🙂

• It seems a bit lucky that we're not going for the iOS headerMode="float" effect. I don't think we should go for it—but if we wanted to, I don't see how we could do it in a world where the header is defined inside the screen-content component. There might be other features like this that we'd potentially be interested in, but that we're blocked from considering with the current implementation.

• I guess I owe some data about this, but I figured there was a decent chance that accessibility (including aspects I don't yet understand) would be improved, possibly a lot, by using the common/documented approach. (As I look for that data, though, I notice that https://github.com/react-navigation/react-navigation/pulls?q=label%3Aaccessibility comes up empty, which is pretty astonishing and not a good sign, hmmmm…still, we could get any future improvements naturally when taking new React Nav versions?)

---

I'd understand if these considerations don't convince you that the trouble (including the trouble with Flow I mentioned in the description) is worth it, but I wanted to put them out there to help inform the decision, since I didn't do so before. 🙂

[gnprice]

Thanks for all those details! Replied in chat.

[chrisbobbe]

Thanks for the review! Revision pushed, with the header-option work removed.

[gnprice]

Thanks @chrisbobbe ! This looks great -- small comments below. Please merge at will after taking a look through those.

[gnprice]

I like this cleanup!

[chrisbobbe]

Please merge at will after taking a look through those.

Done, thanks for the reviews!