[$500] Android - Create room - Inconsistent behavior of cursor after selecting workspace/visibility

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.3.98.0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Open the app
2. Tap the FAB > Start chat > Room
3. Enter the room name
4. Select the workspace and/or visibility

Expected Result:

The cursor is not shown, and the keyboard is hidden

Actual Result:

On Android and iOS, the cursor returns to the room name, and the field is highlighted, while on web and mweb, the cursor is not seen, and the keyboard is hidden on mweb. On iOS, the keyboard overlaps the "Create room" button. Also, the keyboard is hidden on Samsung s10+, while it appears on Pixel 6.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6271875_1699653421342.Android_pixel6.mp4

View all open jobs on GitHub

Bug6271875_1699653421346.iOS.mp4

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fbab8ee06aa1521a
• Upwork Job ID: 1723173111736639488
• Last Price Increase: 2024-06-15

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01fbab8ee06aa1521a

[melvin-bot]

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh (External)

[bernhardoj]

The workspace/visibility is a modal, so the focus will be restored once the modal is closed. I think we should hold this for #24452 (comment)

[tienifr]

The workspace/visibility is a modal, so the focus will be restored once the modal is closed. I think we should hold this for #24452 (comment)

That other issue is related to Composer only, and only there we have the "the focus will be restored once the modal is closed" behavior. Here we should dismiss the keyboard when modal opens like the Expected suggested, because when the user goes to workspace/visibility step, it's most likely they already completes entering the room name and there's no need to refocus on the room name input.

Proposal

Please re-state the problem that we are trying to solve in this issue.

On Android and iOS, the cursor returns to the room name, and the field is highlighted, while on web and mweb, the cursor is not seen, and the keyboard is hidden on mweb. Also, the keyboard is hidden on Samsung s10+, while it appears on Pixel 6.

On iOS, the keyboard overlaps the "Create room" button.

This part is out of scope and I believe we already have another ticket to remove that.

What is the root cause of that problem?

In iOS and some Android devices, if a modal is opened while a keyboard is open, the keyboard will be dismissed natively but will resurface after the modal is closed. It's native behavior. While for web/mWeb it doesn't have that behavior.

What changes do you think we should make in order to solve the problem?

We need to proactively dismiss the keyboard before the modal opens. We can do that by making use of the willAlertModalBecomeVisible of the Modal, it will become true right before the modal open, we need to listen to the modal state and dismiss the keyboard if the willAlertModalBecomeVisible changes from false to true. Also we'll only listen to the modal state if the current screen is focused. We can put the logic in a common useDismissKeyboardOnModalOpen hook for reuse.

function useDismissKeyboardOnModalOpen() {
    const willAlertModalBecomeVisibleRef = useRef(false);
    const isFocused = useIsFocused();    

    useEffect(() => {
        if (!isFocused) {
            return;
        }
        const unsubscribeOnyxModal = onyxSubscribe({
            key: ONYXKEYS.MODAL,
            callback: (modalArg) => {
                if (_.isNull(modalArg) || typeof modalArg !== 'object') {
                    return;
                }

                if (!willAlertModalBecomeVisibleRef.current && modalArg.willAlertModalBecomeVisible) {
                    Keyboard.dismiss();
                }

                willAlertModalBecomeVisibleRef.current = modalArg.willAlertModalBecomeVisible;
            },
        });

        return () => unsubscribeOnyxModal && unsubscribeOnyxModal();
    }, [isFocused]);
}

We use onyxSubscribe to minimize unnecessary component rerendering if the screen is not focused, this is already used elsewhere in the app. 1 alternative is to connect to modal in Onyx in the component.

Then we can use the hook in the WorkspaceNewRoomPage and any other page that has the issue.

What alternative solutions did you explore? (Optional)

We can add Keyboard.dismiss individually on press of the Workspace/visibility, but above is a more global fix.

If instead we want to autofocus whenever the modal closes, we can implement similarly in the useAutoFocusInput hook. If the modal.isVisible changes from true to false, we'll autofocus the input.

[bernhardoj]

