Closed lanitochka17 closed 11 months ago
Job added to Upwork: https://www.upwork.com/jobs/~01d106743c23a6474e
Triggered auto assignment to @flaviadefaria (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Platforms
in OP are β
)Triggered auto assignment to Contributor-plus team member for initial proposal review - @Ollyws (External
)
Confirm buttons have different names
and pressing enter does not submit a form on safari web (Not reproduce on emulator and live device)
We don't pass returnKeyType value for choosing submit button name
we can pass returnKeyType="done"
or other available name for this component
NA
The enter key text is not consistent across platforms.
We don't explicitly set the text for enter key in virtual keyboards. So the keyboards tries to gauge the context on its own, resulting in inconsistent behaviour.
We should set the enterKeyHint
prop for the TextInput
here:
https://github.com/Expensify/App/blob/b403ab5237e9710d9445240ddc20630a3420feb1/src/pages/signin/LoginForm/BaseLoginForm.js#L200-L203
This prop controls the value of text on enter key.
The possible values are:
I will suggest adding next
.
xx
Keyboards are not consistent.
preventDefault
here. When preventDefault
is triggered, the onSubmitEditing is not triggered, causing the form to not submit.returnKeyType
/enterKeyHint
in the TextInput here, so the browser will infer the value, which is different from native.preventDefault
is not executed if the callback is not executed. This can be done by a few ways:
a. [Recommended] The callback will now return a boolean indicating if it's executed or not. For example return false
here (and return true at the last)
In here, we only call preventDefault
if the callback is executed
let isExecuted = false;
if (_.isFunction(callback.callback)) {
isExecuted = callback.callback(event) !== false;
}
if (isExecuted && callback.shouldPreventDefault) {
event.preventDefault();
}
b. Or we can pass shouldPreventDefault
false when we register that listener
c. Or to make sure the component is not rendered prematurely on login screen, so no listener is registered, however this might still impact other inputs once that component is rendered after we login.
returnKeyType
(better because it's cross platform) or enterKeyHint
to the TextInput, I think next
is appropriateOther inputs might have the same issue, we might want to double check and fix similar to above.
Currently all onSubmitEditing
in the app does not work due to the above issues, but the issue doesn't show up because the submission is also handled by the Form button. So an alternative is we can remove all usage of onSubmitEditing
as a cleanup.
Enter key press not submitting form in iOS Safari
Root cause of the problem is simply there is onKeyPress handler in https://github.com/Expensify/App/blob/2160babec48a63cb93c660df4365bc4dc483ccb9/src/pages/signin/LoginForm/BaseLoginForm.js#L200
Adding following code in above file will ensure key press will be handled on pressing enter key.
onKeyPress={({nativeEvent}) => nativeEvent.code === "Enter" && validateAndSubmitForm()}
enterKeyHint/returnKeyType will simply set text label of return button, it will not submit form unless we have keypress handler attached to the textinput over which soft keyboard is being shown.
It should work on all platforms but it still needs to be cross verified if C+ picks this proposal.
Result
https://github.com/Expensify/App/assets/767439/234acab8-81a4-4ff4-8592-de3f908b7a05
N/A
@Ollyws any thoughts on the proposals above?
I can no longer reproduce the enter key issue, I think it may have been fixed by https://github.com/Expensify/App/pull/28374 can anyone else confirm?
@Ollyws yes it is fixed.
In that case all that's left is the enter key label and @ZhenjaHorbach's proposal looks good for that. πππ C+ reviewed
Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@Ollyws One note that this is not just signin page, this issue come up on https://github.com/Expensify/App/pull/28409 which also has one input and submit was not working. I assume this happens in other places too
@alitoshmatov @Ollyws I think this can be fixed in all places if we implement onKeyPress handler in TextInput component and pass it a prop submitOnEnter which should handle submitting form. Can be generic solution.
@Ollyws Hi can you give some feedback on my proposal here?
I can no longer reproduce the enter key issue, I think it may have been fixed by https://github.com/Expensify/App/pull/28374 can anyone else confirm?
This is indeed no longer reproducible due to that PR but that's a workaround that only accidentally fixes the issue. The root cause is still there and all usage of onSubmitEditing
in the app currently basically does not work (you can validate by put a log into those onSubmitEditing
callback). So I think we should still fix that.
cc @MonilBhavsar
I agree we should resolve the root cause and for all the forms. @Ollyws could you please take a look at the above comment.
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
@tienifr
you can validate by put a log into those onSubmitEditing callback
I tried this and onSubmitEditing
works fine when pressing enter in both BaseLoginForm.js
and BaseValidateCodeForm.js
:
It also seems to work fine in OptionsSelector, PDFPasswordForm, The bank account forms. I couldn't seem to find an instance where it didn't work.
Am I missing something?
I tried this and onSubmitEditing works fine when pressing enter in both BaseLoginForm.js and BaseValidateCodeForm.js:
@Ollyws you seemed to put the logging in validateAndSubmitForm
, can you put it in onSubmitEditing
form instead? You'll see the onSubmitEditing
is not triggered at all, validateAndSubmitForm
is triggered from the Submit button instead (which is listening to the Enter key press as well)
@tienifr I tried putting it in onSubmitEditing
and disabling pressOnEnter
or removing the button, and onSubmitEditing
fired fine for me. Tried this on multiple forms.
@tienifr I tried putting it in onSubmitEditing and disabling pressOnEnter or removing the button, and onSubmitEditing fired fine for me. Tried this on multiple forms.
@Ollyws I just tried it again on latest main and it still doesn't work for me. The platform that are affected here is mWeb (not sure if that's the platform you tested it on).
A quick validation: Can you comment out this line and follow the step in the OP of this issue on mWeb Safari to see if this happens pressing it won't submit the form unlike the native app
. If yes, that means onSubmitEditing
never fire (it's yes for me).
@tienifr Yes but isn't that because we're utilizing pressOnEnter
on the button? When we comment out that line the button is being triggered but we've commented out the callback so the form submission doesn't work.
If we disable pressOnEnter
on the button then onSubmitEditing
works fine.
Also, with this line commented out, removing this code doesn't seem to cause onSubmitEditing
to work, suggesting your proposal would not change this behaviour.
Triggered auto assignment to @stephanieelliott (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
@stephanieelliott I'm officially no longer in the BugZero team so reassigning this as it will not close until tomorrow.
@tienifr Yes but isn't that because we're utilizing pressOnEnter on the button? When we comment out that line the button is being triggered but we've commented out the callback so the form submission doesn't work.
@Ollyws you're right, it might have been fixed somewhere, let's proceed with the selected proposal then π.
@MonilBhavsar We're good to proceed with https://github.com/Expensify/App/issues/28569#issuecomment-1746358441. Thanks!
@Ollyws @stephanieelliott @MonilBhavsar this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
π£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πΈ
Thanks for clarifying! π
π£ @Ollyws π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @ZhenjaHorbach π 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 π
π£ @alitoshmatov π An offer has been automatically sent to your Upwork account for the Reporter role π Thanks for contributing to the Expensify app!
Offer link
PR will be ready today
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one π
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.88-11 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-10-30. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
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:
- [x] The PR that introduced the bug has been identified. Link to the PR:
We can't really say this was a regression from any PR, we just never added the returnKeyType to the login form.
- [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:
N/A
- [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.
I don't think a regression test would be helpful here as it's a very minor visual inconsistency that doesn't affect the flow in any meaningful way.
Summarizing payment on this issue:
Contributor+: @Ollyws $500 Upwork - PAID
Upwork job is here, no bonus is included on payout
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
Both keyboard should be the same, or at least pressing "Go" should submit a form
Actual Result:
Keyboards are not consistent. a) In safari instead of "Return" sign it says "Go" b) And pressing it won't submit the form unlike the native app
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.75-8
Reproducible in staging?: Yes
Reproducible in production?: Yes
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
Safari
https://github.com/Expensify/App/assets/78819774/b7c6dc15-1322-4fe9-9b28-800664d280cb
IOS app
https://github.com/Expensify/App/assets/78819774/ed88e01a-8af7-4aed-a4c0-cb0516abc14d
https://github.com/Expensify/App/assets/78819774/2cfe7d44-a94b-4849-82ef-3c93e9b373b4
Expensify/Expensify Issue URL:
Issue reported by: @alitoshmatov
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696083514018509
n+is%3Aissue+label%3A%22Help+Wanted%22)
Upwork Automation - Do Not Edit