Focus compose box when navigate away from RHN

[Beamanator]

<If necessary, assign reviewers that know the area or changes well. Feel free to tag any additional reviewers you see fit.>

Details

Background: When users open & subsequently close a modal, focus should be given to the compose box to make the app as easy to use as possible.

This has been fixed & regressed multiple times

• Fixed: Focus the compose field after closing modals #1699 cc @tgolen
• Fixed: Focus compose field when modal dismissed #1780
• Broke: 6706d16#diff-55a7d35dba1b789188644baad2c0f1a243ec3c2c6819c8b06d5a5ec7c245574a cc @marcaaron

Now that we're more and more completely using React Navigation for our sub-pages (and moving away from pure modals), we can't rely on BaseModal's componentWillUnmount as a catch-all for hiding modals (and therefore allowing ReportActionCompose to move the cursor to the compose box).

We now have to rely on pure React Navigation components / functions in order to help us determine when those modal-like pages open & close.

Therefore, I set up Navigation listeners focus and beforeRemove which are used on modal screens to set modal.isVisible as true when the modal opens (a.k.a. is focused) and false when the modal closes (a.k.a when we navigate away from a modal screen)

• Note: This does work for modal dismissing in iOS!
• Here are the docs for navigation events: https://reactnavigation.org/docs/navigation-events/

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/154180

Fixes #1513

• This should have been closed after @tgolen 's was merged previously, FYI

Tests

Tested On

• Web
• Mobile Web (not needed)
• Desktop
• iOS (only needed when opening profile page from a report, then dismissing (via swipe) as this takes the user back to the report, not the LHN!)
• Android (not needed)

Screenshots

Web

Screen.Recording.2021-03-24.at.9.40.58.PM.mov

Mobile Web

N/A because on mobile, closing a modal-page takes you to the LHN view

Desktop

Same as "Web"

iOS

Screen.Recording.2021-03-25.at.3.52.58.PM.mov

Android

N/A because on mobile, closing a modal-page takes you to the LHN view

[stitesExpensify]

lgtm

[marcaaron]

I could have called setModalVisibility(true); inside a useEffect to ensure it doesn't get set to true twice, but:
First, we are trying not to use those hooks

Tbh, I think we should just make an exception for this unique case. We can't avoid using the hook provided by react-navigation and need to perform a side effect so this is what useEffect() is for. Open to other opinions on this.

[marcaaron]

It doesn't get affected when the CreateMenu opens / closes (which is yet another modal)

Also, sorry, didn't quite understand what is being said about the CreateMenu here.

[Beamanator]

It doesn't get affected when the CreateMenu opens / closes (which is yet another modal)

Also, sorry, didn't quite understand what is being said about the CreateMenu here.

Here I was just saying that I tested my previous solution with toggling the CreateMenu button, and it still worked. Nothing too interesting :D

Update:

I found a few navigation event listeners here (https://reactnavigation.org/docs/navigation-events/)

• Looks like using focus and beforeRemove worked quite well for web AND desktop!

[marcaaron]

Just a heads up that we have @parasharrajat working on a similar issue. I think this PR is possibly moving us in the opposite direction depending on how this all gets resolved. Said another way, if we figure out an alternative then I think these changes here won't be needed.

#2022 (review)

[Beamanator]

Hmm I took a look at the similar PR and it kiiinda seems we may be doing the same thing in two different ways? Here's some thoughts which I thought could help:

Here it looks like wrapping a component in NavigationContext basically gets you the navigation object, where you can add an event listener (focus, blur, beforeRemoe)

Here is another example, without using NavigationContext, where you just add an event listener to the navigation object directly within a Screen.

Finally, Here you can see adding the listener prop to a Screen lets you "add a listener from the component where you defined the navigator rather than inside the screen."

I wonder if it's best to look for "which option is most readable / better long term"? Open to ideas though, what do you think @marcaaron ?

[marcaaron]

That sounds right @Beamanator, the solutions work in similar ways. We have come up with some interesting ways to give other parts of the app insight into which pages or modals are in focus or not. At this point, I'm curious if we can find some other way to focus this TextInput that doesn't require a screen needing all this extra context at all.

[kidroca]

I'm curious if we can find some other way to focus this TextInput that doesn't require a screen needing all this extra context at all.

@marcaaron I had an idea about this as well, here's a rough sketch:

Have a FocusMananger lib

interface FocusManager {
  // restores focus to the last registered element
  restoreFocus: () => void
  
  // stores a react ref to a focusable item
  register: (ref: ElementRef) => void

  // remove stored ref
  unregistrer: (ref: ElementRef) => void
}

Have a withFocusManager HOC that wraps a focusable item (like the input)

render() {
  return (
    <WrappedComponent
       {...this.props}
       ref={el => this.focusableRef = el}
       onFocus={(e => FocusManager.register(e.target))}
       onBlur={(e => FocusManager.unregister(e.target))}
      // onFocus/onBlur should also delegate to props.onFocus, props.onBlur if they exist...
    />
  )
}

componentWillUnmount() {
  // cleanup
  FocusManager.unregister(this.focusableRef);
}

register would push elements in a stack - the element on top would be the one to return focus to
unregister would remove elements from the stack. This way when an element is blurred or it unmounts it's not a valid target to return to
restoreFocus restore focus should be called after a modal closes (either react navigation modal or a regular modal). It check if it has something "suitable" in the stack and restores focus to it. then the stack is reset to clean up from past references

I've done something similar to support prev/next field navigation in react-native

---

If this is something that should happen only on web/desktop a web specific modal components/solution can be used:

https://reactjs.org/docs/accessibility.html#programmatically-managing-focus

A great focus management example is the react-aria-modal. This is a relatively rare example of a fully accessible modal window. Not only does it set initial focus on the cancel button (preventing the keyboard user from accidentally activating the success action) and trap keyboard focus inside the modal, it also resets focus back to the element that initially triggered the modal.

[marcaaron]

@kidroca That sounds interesting, but probably would prefer a simpler solution to this current problem if possible. There are no multiple elements that need focus yet, just one, so I'm unsure what value there would be in implementing a focusable inputs stack. It does sound like exposing the input's ref so it can be triggered from other places is the right line of thinking.

[kidroca]

@kidroca That sounds interesting, but probably would prefer a simpler solution to this current problem if possible. There are no multiple elements that need focus yet, just one, so I'm unsure what value there would be in implementing a focusable inputs stack. It does sound like exposing the input's ref so it can be triggered from other places is the right line of thinking.

I agree,
This doesn't need a stack actually, I was just showcasing
It can just save a ref - where the last focus was
Then restore focus there

Kind of like document.activeElement works on web - you check document.activeElement before you open a modal, then restore focus to that element

---

By the way I see this exact behavior in mobile native for the Modals using react-native-modal - when a modal closes focus is restored to the previously active element (default behavior - not via e.cash autofocus handling)

[Beamanator]

[moving comment elsewhere]

[Beamanator]

Looks like there's some undesired behavior regarding mobile device keyboards, but I believe we decided this will be fixed in a new issue / PR, so I think this can be re-reviewed & hopefully merged 👍

[marcaaron]

This LGTM. Merging with the caveat that this will likely exacerbate some ongoing issues described here. As those issues are being worked on independently I think we can proceed here.