(image taken from #24452 (comment))

That other issue is related to Composer only,

AFAICT, it will be applied to all Modal.

and there's no need to refocus on the room name input.

They also plan to add shouldRestoreKeyboard prop to the modal and in this case, we pass it as false. So, it's better to hold this.

[tienifr]

They also plan to add shouldRestoreKeyboard prop to the modal and in this case, we pass it as false. So, it's better to hold this.

I think for dismissing (and not restoring keyboard) for Modal, my proposal is much more simple and better than all that's being discussed in the other issue. The other issue should focus on handling the refocusing which applies for Composer, for other modals, we should be using this solution.

[sakluger]

I agree with @tienifr that this is an issue and it's not enough to wait for the user to close the modal. Although I don't really understand how this is related to #24452 (comment), the issue that @bernhardoj references.

@hoangzinh I'm curious for your thoughts here. Do you think we should go with @tienifr's proposal, or would it be better to hold?

[hoangzinh]

Although I don't really understand how this is related to #24452 (comment), the issue that @bernhardoj references.

@sakluger There are a lot of conversations in #24452, and they end up with extending the scope of that issue to not only fix in chat reports but also in all pages having modals #24452 (comment).

Add a shouldRestoreKeyboard prop for Modal component (its value could be some enum to handle more complex cases, The effect is somewhat like keyboardShouldPersistTaps).

If this point is implemented, I think we should put this issue on hold, otherwise, we will do duplicate work with them.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@sakluger, @hoangzinh Still overdue 6 days?! Let's take care of this!

[sakluger]

Okay, we'll put on hold for #24452 and revisit once that is complete.

[sakluger]

Thanks @hoangzinh. I switched to Monthly, and also added to the vip-vsb project since it has to do with chat functionality.

[sakluger]

Still on hold for #24452.

[sakluger]

The solution for the hold issue was deployed to prod last week, but I can still reproduce the issue on iOS v1.4.80-17.

@hoangzinh should we open this issue back up for new proposals?

[hoangzinh]

Yes @sakluger

cc @bernhardoj @tienifr just in case you're still interested in fixing this issue

[bernhardoj]

I think #24452 is not complete yet, they are splitting the solution into several PRs.

cc: @ntdiary

[sakluger]

Thanks @bernhardoj. I asked in that issue too, but do you know which PRs are still pending?

[bernhardoj]

I don't see any open PR, but based on this comment, they are trying to split it into 4 phases and the 2nd phase is already completed.

[ntdiary]

Expected Result:

The cursor is not shown, and the keyboard is hidden

Hi, I think this is one of the situations in the general modal focus case.
Ideally, using these two props within the ValueSelectorModal should be sufficient to address this issue.

    onBackdropPress={onBackdropPress}
+   shouldEnableNewFocusManagement
+   restoreFocusType="delete"

However, it's worth noting that there is an issue with react-native-modal.
In the new Fabric architecture, the native modal component mounts earlier than the library onModalWillShow, causing the iOS system to save the keyboard state and blur the keyboard before our JS layer does.
We need to make a tweak to proactively hide the keyboard in our app before the iOS system does.
e.g.,

Here is the final result:

demo.mp4

We could also consider hiding the keyboard earlier in the onPress event to address this specific case. As for other focusing logic, as I described here #24452 (comment), it may be cleaned up later.

If you all agree with these thoughts, I think we don't need to hold this issue any longer. It would be great if a contributor is willing to raise a PR to fix this issue. I'm happy to share some information and, of course, still remain open to different opinions. 😄

[hoangzinh]

Thanks for your suggestion @ntdiary. I will try to check it.

[sakluger]

It sounds like the remaining parts of the hold issue won't solve this, so I've added the help wanted label back.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[hoangzinh]

In the new Fabric architecture, the native modal component mounts earlier than the library onModalWillShow, causing the iOS system to save the keyboard state and blur the keyboard before our JS layer does.

Hi @ntdiary is there any reported issue in react-native-modal or it's just what you found during phases 1 & 2 in your issue? Thank you.

[ntdiary]

Hi @ntdiary is there any reported issue in react-native-modal or it's just what you found during phases 1 & 2 in your issue? Thank you.

@hoangzinh, It's only what I found while testing my PR. As for react-native-modal, it seems it hasn't been updated in two years. :)

[hoangzinh]

Thanks for confirming @ntdiary. I think what @ntdiary suggested here is the best so far. It works well with new focus management. But we might need to conditional it to only trigger on the 1st render.

What do you guys think?

[sakluger]

I'm not an engineer so I can't really evaluate how good that solution is 🙂. @hoangzinh feel free to C+ approve the proposal so we can get a BE engineer assigned to confirm. Thanks!

[hoangzinh]

Sure @sakluger.
@tienifr @bernhardoj anyone agree with my thoughts here and volunteer to implement it? Thank you.

[bernhardoj]

After retesting, I found that the behavior on all platforms now is the same. The room name field is refocused after selecting the workspace visibility because of the focus trap.

[hoangzinh]

Thanks for retesting it @bernhardoj. I confirmed that the above behavior is consistent between mWeb and native atm. cc @sakluger should we close this issue?

Screen.Recording.2024-06-18.at.16.45.43.mov

[sakluger]

Thanks @bernhardoj for retesting! Yes, let's close this out.