[HOLD for payment 2024-08-09] [$250] Expensify Card - Hitting Enter on limit amount page does not proceed to the next page

lanitochka17 commented 1 month ago

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: 9.0.6-1 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Go to workspace settings > Expensify Card (enable in More features)
Click Issue new card
Proceed to Step 4 - Set a limit
Enter an amount, then hit Enter (keyboard)

Expected Result:

Hitting Enter on limit amount page will proceed to the next page

Actual Result:

Hitting Enter on limit amount page does not proceed to the next page

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/4760deda-0a03-4a2a-b886-18b6623471ff

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0131bf3ff1bba82181
Upwork Job ID: 1812597871244092170
Last Price Increase: 2024-07-21
Automatic offers:

rayane-djouah | Reviewer | 103253113
nyomanjyotisa | Contributor | 103253115

Issue Owner

Current Issue Owner: @adelekennedy

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @srikarparsi (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: 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:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

lanitochka17 commented 1 month ago

@srikarparsi FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 1 month ago

We think that this bug might be related to #wave-collect - Release 2

nyomanjyotisa commented 1 month ago

Proposal

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

Hitting Enter on limit amount page does not proceed to the next page

What is the root cause of that problem?

Default disablePressOnEnter here is true https://github.com/Expensify/App/blob/105dce5151053bf6c81e60e68a610dd888c5da45/src/components/Form/FormWrapper.tsx#L66

We don't set disablePressOnEnter on FormProvider here

https://github.com/Expensify/App/blob/105dce5151053bf6c81e60e68a610dd888c5da45/src/pages/workspace/card/issueNew/LimitStep.tsx#L80-L90

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

Add disablePressOnEnter={false} on <FormProvider> here

What alternative solutions did you explore? (Optional)

nkdengineer commented 1 month ago

Proposal

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

Hitting Enter on limit amount page does not proceed to the next page

What is the root cause of that problem?

We don't pass disablePressOnEnter={false} into FormProvider as in other pages like RatePage, WorkspaceNewRoomPage, ...

https://github.com/Expensify/App/blob/105dce5151053bf6c81e60e68a610dd888c5da45/src/pages/workspace/card/issueNew/LimitStep.tsx#L80

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

Pass disablePressOnEnter={false} into FormProvider here

https://github.com/Expensify/App/blob/105dce5151053bf6c81e60e68a610dd888c5da45/src/pages/workspace/card/issueNew/LimitStep.tsx#L80

What alternative solutions did you explore? (Optional)

nyomanjyotisa commented 1 month ago

Proposal updated

Julesssss commented 1 month ago

Not a blocker, this is a minor problem.

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

srikarparsi commented 1 month ago

Hi @adelekennedy! Do you know if it's expected that pressing enter takes you to the next screen? I'm not sure if this is something we want to fix. I can also check in product.

bernhardoj commented 1 month ago

Proposal

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

Pressing Enter doesn't proceed the user to the next page in the limit amount page.

What is the root cause of that problem?

The limit amount page is wrapped with a FormProvider. In FormProvider, if we press enter on an input, it will trigger onSubmitEditing and submit the form, but we will only handle it if shouldSubmitForm is true. https://github.com/Expensify/App/blob/58e257532290aa0228f1c668ed461a15f1b2b136/src/components/Form/FormProvider.tsx#L268-L275

shouldSubmitForm is calculated inside InputWrapper. https://github.com/Expensify/App/blob/58e257532290aa0228f1c668ed461a15f1b2b136/src/components/Form/InputWrapper.tsx#L29-L59

If the InputComponent is included in textInputBasedComponents, then we always return shouldSubmitForm as true for a single line. Otherwise, the shouldSubmitForm value will rely on the props that we pass, and there is no default value for it, which means it will be false.

For the limit amount page, the input that we use is AmountForm, which isn't included in textInputBasedComponents, https://github.com/Expensify/App/blob/58e257532290aa0228f1c668ed461a15f1b2b136/src/components/Form/InputWrapper.tsx#L14

and we don't set the shouldSubmitForm prop to the AmountForm, https://github.com/Expensify/App/blob/58e257532290aa0228f1c668ed461a15f1b2b136/src/pages/workspace/card/issueNew/LimitStep.tsx#L91-L97

that's why pressing Enter does nothing compared to other pages, such as new task.

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

We have 2 options. Add AmountForm to textInputBasedComponents (preferrable), https://github.com/Expensify/App/blob/58e257532290aa0228f1c668ed461a15f1b2b136/src/components/Form/InputWrapper.tsx#L14

or pass shouldSubmitForm as true for AmountForm input wrapper. https://github.com/Expensify/App/blob/58e257532290aa0228f1c668ed461a15f1b2b136/src/pages/workspace/card/issueNew/LimitStep.tsx#L91-L97

srikarparsi commented 1 month ago

It does looking like pressing enter to go to the next page works everywhere else so let's get this fixed. Making this external to pick a proposal.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0131bf3ff1bba82181

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

@srikarparsi, @adelekennedy, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

rayane-djouah commented 1 month ago

Thank you, everyone, for your proposals. All three proposals are valid; however, I am opting for the first proposal. Although it was updated after @nkdengineer's proposal (Initially it suggested adding disablePressOnEnter={false} also in <FormWrapper> which is unnecessary, but It's a detail that we wouldn't have missed during PR review.). Most importantly, it addresses the disablePressOnEnter={false} missing prop in <FormProvider> usage within LimitStep. I will defer to @srikarparsi for the final decision on this matter.

@nyomanjyotisa proposal looks good to me.

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 1 month ago

Current assignee @srikarparsi is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

bernhardoj commented 1 month ago

@rayane-djouah any thoughts on my proposal? I don't think disabling disablePressOnEnter fixes the root cause of the issue. You can see many pages don't disable that, yet pressing enter still works submitting the form.

srikarparsi commented 1 month ago

Bump on the above comment @rayane-djouah whenever you have a chance. Just want to understand the rationale.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

rayane-djouah commented 1 month ago

Sorry - I will re-review shortly

rayane-djouah commented 1 month ago

@bernhardoj, your proposal ensures that the form is submitted when an input is focused. However, in the "submit expense" flow, we submit the form when the Enter key is pressed even if no input is focused. I've raised a question regarding the expected behavior on Slack here.

rayane-djouah commented 1 month ago

@srikarparsi - Based on the Slack discussion, we concluded that the Enter key should submit the form even if no value is focused. Additionally, @bernhardoj's proposal suggested following the initiative from this PR, which addresses the inconsistent text input submission experience between mobile web and native platforms by allowing form submission using the mobile keyboard's Enter key. However, since we're using the number pad component for the amount input on mobile, there's no need to include AmountForm in textInputBasedComponents or to pass shouldSubmitForm as true for the AmountForm input wrapper.

To avoid this bug in other app parts, I propose making the disablePressOnEnter default option false and only passing disablePressOnEnter={true} to FormProvider when necessary. (cc @@nyomanjyotisa)

Let's move forward with @nyomanjyotisa's proposal.

melvin-bot[bot] commented 1 month ago

@srikarparsi, @adelekennedy, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

rayane-djouah commented 1 month ago

friendly bump @srikarparsi ^^

srikarparsi commented 1 month ago

This sounds good to me

melvin-bot[bot] commented 1 month ago

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

📣 @nyomanjyotisa 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

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

https://github.com/Expensify/App/pull/46177

If no regressions arise, payment will be issued on 2024-08-09. :confetti_ball:

For reference, here are some details about the assignees on this issue:

@nyomanjyotisa requires payment automatic offer (Contributor)
@rayane-djouah requires payment automatic offer (Reviewer)

melvin-bot[bot] commented 1 month ago

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:

[ ] [@rayane-djouah] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@rayane-djouah] 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:
[ ] [@rayane-djouah] 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:
[ ] [@rayane-djouah] Determine if we should create a regression test for this bug.
[ ] [@rayane-djouah] 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.
[ ] [@adelekennedy] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

adelekennedy commented 3 weeks ago

Payments made! @rayane-djouah just the checklist here!

rayane-djouah commented 3 weeks ago

[x] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/44376
[x] 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: https://github.com/Expensify/App/pull/44376/files#r1712720155
[x] 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: N/A
[x] Determine if we should create a regression test for this bug. Yes
[x] 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.

Regression Test Proposal

General regression test steps for forms:

Open the app.
Navigate to any page that contains a form.
Focus on a non-multiline input field and enter text.
Press the Enter key on the keyboard.
Verify that pressing the enter key submits the form.

Regression test steps specific to this bug (The form on limit amount page):

Go to workspace settings > Expensify Card (enable in More features).
Click Issue new card.
Proceed to Step 4 - Set a limit.
Enter an amount, then press the Enter key on the keyboard.
Verify that hitting Enter on limit amount page will proceed to the next page.

Do we agree 👍 or 👎

adelekennedy commented 3 weeks ago

makes sense to me! @srikarparsi any additional thoughts on the steps above?

srikarparsi commented 3 weeks ago

Nope, the steps look good to me as well!

Expensify / App