[HOLD for payment 2023-09-12] [HOLD for payment 2023-09-11] [$500] Request money - Enter key does not move to next page on big number pad page

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

---

Issue found when executing PR #26520

Action Performed:

1. Go to staging.new.expensify.com
2. Go to + > Request money
3. Enter an amount
4. Hit Enter on the keyboard

Expected Result:

You can press enter on both, amount and Distance request page to continue to a next step

Actual Result:

Request money flow is not proceeded to the next page

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.62-1

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

Bug6186267_20230902_234752.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

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

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

[mkhutornyi]

Proposal

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

Request money - Enter key does nothing on Manual request amount input page

What is the root cause of that problem?

This came from 5b8cd86 where pressOnEnter prop is added to DistanceRequest. cc: @hayata-suenaga

Then why does this happen?
We render all tabs on Request money page in the order of left > right.
So Distance tab page is rendered after Manual tab page render.
So Button's enter key is subscribed in MoneyRequestAmountForm first. And then DistanceRequest next.
So when user press Enter key, DistanceRequest's button onPress takes precedence over MoneyRequestAmountForm's button.

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

Step 1. set pressOnEnter prop value conditionally based on page active status.

const isFocused = useIsFocused();
<Button pressOnEnter={isFocused} />

Step 2. Update Button component to dynamically subscribe/unsubscribe enter key listener based on pressOnEnter prop change.
This logic will be added in componentDidUpdate

What alternative solutions did you explore? (Optional)

remove pressOnEnter prop from DistanceRequest's button

[napster125]

Proposal

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

Request money - Enter key does not move to next page on new money request page.

What is the root cause of that problem?

The root cause of this problem lies within the Button component, specifically in the way the Enter key listener is set up.

App/src/components/Button/index.js

Lines 167 to 190 in 52b1e3e

	componentDidMount() {
	if (!this.props.pressOnEnter) {
	return;
	}
	const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
	// Setup and attach keypress handler for pressing the button with Enter key
	this.unsubscribe = KeyboardShortcut.subscribe(
	shortcutConfig.shortcutKey,
	(e) => {
	if (!validateSubmitShortcut(this.props.isFocused, this.props.isDisabled, this.props.isLoading, e)) {
	return;
	}
	this.props.onPress();
	},
	shortcutConfig.descriptionKey,
	shortcutConfig.modifiers,
	true,
	false,
	this.props.enterKeyEventListenerPriority,
	false,
	);
	}

Currently, this listener is only established during the componentDidMount lifecycle. This issue primarily occurs on the "New Request Money" page because the text field starts empty, causing the isDisabled property to be initially set to true, and subsequently to false as the user inputs data. However, the Button component does not respond to this change in isDisabled during the componentDidUpdate lifecycle, preventing the Enter key event from being properly handled.

It's worth noting that this issue is observed in the development environment but is not likely a regression of commit, as the behavior of the Button component may be expected.

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

To resolve this issue, I recommend adding the same logic for setting up the Enter key listener in the componentDidUpdate lifecycle when isDisabled changes. This modification ensures consistent behavior regardless of the initial state of isDisabled.

componentDidMount() {
    this.setupEnterKeyListener();
}
componentDidUpdate(prevProps) {
    if (prevProps.isDisabled === this.props.isDisabled || this.props.isDisabled) {
        return;
    }
    this.setupEnterKeyListener();
}
componentWillUnmount() {
    // Cleanup event listeners
    if (!this.unsubscribe) {
        return;
    }
    this.unsubscribe();
}

What alternative solutions did you explore? (Optional)

n/a

Result

6.mp4

[mountiny]

@mkhutornyi I think your solution sounds good and I would prefer it over @napster125 proposed way as we dont want to mess with Button component logic right now give its such heavily used component.

Can you describe how you gonna get the isActiveTab?

remove pressOnEnter prop from DistanceRequest's button

this will make it work for other pages, but then it does not work on the Distance request page.

[mountiny]

I think I will just remove the pressOnEnter now because I am not sure if its trivial to get the tab index. I think we can live without the Enter working on the distance given it will be mainly used on mobile.

