[HOLD for payment 2023-10-25] [$500] Fab - App disables CTRL+K / CTRL+SHIFT+K shortcuts on FAB menu option pages

[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.81.5
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: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697011802723769

Action Performed:

1. Open the app
2. Click on green plus and click on any FAB menu option eg: 'save the world'
3. Click 'CTRL+K' or 'CTRL+SHIFT+K' and observe that it does not work

Expected Result:

App should not disable CTRL+K / CTRL+SHIFT+K shortcuts

Actual Result:

App disables CTRL+K / CTRL+SHIFT+K shortcuts on FAB menu option pages

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

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

mac.chrome.ctrl.k.not.working.save.the.world.mov

windows.chrome.ctrl.k.not.working.save.the.world.mp4

Recording.4952.mp4

MacOS: Desktop

mac.desktop.ctrl.k.not.working.save.the.world.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~015979bf65c1172e21
• Upwork Job ID: 1712146924604538880
• Last Price Increase: 2023-10-11

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~015979bf65c1172e21

[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 - @ntdiary (External)

[pradeepmdk]

Proposal

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

App disables CTRL+K / CTRL+SHIFT+K shortcuts on FAB menu option pages

What is the root cause of that problem?

when popovermenu open we are setting closemodal but we never cleaned

App/src/components/PopoverWithoutOverlay/index.js

Line 48 in b820046

Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);

so here when close modal is present its doesn't onModalCloseCallback trigger

App/src/libs/actions/Modal.ts

Line 17 in b820046

function close(onModalCloseCallback: () => void, isNavigating = true) {

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

App/src/components/PopoverWithoutOverlay/index.js

Line 48 in b820046

Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);

here on cleanup function we should clear the modal

 Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);

        return () => {
            Modal.setCloseModal(null)
        }
        // We want this effect to run strictly ONLY when isVisible prop changes
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [props.isVisible]);

What alternative solutions did you explore? (Optional)

App/src/components/PopoverMenu/index.js

Line 81 in f557852

if (selectedItemIndex.current !== null) {

here when once popover menu selected we should clear the modal
Modal.setCloseModal(null)

[maxconnectAbhi]

Proposal

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

Fab - App disables CTRL+K / CTRL+SHIFT+K shortcuts on FAB menu

What is the root cause of that problem?

Certainly! It seems that using the keyboard shortcuts CTRL+K or CTRL+SHIFT+K should open a search modal. However, if you've already opened a modal using the FAB menu, these shortcuts won't trigger the search modal again.

https://github.com/Expensify/App/blob/f5578523a10653b03de827e38f99f3ba54241a69/src/libs/Navigation/AppNavigator/AuthScreens.js#L185C8-L198C11

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

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

To ensure the CTRL+K or CTRL+SHIFT+K shortcuts activate the search modal, make sure to close any currently open modals beforehand through Modal.setCloseModal() function.

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

result

Clip.mov

What alternative solutions did you explore? (Optional)

NA

[ishpaul777]

regression from #29251 confirmed by reverting changes from PR

[astrohunter62]

Proposal

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

App disables CTRL+K / CTRL+SHIFT+K shortcuts after the modal is closed.

What is the root cause of that problem?

App/src/components/PopoverWithoutOverlay/index.js

Lines 42 to 46 in 64d3e39

	// We prevent setting closeModal function to null when the component is invisible the first time it is rendered
	if (!firstRenderRef.current || !props.isVisible) {
	firstRenderRef.current = false;
	return;
	}

The purpose of this code block is to prevent setting closeModal function to null when the component is invisible the first time it is rendered.
It's not correctly working because !firstRenderRef.current.

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

 if (firstRenderRef.current || !props.isVisible) { 
     firstRenderRef.current = false; 
     return; 
 } 

Results:

29341.webm

[0xmiros]

This should be handled as regression as it's within regression period.
This came from one of #29251 (cc: @s-alves10 @Ollyws ) or #28486 (cc: @dukenv0307 @parasharrajat )

Context: #27639 (comment)

[s-alves10]

The root cause of this issue is the wrong condition here

App/src/components/PopoverWithoutOverlay/index.js

Lines 43 to 48 in 0c7f390

	if (!firstRenderRef.current || !props.isVisible) {
	firstRenderRef.current = false;
	return;
	}
	firstRenderRef.current = false;
	Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);

This looks weird. This should be updated like this

        if (firstRenderRef.current && !props.isVisible) {
            firstRenderRef.current = false;
            return;
        }
        firstRenderRef.current = false;
        Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);

@dukenv0307 let me know your thought on this please

[mallenexpensify]

@ntdiary can you review the above proposals plz?

[dukenv0307]

@s-alves10 In my PR I used prevIsVisible === props.isVisible to prevent setting closeModal function to null when the first time the component is rendered with isVisible prop as false and (!firstRenderRef.current || !props.isVisible) is used to not earlier return if the component is rendered the first time with isVisible as true.

More context here #28241 (comment), #28486 (comment)

In your PR, prevIsVisible === props.isVisible condition is removed makes the logic is wrong.

cc @parasharrajat

[ntdiary]

@ntdiary can you review the above proposals plz?

Sure, will review soon. : )

[astrohunter62]

@ntdiary Could you review my proposal?

