[$500] Expense - Unable to save Merchant by hitting Enter in confirmation page

[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: v1.4.44-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com.
2. Go to FAB > Request money > Manual.
3. Enter amount and select participant.
4. Proceed to confirmation page.
5. Click Show more > Merchant.
6. Enter a merchant and hit Enter.

Expected Result:

The Merchant will be saved when hitting Enter.

Actual Result:

The Merchant is not saved when hitting Enter.
This issue also occurs with Description.

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

Bug6393850_1709032184834.20240227_133522.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0173bf7e04af9fbaa2
• Upwork Job ID: 1762479426479763456
• Last Price Increase: 2024-02-27

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0173bf7e04af9fbaa2

[melvin-bot]

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

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

[kbecciv]

We think that this bug might be related to #wave5-free-submitters
CC @dylanexpensify

[dukenv0307]

Proposal

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

The Merchant is not saved when hitting Enter.
This issue also occurs with Description.

What is the root cause of that problem?

disablePressOnEnter is true by default. So we can't submit form by pressing enter key

App/src/components/Form/FormWrapper.tsx

Line 60 in cfa0ae3

disablePressOnEnter = true,

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

Pass disablePressOnEnter as false into FormProvider. I think shouldSubmitForm is used for multiline input

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

Line 141 in cfa0ae3

<FormProvider

What alternative solutions did you explore? (Optional)

NA

[Pujan92]

Please link the offended PR as it is not reproducible on production.

[grgia]

I don't think this needs to block deploy

[Pujan92]

It is from #31606 where BaseOptionsSelector enter callback triggered, same is happening for the description step form too. cc: @Piotrfj

[BhuvaneshPatil]

Proposal

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

Expense - Unable to save Merchant by hitting Enter in confirmation page

What is the root cause of that problem?

The pr is - #31606
The root cause of problem is we have subscribed to enter shortcut on BaseOptionsSelector, that prevents the form from submitting, and that avoid bubbling of that event.

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

We shall unsubscribe to Enter shortcut key event.
When we go to different pages (ex - Description, Merchant)
To Do this, we need to make following changes -

1. For use effect here -
   

   App/src/components/OptionsSelector/BaseOptionsSelector.js

   Lines 370 to 377 in 04644d8

   	useEffect(() => {
   	// Unregister the shortcut before registering a new one to avoid lingering shortcut listener
   	enterSubscription.current();
   	if (!disableEnterShortCut) {
   	subscribeToEnterShortcut();
   	}
   	// eslint-disable-next-line react-hooks/exhaustive-deps
   	}, [disableEnterShortCut]);

   
   we shall also check if isFocused (not from prop) is true. This means we are only subscribing if we are on the page that has BaseOptionsSelector, not the inside. Because then it will act as we press enter it's on the confirmation list.

Changes

useEffect(() => {
    // Unregister the shortcut before registering a new one to avoid lingering shortcut listener
    enterSubscription.current();
    if (!disableEnterShortCut && isFocused) {
        subscribeToEnterShortcut();
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [disableEnterShortCut]);

2. While subscribing to events, we shall also check if the window is focused (the value we using `useIsFocused()`) for this use effect

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 379 to 387 in 04644d8

	useEffect(() => {
	if (props.isFocused) {
	subscribeToEnterShortcut();
	subscribeToCtrlEnterShortcut();
	} else {
	unSubscribeFromKeyboardShortcut();
	}
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [props.isFocused]);

Changes

useEffect(() => {
    if (isFocused) {
        subscribeToEnterShortcut();
        subscribeToCtrlEnterShortcut();
    } else {
        unSubscribeFromKeyboardShortcut();
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [isFocused]);

3. We need to remove the subscriptions done by the hooks cause they kind of pollute the events (spent alot of time in this )
   

   App/src/components/OptionsSelector/BaseOptionsSelector.js

   Lines 305 to 309 in 069efef

   	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, selectFocusedOption, {
   	shouldBubble: !allOptions[focusedIndex],
   	captureOnInputs: true,
   	});
   	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER, selectOptions, {captureOnInputs: true});

result -

Screen.Recording.2024-02-28.at.2.32.24.AM.mov

What alternative solutions did you explore? (Optional)

[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.

[Piotrfj]

Update: re-refactoring is ready to be tested, it fixes all the regression issues I was aware of. Please test it (especially the tags, because I could not yet replicate the scenario on my site).
The fix is based on the @roryabraham solution, so special thanks for him!
Link to PR: #37494

[melvin-bot]

@Pujan92, @grgia Eep! 4 days overdue now. Issues have feelings too...

[grgia]

I take it we can close this one out @Pujan92 ?

[situchan]

Offending PR was reverted - #31606 (comment)
Fine to close