[HOLD for payment 2024-02-07] [HOLD for payment 2024-02-05] IOU - CMD+Enter command does not work when selecting currency

[kbecciv]

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

---

Version Number: 1.4.32-2
Reproducible in staging?: y
Reproducible in production?: n
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 https://staging.new.expensify.com/
2. Log in under your HT account
3. Tap on the green plus button (FAB)
4. Select Request money
5. In RHP, click on currency
6. Press CMD+Enter

Expected Result:

The CMD+Enter command should close the currency toggle, returning the user to the money amount entry page

Actual Result:

CMD+Enter command does not work when selecting currency

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

Bug6355597_1706262292979.Recording__1201.mp4

View all open jobs on GitHub

[github-actions]

👋 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 @cristipaval (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #vip-bills
CC @davidcardoza

[dukenv0307]

Proposal

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

• IOU - CMD+Enter command does not work when selecting currency

What is the root cause of that problem?

• In here, onConfirm is ()=>{} so CMD+ENTER does not do anything

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

• We can pass the onConfirm function to:
  

  App/src/pages/iou/request/step/IOURequestStepCurrency.js

  Line 133 in e92ffbf

  <SelectionList

• So the above will become:

                <SelectionList
                    onConfirm={(option) => {
                        confirmCurrencySelection(option);
                    }}
                    ...

• Then in BaseSelectionList, we update the onConfim function so it can receive option param and works properly

What alternative solutions did you explore? (Optional)

• In here, in this case (selecting currency), we should use selectFocusedOption rather than onConfirm.

• To keep the backward compatibility, we can add the condition to know when we should use onConfirm, and when we should use selectFocusedOption. For example, create param shouldConsiderEnterSamAsCmdEnter. if true, use selectFocusedOption

[paultsimura]

The offending PR is this migration: #34794

cc: @allroundexperts

[allroundexperts]

@paultsimura Thanks for pointing out. Do you mind giving a short explanation of why this is the case?

[paultsimura]

@paultsimura Thanks for pointing out. Do you mind giving a short explanation of why this is the case?

I'll try 🙂

Here, we're now setting the default value for onConfirm (previously it was undefined, and I think it should remain such):

App/src/components/SelectionList/BaseSelectionList.tsx

Line 46 in e92ffbf

onConfirm = () => {},

Then, here we use the !!onConfirm check, which is now true (unless undefined was passed explicitly):

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 386 to 391 in e92ffbf

	/** Calls confirm action when pressing CTRL (CMD) + Enter */
	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm, {
	captureOnInputs: true,
	shouldBubble: !flattenedSections.allOptions[focusedIndex],
	isActive: !disableKeyboardShortcuts && !!onConfirm && isFocused,
	});

But since onConfirm is an empty function, this command does nothing.
And this shorcut overlaps with this line:

App/src/components/SelectionList/BaseSelectionList.tsx

Line 423 in 15f349a

onSubmitEditing={selectFocusedOption}

Now pressing CMD+Enter does not trigger onSubmitEditing (which is the part that actually works in Prod currently – if you unfocus the input and use CMD+Enter in prod, it does nothing either) because the keyboard listener with empty function is a higher level one, and it gets executed instead.

[allroundexperts]

@filip-solecki This should be a quick fix. Can you raise a PR?

[paultsimura]

Proposal

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

The CMD+Enter does nothing on selection lists with no onConfirm passed

What is the root cause of that problem?

The offending PR is this migration: #34794

We're now setting the default value for onConfirm (previously it was undefined):

App/src/components/SelectionList/BaseSelectionList.tsx

Line 46 in e92ffbf

onConfirm = () => {},

Then, here we use the !!onConfirm check, which is now true (unless undefined was passed explicitly):

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 386 to 391 in e92ffbf

	/** Calls confirm action when pressing CTRL (CMD) + Enter */
	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm, {
	captureOnInputs: true,
	shouldBubble: !flattenedSections.allOptions[focusedIndex],
	isActive: !disableKeyboardShortcuts && !!onConfirm && isFocused,
	});

But since onConfirm is an empty function, this command does nothing.

And this shortcut overlaps with this line:

App/src/components/SelectionList/BaseSelectionList.tsx

Line 423 in 15f349a

onSubmitEditing={selectFocusedOption}

Now pressing CMD+Enter does not trigger onSubmitEditing (which is the part that actually works in Prod currently – if you unfocus the input and use CMD+Enter in prod, it does nothing either) because the keyboard listener with an empty function is a higher level one, and it gets executed instead.

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

Instead of setting the default value to onConfirm, we should leave it undefined by default, and instead change this code to add the listener only if onConfirm is not falsy:

    if (!!onConfirm) {
        /** Calls confirm action when pressing CTRL (CMD) + Enter */
        useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm, {
            captureOnInputs: true,
            shouldBubble: !flattenedSections.allOptions[focusedIndex],
            isActive: !disableKeyboardShortcuts && isFocused,
        });
    }

App/src/components/SelectionList/BaseSelectionList.tsx

Lines 386 to 391 in e92ffbf

	/** Calls confirm action when pressing CTRL (CMD) + Enter */
	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm, {
	captureOnInputs: true,
	shouldBubble: !flattenedSections.allOptions[focusedIndex],
	isActive: !disableKeyboardShortcuts && !!onConfirm && isFocused,
	});

What alternative solutions did you explore? (Optional)

[allroundexperts]

    if (!!onConfirm) {
        /** Calls confirm action when pressing CTRL (CMD) + Enter */
        useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, onConfirm, {
            captureOnInputs: true,
            shouldBubble: !flattenedSections.allOptions[focusedIndex],
            isActive: !disableKeyboardShortcuts && isFocused,
        });
    }

This violates the rule of the hooks.

[filip-solecki]

@allroundexperts - sure, I am preparing PR

[filip-solecki]

@allroundexperts - PR created, right now onConfirm function may be undefined and if it is code runs selectFocusedOption function which select focused element

[allroundexperts]

Reviewing in a moment.

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

• [CP Staging] Fix CMD+Enter in SelectionList #35241

If no regressions arise, payment will be issued on 2024-02-05. 🎊

[melvin-bot]

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

• [CP Staging] Fix CMD+Enter in SelectionList #35241

If no regressions arise, payment will be issued on 2024-02-07. 🎊

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[cristipaval]

This is a regression fixed and reviewed by the contributors who worked on the original PR. No payment due here.