[s-alves10]

@dukenv0307

when the first time the component is rendered with isVisible prop as false. This condition is enough with this?

if (firstRenderRef.current && !props.isVisible)

[dukenv0307]

@s-alves10 Yes. I mean this condition is wrong after your PR is merged.

[s-alves10]

I'm not sure why you used such complex condition

if (prevIsVisible === props.isVisible && (!firstRenderRef.current || !props.isVisible))

prevIsVisible was removed because it has negative effect. We should remove that.

[parasharrajat]

Let's revert both PRs #28486 #29251. This regression seemed to be caused after #29251 changes.

Then @dukenv0307 and I can work on a better solution.

[melvin-bot]

Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[ntdiary]

@parasharrajat @ntdiary I am in the middle of solving other issue and I thinkk my PR could solve this issue and the other attachment navigation issue. What do you think about my PR? Thank you...

@tsa321, I believe your solution can work, but there are still some issues with PopoverWithoutOverlay itself.

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

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

props.onModalHide();, this else code block should only be executed after the popup is closed. Also, the race condition you mentioned in your proposal, I think it's because we are listening for the contextmenu event during the bubbling phase, so document receive event after the message dom element, which cause the second popup will show before the first one closes.

Eh, my bad, I didn't notice there's also a e.stopPropagation();. Also, can we use capture for contextmenu?

[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.86-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Revert "fix: remove unnecessary dependency" #29663

If no regressions arise, payment will be issued on 2023-10-25. 🎊

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:

• @ntdiary requires payment
• @dhanashree-sawant requires payment

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]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ntdiary] The PR that introduced the bug has been identified. Link to the PR:
• [@ntdiary] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@ntdiary] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@ntdiary] Determine if we should create a regression test for this bug.
• [@ntdiary] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[ntdiary]

Note: This issue has gone away. We reverted two related PRs (#29251, #28486), and their original issues have been addressed by the new PR #29152.

Finally, based on the discussion in Slack, we should create regression test for these cases, ensure the keyboard shortcuts can always work well.

case1:

1. Click the FAB button to open the global menu.
2. Press Esc to close the menu.
3. Press CTRL+K or CMD+K to open the search page.
4. Verify the search page can be opened correctly.

case2:

1. Open any chat with an attachment.
2. Click the attachment to open the preview modal.
3. Press CTRL+K or CMD+K to open the search page.
4. Verify the search page can be opened correctly.
5. Repeat step 1 and step 2.
6. reload the app.
7. Press CTRL+K or CMD+K to open the search page.
8. Verify the search page can be opened correctly.

Please feel free to let me know if I'm wrong. : )
cc @sakluger @parasharrajat @mollfpr

[mountiny]

Coming from slack, created an issue for this one so we have it in regression suite sooner than later https://github.com/Expensify/Expensify/issues/328177

[sakluger]

Thanks @mountiny!

I'm a bit unsure about payments here.

• We should definitely pay out @dhanashree-sawant for the bug report (Offer sent)
• @ntdiary it doesn't look like you reviewed any PRs, but you did review proposals and write the regression test steps. Do you think it's fair to pay you 50% of the base price for your work here, or were you thinking otherwise?

[dhanashree-sawant]

Hi @sakluger, by mistake you have sent me an offer of 500$ instead of 50$, can you rectify it once you are available?

[ntdiary]

@ntdiary it doesn't look like you reviewed any PRs, but you did review proposals and write the regression test steps. Do you think it's fair to pay you 50% of the base price for your work here, or were you thinking otherwise?

@sakluger, ha, don't worry, I think 50% of the base price is already very good. : )

[sakluger]

@dhanashree-sawant good catch! Thank you for letting me know, I've updated the offer 🙇
@ntdiary thanks for confirming, I send you an offer.

[ntdiary]

@ntdiary thanks for confirming, I send you an offer.

@sakluger, have accepted. 😄

[dhanashree-sawant]

Thanks @sakluger, it shows offer is withdrawn, can you check once you are available?

[sakluger]

@dhanashree-sawant in order to modify an offer that hasn't been accepted yet, I had to withdraw the offer and send a new one. Here is the new offer link, not sure if that offer works for you, but you should be able to find the new offer in your Upwork inbox. Can you let me know once you've accepted?

@ntdiary your payment is completed, thanks!

[dhanashree-sawant]

Thanks @sakluger, this is what it is displaying for new offer link, not sure how we can solve it.

[sakluger]

@dhanashree-sawant here's the offer link on my side: https://www.upwork.com/nx/wm/offer/27395089

Can you check your Upwork messages and see if there's a message from me on or around Thu, Oct 26 containing an offer? For you, it might be on Friday, Oct 27 because of the time zone difference.

[dhanashree-sawant]

No sorry I couldn't find any offer in the messages. If you check the URL in the screenshot, It seems like upwork is displaying offer not available for the same offer link. I think offers on this job won't work and may need a new job.

[sakluger]

Alright, new job post, let's try this again! https://www.upwork.com/jobs/~0133af5efe5e763c64

Please let me know if that still doesn't work for you.

[dhanashree-sawant]

Thanks yes now I was able to accept the offer.

[sakluger]

Great, thank you for your patience while we got that figured out 🙇

All done now, closing!