[HOLD for payment 2023-09-04] [$1000] Chat - Pressing CMD+K when Global create is open closes the menu and doesn’t open search

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Go to https://staging.new.expensify.com/
   and log in
2. From LHN press the plus icon
3. From the keyboard press CMD+K (or CTRL+K on Windows)

Expected Result:

The Global create menu closes and the Right hand modal opens with search

Actual Result:

The Global create menu closes but the Right hand modal doesn't open with search

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.51.0

Reproducible in staging?: Yes

Reproducible in production?: No

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6156805_Recording__226.mp4

Bug6156805_Recording__2prod_28.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause -Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d1f9cb6cfc1f3fe2
• Upwork Job ID: 1688913841274916864
• Last Price Increase: 2023-08-08
• Automatic offers:
• aimane-chnaif | Reviewer | 26071710

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @Beamanator (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Beamanator]

I wouldn't consider this a blocker - cc @puneetlath

And def can be worked externally, marking as such

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @zanyrenney (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

Current assignee @zanyrenney is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Current assignee @mananjadhav is eligible for the External assigner, not assigning anyone new.

[alanleungcn]

Proposal

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

Pressing CMD+K when Global create is open closes the menu and doesn’t open search

What is the root cause of that problem?

When we subscribe the keyboard shortcut in the AuthScreens component, we didn't specify the excludedNodes parameter for the subscribe function.

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

We should exclude the global create menu, i.e. do not prevent default and let the event bubble. Below is a sample of what could be done and a video showing the result.

this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
        ...
        searchShortcutConfig.descriptionKey,
        searchShortcutConfig.modifiers,
        true,
        false,
        0,
        true,
        ['DIV'], // exclude the global create menu here
);

Pressing.CMD+K.when.Global.create.is.open.closes.the.menu.and.doesn.t.open.search.mov

What alternative solutions did you explore? (Optional)

None

[melvin-bot]

📣 @alanleungcn! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[alanleungcn]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01f5c90a04dc1b08b8

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[Thanos30]

Proposal

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

CMD+K keyboard shortcut doesn't work when FloatingActionButtonAndPopover is open. Also happening with CMD+SHIFT+K

What is the root cause of that problem?

The root cause of this problem is in the way we subscribe the keyboard shortcuts in AuthScreens.js:

this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
            searchShortcutConfig.shortcutKey,
            () => {
                Modal.close(() => {
                    if (Navigation.isActiveRoute(ROUTES.SEARCH)) {
                        return;
                    }
                    return Navigation.navigate(ROUTES.SEARCH);
                });
            },
            searchShortcutConfig.descriptionKey,
            searchShortcutConfig.modifiers,
            true,
        );

The shortcut subscription for creating a new group is similar. As you can see, we are sending the callback inside the close function of the Modal:

function close(onModalCloseCallback, isNavigating = true) {
    if (!closeModal) {
        // If modal is already closed, no need to wait for modal close. So immediately call callback.
        if (onModalCloseCallback) {
            onModalCloseCallback();
        }
        onModalClose = null;
        return;
    }
    onModalClose = onModalCloseCallback;
    closeModal(isNavigating);
}

At this point, our shortcut is closing the Modal, which triggers the Modal's closing function, which is set on FloatingActionButtonAndPopover, and it's overriding our shortcut.

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

Since we want our keyboard shortcuts to close the Modals and navigate to the appropriate location (Search or New group), it's better to respect the close function of each modal, and then proceed with the navigation.

So, in AuthScreens.js , we will make the subscriptions like this:

this.unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
            searchShortcutConfig.shortcutKey,
            () => {
                Modal.close();
                if (Navigation.isActiveRoute(ROUTES.SEARCH)) {
                    return;
                }
                Navigation.navigate(ROUTES.SEARCH);
            },
            searchShortcutConfig.descriptionKey,
            searchShortcutConfig.modifiers,
            true,
        );