[napster125]

@mountiny it's impossible because this line will prevent the enter key event when isDisabled is true.

https://github.com/Expensify/App/blame/52b1e3e9abaabdaf79c0eeafef5665292cf2ab9c/src/components/Button/index.js#L178

[mountiny]

@napster125 what is impossible specifically?

[napster125]

@mountiny if so, your PR won't fix this issue. without pressOnEnter, we can't move to next page.

[b4s36t4]

we can't move to next page.

Even if we click?

[napster125]

@napster125 what is impossible specifically?

This is from my proposal.

Currently, this listener is only established during the componentDidMount lifecycle. This issue primarily occurs on the "New Request Money" page because the text field starts empty, causing the isDisabled property to be initially set to true, and subsequently to false as the user inputs data. However, the Button component does not respond to this change in isDisabled during the componentDidUpdate lifecycle, preventing the Enter key event from being properly handled.

@mountiny without resolving this, we can't fix the issue when enter key is pressed.

[napster125]

we can't move to next page.

Even if we click?

No, I mean when we press enter key.

[mountiny]

we can't move to next page.

Yeah I know:

I think I will just remove the pressOnEnter now because I am not sure if its trivial to get the tab index. I think we can live without the Enter working on the distance given it will be mainly used on mobile.

I think that is better given the urgency, basically the flow we need for the conference will be people on their phones mainly so if the enter does not work on the distance page, thats fine, but we prefer to fix this on the other pages.

I would then let this issue to be exported to figure out a general solution which will enable enter on all pages so feel free to think about that

[napster125]

@mountiny if so, we don't need to remove pressOnEnter.

[mkhutornyi]

Can you describe how you gonna get the isActiveTab?

@mountiny It's pretty simple. react-navigation supports useIsFocused for Tab navigator as well

[mkhutornyi]

I can raise PR right now if my solution is acceptable

[dylanexpensify]

Ah got it - @parasharrajat so to confirm:

• We need to pay you for internal review of PR in deploy blocker?
• We haven't selected a proposal yet for this issue? cc @thesahindia

Is that right?

[parasharrajat]

@dylanexpensify Correct.

[mountiny]

What is the bug still left here to solve?

[parasharrajat]

We disabled the Enter key submission on the Distance Request page to solve this DB which we thought of pursuing further. #26588 (comment)

[mountiny]

Ok, @dylanexpensify maybe we could create a new ticket for this issue so we can keep stuff separate and cleaner.

[parasharrajat]

In that case, please assign me as C+ here for clarity

[dylanexpensify]

Done ^

[dylanexpensify]

@parasharrajat mind applying here: https://www.upwork.com/jobs/~019e7a2b44273824ea

[mkhutornyi]

@mountiny Am I eligible for any compensation here? As my proposal (correct root cause and alternative solution) might have helped fixing this deploy blocker.

[parasharrajat]

@dylanexpensify I will request it.

Did you open the new issue?

[parasharrajat]

@mkhutornyi Reporting will be paid off when the issue is completely solved.

[dylanexpensify]

@parasharrajat yes here!

[mountiny]

I am confused whats he status on this one, can we close it out?

[b4s36t4]

@mountiny I think you can close this as there's another issue created for tracking the main issue. #28022

[mountiny]

thanks! Feel free to reopen if closing is not the best path forwards

[parasharrajat]

Payment requested as per #26588 (comment) #26588 (comment)

@dylanexpensify Can you post the payment summary here? Thanks.

[dylanexpensify]

Payment summary:

• contributor+: @parasharrajat $500

[JmillsExpensify]

$500 payment approved for @parasharrajat based on BZ summary.

[mkhutornyi]

Am I eligible for any compensation here? As my proposal (correct root cause and alternative solution) might have helped fixing this deploy blocker.

---

Off topic:
@parasharrajat gave 👎 on my main solution but unfortunately his solution of allowing bubble caused another regression - #29051, while mine wouldn't have caused.

[parasharrajat]

@mkhutornyi You are eligible on #28022 as discussed before.