Expensify
Expensify Card - Hitting Enter on limit amount page does not proceed to the next page
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: 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
Triggered auto assignment to @srikarparsi (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
: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.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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
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)
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)
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.
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.
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
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.
Job added to Upwork: https://www.upwork.com/jobs/~0131bf3ff1bba82181
Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah (External)
@srikarparsi, @adelekennedy, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
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
Current assignee @srikarparsi is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
@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.
Bump on the above comment @rayane-djouah whenever you have a chance. Just want to understand the rationale.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@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.
@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.
@srikarparsi, @adelekennedy, @rayane-djouah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
📣 @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 📖
Reviewing label has been removed, please complete the "BugZero Checklist".
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)