[$125] Request money - The 'Enter' key fails to select the email/workspace/phone when requesting money.

[lanitochka17]

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.4.24-0
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. Sign in to staging Expensify
2. Navigate to FAB > Request Money > Add amount, then press 'Enter' to select the email. Observe that the 'Enter' key does not work as expected

Expected Result:

The 'Enter' key should successfully select the email during the request for money

Actual Result:

The 'Enter' key does not select the email as intended during the request for money

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

Bug6337734_1704896732715.Screen_Recording_2024-01-10_at_5.12.27_AM.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 @youssef-lr (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[bernhardoj]

Proposal

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

Can't select a participant with the enter key.

What is the root cause of that problem?

This is a regression from #31354.

When we select a participant, we return early if the screen is not focused.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 335 to 341 in 55746c7

	selectFocusedOption(e) {
	const focusedItemKey = lodashGet(e, ['target', 'attributes', 'id', 'value']);
	const focusedOption = focusedItemKey ? _.find(this.state.allOptions, (option) => option.keyForList === focusedItemKey) : this.state.allOptions[this.state.focusedIndex];
	if (!focusedOption || !this.isFocused) {
	return;
	}

isFocused value initially is false.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Line 87 in 55746c7

this.isFocused = false;

and updated true inside the focus listener, but focus listener is called only if the screen is re-focused, not when the page is mounted, so isFocused is always false.

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 102 to 117 in 55746c7

	this.focusListener = this.props.navigation.addListener('focus', () => {
	// Screen coming back into focus, for example
	// when doing Cmd+Shift+K, then Cmd+K, then Cmd+Shift+K.
	// Only applies to platforms that support keyboard shortcuts
	if ([CONST.PLATFORM.DESKTOP, CONST.PLATFORM.WEB].includes(getPlatform())) {
	this.subscribeToKeyboardShortcut();
	}
	if (this.props.autoFocus && this.textInput) {
	this.focusTimeout = setTimeout(() => {
	this.textInput.focus();
	}, CONST.ANIMATED_TRANSITION);
	}
	this.isFocused = true;
	});

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

We can set the initial value of isFocused to true, but we still need to duplicate the code from the focus listener to the componentDidMount

isFocused is actually not needed because we already unsubscribe the shortcut when component blurs

[melvin-bot]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

[youssef-lr]

I think this can be demoted.

[melvin-bot]

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

[melvin-bot]

Current assignee @johncschuster is eligible for the Bug assigner, not assigning anyone new.

[bernhardoj]

@youssef-lr should I raise the PR now?

[situchan]

@hurali97 can you please check this?

[mountiny]

@bernhardoj I have reverted that PR locally and the enter still does not work

[bernhardoj]

@mountiny I tried switching to the commit before the PR and somehow it still didn't work, but after another refresh, the issue is gone.

[mountiny]

interesting, we are reverting it so you can confirm then, thank you!

[situchan]

Interesting. I am not able to reproduce this bug on main but instead enter key doesn't work on amount input page.
Both pages work fine on staging

Screen.Recording.2024-01-11.at.12.11.48.AM.mov

[mountiny]

Oh I was testing the amount page before, that was not fixed with the revert

[situchan]

Seems like the revert introduced this bug as not happening on staging 😄

[youssef-lr]

Yeah I can't reproduce the original issue, but the Enter key in the amount page is broken.

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

[youssef-lr]

Actually, I can only reproduce locally, but not on staging. Enter key is working fine in both amount page and participants selector.

[situchan]

Actually, I can only reproduce locally, but not on staging. Enter key is working fine in both amount page and participants selector.

yes, this will become deploy blocker on next deploy 😄

[bernhardoj]

The enter doesn't work on the amount page is a regression from #33055

The root cause is similar to the previous issue. The isFocused ref here is initially false and when we set up the config, the isActive is always false.

App/src/components/Button/index.tsx

Lines 153 to 164 in 7517d74

	const config = useMemo(
	() => ({
	isActive: pressOnEnter && !shouldDisableEnterShortcut && isFocused.current,
	shouldBubble: allowBubble,
	priority: enterKeyEventListenerPriority,
	shouldPreventDefault: false,
	}),
	// eslint-disable-next-line react-hooks/exhaustive-deps
	[shouldDisableEnterShortcut],
	);
	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, keyboardShortcutCallback, config);

Even if the ref is updated, the config memo is never re-calculated.

It's better to use useIsFocused hook just like before the PR and add the focused state to the deps. We don't need to worry that it will re-render the Button component because, in that PR, we already separate the shortcut logic into a separate empty component.

But after fixing this, you will notice that pressing enter will show an invalid amount error and that is because the shortcut callback here is never recreated when onPress instance is updated.

App/src/components/Button/index.tsx

Lines 142 to 151 in 7517d74

	const keyboardShortcutCallback = useCallback(
	(event?: GestureResponderEvent | KeyboardEvent) => {
	if (!validateSubmitShortcut(isDisabled, isLoading, event)) {
	return;
	}
	onPress();
	},
	// eslint-disable-next-line react-hooks/exhaustive-deps
	[isDisabled, isLoading],
	);

The onPress will call submitAndNavigateToNextPage

App/src/pages/iou/steps/MoneyRequestAmountForm.js

Lines 229 to 233 in 7517d74

	const submitAndNavigateToNextPage = useCallback(() => {
	if (isAmountInvalid(currentAmount)) {
	setFormError(translate('iou.error.invalidAmount'));
	return;
	}

which will check the currentAmount state, but it's always empty when executed inside the shortcut callback.

So, we should add onPress to the shortcut callback deps.

[hurali97]

@bernhardoj Thanks for pin pointing the issue, I have raised a PR fixing this 👍

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

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

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[melvin-bot]

This issue has not been updated in over 15 days. @johncschuster, @youssef-lr, @bernhardoj eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[youssef-lr]

This was fixed.