, where we are first running the close() function, and later navigate, so we don't overlap the onClose functions.

[SosenWiosen]

Proposal

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

CMD+K doesn't navigate to search when FloatingActionButtonAndPopover is open.

What is the root cause of that problem?

FloatingActionButtonAndPopover uses PopoverWithoutOverlay. In PopoverWithoutOverlay/index.js Modal.onModalDidClose() isn't called when the Popover gets closed. Thus the callback from AuthScreen.js never gets called.

React.useEffect(() => {
        if (props.isVisible) {
            props.onModalShow();
            onOpen({
                ref,
                close: props.onClose,
                anchorRef: props.anchorRef,
            });
        } else {
            props.onModalHide();
            close(props.anchorRef);
        }
        Modal.willAlertModalBecomeVisible(props.isVisible);
        Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);
        
        // We want this effect to run strictly ONLY when isVisible prop changes
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [props.isVisible]);

    if (!props.isVisible) {
        return null;
    }

This is different to BaseModal.js.

  componentWillUnmount() {
       // Only trigger onClose and setModalVisibility if the modal is unmounting while visible.
       if (this.props.isVisible) {
           this.hideModal(true);
           Modal.willAlertModalBecomeVisible(false);
       }
       // To prevent closing any modal already unmounted when this modal still remains as visible state
       Modal.setCloseModal(null);
   }
   
   hideModal(callHideCallback = true) {
       if (this.props.shouldSetModalVisibility) {
           Modal.setModalVisibility(false);
       }
       if (callHideCallback) {
           this.props.onModalHide();
       }
       Modal.onModalDidClose();
   }

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

We should change the useEffect in PopoverWithoutOverlay.js to call Modal.onModalDidClose(); (the same logic as in BaseModal.js).

React.useEffect(() => {
        if (props.isVisible) {
            props.onModalShow();
            onOpen({
                ref,
                close: props.onClose,
                anchorRef: props.anchorRef,
            });
        } else {
            props.onModalHide();
            close(props.anchorRef);
            Modal.onModalDidClose();
        }
        Modal.willAlertModalBecomeVisible(props.isVisible);
        Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);

        // We want this effect to run strictly ONLY when isVisible prop changes
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [props.isVisible]);

Video

Screen.Recording.2023-08-08.at.18.17.37.mov

[aimane-chnaif]

@allroundexperts and I will be taking this as it came from new feature of non-modal popover.
cc: @dangrous

[melvin-bot]

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

📣 @allroundexperts Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

❌ There was an error making the offer to @aimane-chnaif for the Contributor role. The BZ member will need to manually hire the contributor. cc @thienlnam

[melvin-bot]

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @allroundexperts Please request via NewDot manual requests for the Contributor role ($1000)

[mananjadhav]

Considering this is a regression and @aimane-chnaif @allroundexperts are handling it, I am unassigning myself.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.57-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: popover without overlay changes #24515

If no regressions arise, payment will be issued on 2023-09-04. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @allroundexperts does not require payment (Eligible for Manual Requests)
• @aimane-chnaif requires payment offer (Reviewer)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[zanyrenney]

Asking @Beamanator in DM about the deploy block and it being a regression here before doing the payment!

[Beamanator]

Looks like this was potentially linked as a regression (see link in this comment) but that was taken back as the PR for this issue actually fixes a few regressions from a different PR.

So I think we're good to pay this out as normal, though @dangrous it seems you had some thoughts about the original PR that caused these regressions? I think we should be good to pay this out as normal, right?

[dangrous]

I thiiiink we were deciding that no payment needed for this one since it was a regression on that other PR - but lemme know if you disagree!

[allroundexperts]

That's correct!

[zanyrenney]

Payment summary

As this was a regression from the PR there is no payment for the fix.

There is also no payment for the bug reported because it was reported by the internal Applause team, we are good to close this one out. Thanks everyone!