melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kbecciv closed 1 year ago
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
laurenreidexpensify
commented
1 year ago Sending to Upwork
melvin-bot[bot]
commented
1 year ago Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new.
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~01ef7c83ea1ccdd30c
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @pecanoro (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
railway17
commented
1 year ago @kbecciv Can you share your device info here? Looks like this is specific issue for certain device.
esh-g
commented
1 year ago Yes, I'm trying to replicate and it is smooth on android API 33. Seems like an issue with a specific device
kbecciv
commented
1 year ago @railway17 Device info
aitoraznar
commented
1 year ago My proposal is to not hide the keyboard when pressing "Continue" button, that way we avoid the keyboard flickering. I found the line in which the keyboard is forced to be hidden. Is the value passed to blurOnSubmit that closes the keyboard, so we need to adjust the conditional.
I also tested that the magic link and forgot password pages are shown. Also it works better in iOS.
index 574e96e6ca..1cf2472b49 100644
--- a/src/pages/signin/SignInPage.js
+++ b/src/pages/signin/SignInPage.js
@@ -68,8 +68,7 @@ class SignInPage extends Component {
// Show the resend validation link form if
// - A login has been entered
// - AND is not validated or password is forgotten
- const shouldShowResendValidationLinkForm = this.props.credentials.login
- && (!this.props.account.validated || this.props.account.forgotPassword);
+ const shouldShowResendValidationLinkForm = showLoginForm === false && showPasswordForm === false;
const welcomeText = shouldShowResendValidationLinkForm
? ''```I could reproduce this issue in smaller devices as I mentioned before: The smaller, the easier reproducible like below video. https://www.loom.com/share/a55a68a30c7d4bd6b6813c2a703735f9. (My emulators are Galaxy Nexus 4.65", Pixel 3a 5.6" (similar to @kbecciv 's testing device size), Pixel 4)
It is why Keyboard is hidden when submitting the LoginForm by blurOnSubmit (like @aitoraznar mentioned above).
Actually, I don't think we should change the shouldShowResendValidationLinkForm like @aitoraznar suggested because this variable is also used for shouldShowWelcomeText and ResendValidationForm.
We can just remove blueOnSubmit prop from LoginForm like below:
so that password managers can access the values. Conditionally rendering these components will break this feature. */}
- <LoginForm isVisible={showLoginForm} blurOnSubmit={shouldShowResendValidationLinkForm} />
+ <LoginForm isVisible={showLoginForm}
+ // blurOnSubmit={shouldShowResendValidationLinkForm}
+ />
<PasswordForm isVisible={showPasswordForm} />
{shouldShowResendValidationLinkForm && <ResendValidationForm />}I checked also the blurOnSubmit in LoginForm. It is only used to force email input blur, but focus will be moved to password input when PasswordForm is loaded by the code that is written in componentDidMount of this component.
Result will like below:
https://www.loom.com/share/c59ed59611e941e59aa74ea10d62e71b
huzaifa-99
commented
1 year ago We are always hiding the keyboard when going from LoginForm to PasswordForm screen. The reason why we are hiding keyboard has roots in this PR https://github.com/Expensify/App/pull/7604.
In #7604 we added a change to hide keyboard when going from LoginForm to ResetValidationForm screen (creating a new account) which is a nice thing, but this change also made the keyboard hidden when going from LoginForm to PasswordForm screen.
So we only need to hide the keyboard when going from LoginForm to ResetValidationForm screen (when a new account is being created). To do that we can make these changes in SignInPage.js
// Show the resend validation link form if
// - A login has been entered
@@ -83,7 +87,7 @@ class SignInPage extends Component {
>
{/* LoginForm and PasswordForm must use the isVisible prop. This keeps them mounted, but visually hidden
so that password managers can access the values. Conditionally rendering these components will break this feature. */}
- <LoginForm isVisible={showLoginForm} blurOnSubmit={shouldShowResendValidationLinkForm} />
+ <LoginForm isVisible={showLoginForm} blurOnSubmit={this.props.account.validated === false} />
<PasswordForm isVisible={showPasswordForm} />
{shouldShowResendValidationLinkForm && <ResendValidationForm />}
</SignInPageLayout>The this.props.account.validated is false for a new account. The changes above will work for both redirections Loginform -> PasswordForm and LoginForm -> ResendValidationForm
Thoughts @rushatgabhane @laurenreidexpensify
thesahindia
commented
1 year ago I think we should close this issue because we are going #passwordless
thesahindia
commented
1 year ago @huzaifa-99, you don't need to tag people in your proposal. The assigned C+ will review your proposal.
rushatgabhane
commented
1 year ago I think we should close this issue because we are going #passwordless
@thesahindia that's true. But the fix is really easy and affects a core flow of the app.
@JmillsExpensify do you think we should close this issue?
JmillsExpensify
commented
1 year ago I don't think this form is affected by passwordless, because you still need to enter your email address to sign-up for the app. So it's going to still exist. I think this is a pretty cosmetic bug but worth fixing if the corresponding solution is also straightforward.
huzaifa-99
commented
1 year ago I think we should close this issue because we are going #passwordless
@thesahindia that's true. But the fix is really easy and affects a core flow of the app.
@JmillsExpensify do you think we should close this issue?
@rushatgabhane I am not sure if my proposal is accepted? (as you linked it in the comment)
rushatgabhane
commented
1 year ago @huzaifa-99 sorry, no proposal is accepted yet. All seem similar so im trying to find which one fits best to our criteria of "first best unique" proposal
rushatgabhane
commented
1 year ago @pecanoro I like @huzaifa-99's proposal. 🎀 👀 🎀 C+ reviewed
Proposal 1 changes shouldShowResendValidationLinkForm which might have side effects / is unnecessary.
Proposal 2 removes blurOnSubmit, which isn't a good idea because the keyboard won't be dismissed when the magic link page is opened.
Thanks for your proposals all
rushatgabhane
commented
1 year ago updated https://github.com/Expensify/App/issues/13044#issuecomment-1332918427 to recommend a proposal
aitoraznar
commented
1 year ago Hi @rushatgabhane.
The main goal of my work is always to fix the issue taking into account small improvement we can apply on the way. The selected proposal would work but it is not the best for the forahead of the project.
My proposal not only fixes this issue but also makes the code more clear than before. It makes more clear the case of when each screen should be shown and the blurOnSubmit is using a meaningful variable that is used in other parts of the code.
Cheers.
pecanoro
commented
1 year ago @rushatgabhane I agree with you, it's the best proposal IMO. Though @huzaifa-99 instead of this.props.account.validated === false, you can simply do !this.props.account.validated.
melvin-bot[bot]
commented
1 year ago 📣 @huzaifa-99 You have been assigned to this job by @pecanoro! Please apply to this job in Upwork 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 📖
huzaifa-99
commented
1 year ago Hi @pecanoro. Hm, !this.props.account.validated was also passing when validated was undefined so just to be sure it is a non validated account (new account) I added an equality check for false
huzaifa-99
commented
1 year ago @pecanoro applied on Upwork, PR will be up soon
pecanoro
commented
1 year ago Oh I see we don't make that prop required so it could potentially be undefined. Then yes, go with your solution!
huzaifa-99
commented
1 year ago @rushatgabhane I was just finishing up the screenshots/videos section of the PR template and realized that we may also want to keep the virtual keyboard open when going from LoginForm to PasswordForm screen on mobile web as well, is this the case?
If so we would also need some other changes only for mobile web.
diff --git a/src/components/TextInput/baseTextInputPropTypes.js b/src/components/TextInput/baseTextInputPropTypes.js
index b18c1fe6c..ce6f06623 100644
--- a/src/components/TextInput/baseTextInputPropTypes.js
+++ b/src/components/TextInput/baseTextInputPropTypes.js
@@ -70,6 +70,9 @@ const propTypes = {
/** Whether we should wait before focusing the TextInput, useful when using transitions */
shouldDelayFocus: PropTypes.bool,
+
+ /** Callback to run when TextInput loses focus */
+ onBlur: PropTypes.func,
};
const defaultProps = {
@@ -101,6 +104,7 @@ const defaultProps = {
prefixCharacter: '',
onInputChange: () => {},
shouldDelayFocus: false,
+ onBlur: () => {},
};
export {propTypes, defaultProps};
diff --git a/src/pages/signin/LoginForm.js b/src/pages/signin/LoginForm.js
index e9813c3bf..687627c0c 100755
--- a/src/pages/signin/LoginForm.js
+++ b/src/pages/signin/LoginForm.js
@@ -69,6 +69,7 @@ class LoginForm extends React.Component {
super(props);
this.onTextInput = this.onTextInput.bind(this);
this.validateAndSubmitForm = this.validateAndSubmitForm.bind(this);
+ this.focusTarget = null;
this.state = {
formError: false,
@@ -123,7 +124,11 @@ class LoginForm extends React.Component {
/**
* Check that all the form fields are valid, then trigger the submit callback
*/
- validateAndSubmitForm() {
+ validateAndSubmitForm(event) {
+ if(event.currentTarget === this.focusTarget && !this.props.blurOnSubmit){
+ this.input.focus();
+ }
+
if (this.props.network.isOffline) {
return;
}
@@ -178,6 +183,13 @@ class LoginForm extends React.Component {
autoCapitalize="none"
autoCorrect={false}
keyboardType={CONST.KEYBOARD_TYPE.EMAIL_ADDRESS}
+ onBlur={(e) => {
+ if(e.relatedTarget){
+ this.focusTarget = e.relatedTarget
+ } else {
+ this.focusTarget = null
+ }
+ }}
/>
</View>
{!_.isEmpty(this.props.account.success) && (The changes mentioned in my proposal here solve the regression from #7604.
But to keep the keyboard open when transitioning on mobile web as well, the simplest way I think is to focus the input element again after it loses focus (and submit button gets focused). This will be done when the order of operations is
Click on Input -> Type Email -> Click on Submit -> we then focus on input and it keeps the keyboard openApologies as I did not test before for mobile web as from the title I thought its only related to android. Please let me know if these changes are fine/needed. Thanks.
rushatgabhane
commented
1 year ago @huzaifa-99 thanks and don't worry about it.
I noticed it too, but it's unrelated to this issue and there are multiple ways to solve it. Ideally, the keyboard shouldn't close in the first place
Please report the bug on slack and post a proposal for the same
huzaifa-99
commented
1 year ago PR is up for review here
huzaifa-99
commented
1 year ago @huzaifa-99 thanks and don't worry about it.
I noticed it too, but it's unrelated to this issue and there are multiple ways to solve it. Ideally, the keyboard shouldn't close in the first place
Please report the bug on slack and post a proposal for the same
@rushatgabhane Thanks, bug reported here
laurenreidexpensify
commented
1 year ago Offers sent in Upwork to @rushatgabhane @huzaifa-99
laurenreidexpensify
commented
1 year ago still in review
melvin-bot[bot]
commented
1 year ago @pecanoro, @rushatgabhane, @laurenreidexpensify, @huzaifa-99 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
pecanoro
commented
1 year ago PR was merged, not overdue!
melvin-bot[bot]
commented
1 year ago The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.38-6 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 2022-12-20. :confetti_ball:
After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
melvin-bot[bot]
commented
1 year 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:
rushatgabhane
commented
1 year ago PR that introduced the bug: https://github.com/Expensify/App/pull/7604
Commented on PR - https://github.com/Expensify/App/pull/7604#issuecomment-1355031662
I'm assuming no discussion is needed
pecanoro
commented
1 year ago I would also say no discussion is needed, more than a bug, it was a small glitch since no functionality was broken.
laurenreidexpensify
commented
1 year ago Paid. Will start a regression test convo shortly.
laurenreidexpensify
commented
1 year ago Regression testing is already covered here
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:
There should be no jerks in the transition
Actual Result:
The transition after enter email and click "Continue" occurs with a jerk
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.31.8
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/93399543/204021611-88c055f7-10cf-4f1c-8bb1-d7551305f5d9.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit