[Chat] [IOU] Mobile keyboard should not be displayed at IOUConfirm step

[jliexpensify]

If you haven’t already, check out our contributing guidelines for onboarding!

---

Problem

The mobile alphabetic keyboard is being displayed on physical devices when the IOUAmount page is displayed. It isn't always consistent but occurs most of the time with no obvious pattern. This occurs on both Android and iOS physical devices.

To reproduce this issue, apply the following diff so that 'New chat' and 'New group' launches the IOUModal:
Make sure you include the final new line!

diff --git a/src/components/CreateMenu.js b/src/components/CreateMenu.js
index 7c7afe3a..f311b4a1 100644
--- a/src/components/CreateMenu.js
+++ b/src/components/CreateMenu.js
@@ -53,12 +53,12 @@ class CreateMenu extends PureComponent {
             {
                 icon: ChatBubble,
                 text: 'New Chat',
-                onPress: () => this.setOnModalHide(() => Navigation.navigate(ROUTES.NEW_CHAT)),
+                onPress: () => this.setOnModalHide(() => Navigation.navigate(ROUTES.IOU_REQUEST)),
             },
             {
                 icon: Users,
                 text: 'New Group',
-                onPress: () => this.setOnModalHide(() => Navigation.navigate(ROUTES.NEW_GROUP)),
+                onPress: () => this.setOnModalHide(() => Navigation.navigate(ROUTES.IOU_BILL)),
             },
         ].map(item => ({
             ...item,

iouAmountKeyboard.mp4

Desired Solution

The alphabetic keyboard should not be displayed, as we are not rendering an Input field on mobile:

Upwork Link: https://www.upwork.com/ab/applicants/1372398296191315968/job-details

[Maftalion]

This looks to have been introduced with the new React Navigation changes and unrelated to the IOU screens, you can set any route or simply close the menu and it will still cause the keyboard to show.

Proposal

• The bug actually lives inside ReportActionCompose. since the list of chats is really an "open drawer", there is actually a report view mounted as well:

In the componentDidUpdate we have the following code that cause the input in the report to be focused when the menu modal is closed:

        // When any modal goes from visible to hidden, bring focus to the compose field
        if (prevProps.modal.isVisible && !this.props.modal.isVisible) {
            this.setIsFocused(true);
        }

• The solution would be to only run this logic when report component is actually "focused". RNav5 only provides this functionality via hooks so we'd have to set up a wrapper to be able to allow it to work with class components:

const ReportActionComposeWrapped = (props) => {
    const isFocused = useIsFocused();

    // eslint-disable-next-line react/jsx-props-no-spreading
    return <ReportActionCompose {...props} isFocused={isFocused} />;
};

Which will allow us to check if the page is focused before focusing the input which resolves the keyboard issue:

        // When any modal goes from visible to hidden, bring focus to the compose field
        if (this.props.isVisible && prevProps.modal.isVisible && !this.props.modal.isVisible) {
            this.setIsFocused(true);
        }

Android Back Issue

• For the android back button, I'm unable to reproduce but it might have something to do with Navigation.dismissModal.

[Julesssss]

Hi @Maftalion, thanks for sharing the detailed solution. We would love for you to take on this issue via UpWork if you're interested.

[Maftalion]

@Julesssss thanks, just submitted a proposal!

Also just want to make sure, but you're fine with the useIsFocused hook being used, correct? I think the style guide says to avoid hooks but I don't see an alternative for this.

[Julesssss]

That's a great point @Maftalion . I'm not sure about this so please hold while I confirm this internally. I'll get back to you ASAP.

[Julesssss]

Okay, we’re not quite ready to start using Hooks as this requires a bit more internal discussion and we’d prefer to spend a bit more time exploring alternatives (or reverse engineering Hooks) for now.

But we'd love to hear any other thoughts you had regarding the issue.

[parasharrajat]

@Julesssss @jliexpensify I am posting a proposal now as I thought it was assigned to the @Maftalion.

Proposal

1. So as per the comment [Chat] [IOU] Mobile keyboard should not be displayed at IOUConfirm step #1855 (comment), we can't use hooks so I have another solution.
2. Surely, it's the focusing issue as mentioned in the proposed solution above.

  // When any modal goes from visible to hidden, bring focus to the compose field
        if (prevProps.modal.isVisible && !this.props.modal.isVisible) {
            this.setIsFocused(true);
        }

This is code is forcing the focus on the input when we close the CreateMenu Modal. But report screen was never unmounted thus this is triggered and resulted in focus being set on the input.

3. We can use the NavigationContext. Like this

ReportActionCompose.contextType = NavigationContext;

which allows us to listen for the focus event on the navigation instance.

then we can attach listeners like this.

   componentDidMount() {
        this.unsubscribeFocus = this.context.addListener('focus', () => this.setState({
            isScreenFocused: true,
        }));
        this.unsubscribeBlur = this.context.addListener('blur', () => this.setState({
            isScreenFocused: false,
        }));
    }

    // eslint-disable-next-line react/sort-comp
    componentWillUnmount() {
        this.unsubscribeFocus();
        this.unsubscribeBlur();
    }

4. Now we update the ComponentDidUpdate like this.

 componentDidUpdate(prevProps) {
        const valueToReturn = this.context.isFocused();

        if (this.state.isScreenFocused !== valueToReturn) {
            // If the value has changed since the last render, we need to update it.
            // This could happen if we missed an update from the event listeners during re-render.
                // eslint-disable-next-line react/no-did-update-set-state
            this.setState({
                isScreenFocused: valueToReturn,
            });
        }

        // The first time the component loads the props is empty and the next time it may contain value.
        // If it does let's update this.comment so that it matches the defaultValue that we show in textInput.
        if (this.props.comment && prevProps.comment === '' && prevProps.comment !== this.props.comment) {
            this.comment = this.props.comment;
        }

        // When any modal goes from visible to hidden, bring focus to the compose field
        if (this.state.isScreenFocused && prevProps.modal.isVisible && !this.props.modal.isVisible) {
            this.setIsFocused(true);
        }
    }

which will fix this issue. If it's already assigned, feel free to take a look at the proposal. Maybe it will help.

[Maftalion]

@Julesssss I'm fine with deferring this task to @parasharrajat since he took the time to figure out how to do it without using the hook. My only suggestion would be to set it up as a HOC so this can used on other screens as well without needing all the boilerplate.

I also found another way to fix this as well, if we just use this.props.isSmallScreenWidth && !Navigation.isDrawerOpen() instead of isScreenFocused then we can achieve the same result.

[parasharrajat]

this.props.isSmallScreenWidth && !Navigation.isDrawerOpen()

Interesting Hack @Maftalion . Will try this one as well. Thanks for sharing.

[jliexpensify]

Hi @parasharrajat and @Maftalion - thanks for both sharing potential solutions. So that I'm 100% clear on things, was it decided that Rajat was going to take this on instead?

If so, please let me know and I'll look into changing this in Upwork. @parasharrajat if you are taking this on, do you mind submitting a proposal in Upwork as well?

cc @Julesssss re: the discussion on not using hooks

[Julesssss]

@jliexpensify yeah, that's right.

Thanks to both of you for figuring this issue out.

[jliexpensify]

Awesome, as discussed - I've hired @parasharrajat via Upworks. Thanks for all your help @Maftalion !

[kidroca]

Hey guys the selected approach is good and will greatly reduce side effects caused by ReportActionCompose.componendDidUpdate

I just want to mention something that you might consider for the future:

My observations:

The componendDidUpdate in ReportActionCompose is a means to reset the view because it doesn't unmount when focus moves away:

• it checks whether a new comment is loaded from props and resets the comment
• it checks whether we've opened a new report and focuses the input
  • btw this means if we open the drawer and select the same chat (report) focus would not go to the field
• it reacts to modal closes and brings focus to the input
  • this issue fixes an unwanted side effect for this point

With the new functionality brought in this ticket - a HOC providing focus awareness we can do the following. Instead of anticipating and reacting inside componendDidUpdate we can:

• remove this lifecycle method partially or all together
  • there's still some stuff that we need to wait from Onix
  • it would reduce complexity in componendDidUpdate (or remove it altogether) if we have a parent loader component responsible for getting/waiting data from Onyx, rendering a loading state in the meantime, and mounting ReportActionCompose with the appropriate data after we have everything (which will be instant for already persisted data), compared to anticipating changes in componentDidUpdate
• use focus awareness to conditionally mount the component
• re-mounting would guarantee a fresh state every time we move to a specific chat
  • we can auto focus the text field at this point
• when we move away the view and it's child views will unmount fixing other bugs as this one: "When you are inside a chat and press the back button in the header the keyboard stays visible". This is because a text field is still focused it's just not in the visible area of the screen like so:
• 
• Or this Can use keyboard arrow when chat switcher is open to get to the chat composer #1228 where the solution is to test and disable the text input in ReportActionCompose while the drawer is open

Pseudo Implementation (in ReportScreen)

<ScreenWrapper>
  <HeaderView />
  {
     isFocused
       ? <ReportView reportID={activeReportID} />
       :  <PlaceholderView reportID={activeReportID} />
  }
</ScreenWrapper>

The purpose of the PlaceholderView is to preserve smooth screen transitions - it might not be needed at all depending on when isFocused swaps states. It can be a stateless clone of the last ReportView rendered

Alternatively isFocused can be forwarded to mount/unmount just the ReportActionCompose component as that's the root cause for all the problems

There's one benefit I see in the ReportView staying mounted, but it still has 2 edges:

• When you return to the same chat, content is already rendered - the experience is smooth
• When you open another chat though, you can briefly see the previous chat's content, it has to first clean up and shortly after render the new chat

I think the fast response when you open the same chat will be preserved even if you start from unmounted state

• you already have the data cached locally with ONYX
• from observations as you swap between two chats that have data cached locally - It happens fast even though it's triggering re-render for most of the components (children) involved

---

There might be something already baked in react-navigation regarding such cases - A view outside the visible area causing side effects, or they might have a suggestion in their docs

Most of this is from the top of my head. I haven't studied this project entirely so I might come up with something better (Do chime in if you're interested), but usually side effects causing logic tend to cause issues like these and patching it with more conditional statements works only to a certain extent.

[parasharrajat]

@kidroca Thanks for bringing in this and suggesting an approach. I am well aware of the issue you mentioned. That's the only reason to Introduce the HOC approach. With this, we can manage the focus more accurately. I have some thoughts on fixing the mentioned issue with much simplicity.

[Julesssss]

Thanks for that suggestion @kidroca.

Would you like to create an Expensify.cash issue to continue this discussion? We might need to discuss a bit further with other colleagues, but I think that could be a great further improvement and I would be happy to get an UpWork issue set up for that investigation/implementation.

[kidroca]

Yes, I'd like that. Definitely needs more discussion and I don't want to spam the current ticket. I'll open a new ticket when I'm done with my current work.

[Maftalion]

The only caveat to this approach is that I think @marcaaron started setting up the Right Docked Modals as screens, so on web/desktop it would render the PlaceholderView when those modals open on top of the report. But the isFocused functionality would definitely be useful to be set up as a HoC in general

[kidroca]

I think it would be sufficient to just unmount the input field TextInputFocusable as a proof of concept:

ReportActionCompose.render

{
  chatHasFocus
    ? <TextInputFocusable />
    : <FakeInput>Write something...</FakeInput>
}

When there's no text input mounted it won't be able to accidentally steal focus

---

There's something similar already:

const inputDisable = this.props.isSmallScreenWidth && Navigation.isDrawerOpen();

But it's re-evaluated when "something" triggers a new render - props need to change
chatHasFocus -> This will be a prop (disregard the name) so it will trigger a re-render exactly when needed

[marcaaron]

I left a similar comment on @parasharrajat's PR, but wondering if another solution here is to be more selective about when we are focusing this input instead of assuming we always want to do it when "any modal closes and the screen is focused".

I like the HOC idea and there is a good chance knowing whether a screen is focused will be useful in the future, but perhaps not yet. If we know the specific situations where we want to focus the input and can find a way to focus explicitly perhaps we won't need to know if "modal is visible" or "screen has focus". But maybe this is easier said than done.

[Maftalion]

@marcaaron true, if the experience should be to focus only when the report changes (and not re-focus when a modal closes) then the conditional can probably just be simplified to (prevProps.reportID !== this.props.reportID) and the issue would be solved as well.

[kidroca]

@Maftalion this is exactly what I did here: 0395526

ReportActionCompose.componentDidUpdate

// When the report ID changes bring focus to the compose field
if (prevProps.reportID !== this.props.reportID) {
    this.setIsFocused(true);
}

And this is the result:

2021-03-26.21-51-26.mp4

There's still a place for improvement - the slight jitter that happens when you transition from modal to modal - its react native trying to return focus to the view that was focused before opening the modal, only to open another modal. It's not that prominent on a real device, but perhaps we can disable the field to try make it "unfocusable" when the 2nd modal is selected to appear (openPicker()) and restore enabled state on close but before onModalHide

---

Edit: I found out that this behavior is not intended for mobile while I was creating #2129

On native layers we like to have the Text Input not focused so the user can read new chats without they keyboard in the way of the view

This is a comment inside TextInputFocusable

On native focusing would also bring the keyboard, so autofocusing should probably be avoided - more details in the new issue

Much better experience when the keyboard does not try to appear between modals

2021-03-29.13-56-42.mp4

[marcaaron]

@parasharrajat We are currently discussing whether to just get rid of auto focusing the input altogether on native platforms. This should fix this issue and a couple of others if we decide this is the best way forward.

[parasharrajat]

@marcaaron Yup sure. There is a couple of ongoing PR and opinions(including yours) that would change the objective. Thus I am waiting as per my update here. Thanks for the update info.

[marcaaron]

Here's what I am thinking a complete fix for this should be:

1. Disable the "modal visibility aware" autofocus logic for the chat comment input when:

   • we are using a native platform
   • we are using mobile web (defined as anything that has a touch screen or software keyboard)

2. Modify this logic here so that it will only focus the input on desktop/web + when the report isFocused (if necessary)
   https://github.com/Expensify/Expensify.cash/blob/e9e726aa2a2f24ae98e88d9aa166acf4e8036ecb/src/pages/home/report/ReportActionCompose.js#L75-L78

3. Undo this change here https://github.com/Expensify/Expensify.cash/blob/e9e726aa2a2f24ae98e88d9aa166acf4e8036ecb/src/pages/home/report/ReportActionCompose.js#L261-L262

We should only want to "autoFocus" on web/desktop (see rules of step 1). So, we require a custom implementation for this anyway instead of just passing autoFocus directly to the TextInput

4. Finally we should make sure that the keyboard is always dismissed when navigating from a chat to the sidebar/drawer content. @kidroca I think you've mentioned that 4 will no longer be necessary once we do the changes here