Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove didMount to stop auto focus on PasswordForm #7090

Merged
merged 4 commits into from
Jan 13, 2022

Conversation

Enragedsaturday
Copy link
Contributor

@Enragedsaturday Enragedsaturday commented Jan 7, 2022

Details

Hidden password element was pulling focus, and accepting any text input prior to focusing on login form (email). Added a check to see if the component is visible before pulling focus on mount, added same to the login form as per maintainer request on main issue.

Fixed Issues

$ #7199

$ #7133

$ #7058

$ #7090

Tests

Tested on Web, iOS, and Android

  1. Make sure app is on sign on page with the "phone or email" input and not password input.
  2. Check that on initial load focus is on login text input. If not type characters on keyboard. (If it is pull focus and type in characters on keyboard)
  3. Submit and the password component will display.
  4. Ensure the password component does not have the characters you typed in on the previous screen when focus was not on the login component.
  • [ ✓] Verify that no errors appear in the JS console

QA Steps

  1. Make sure app is on sign on page with the "phone or email" input and not password input.
  2. Check that on initial load focus is on login text input. If not type characters on keyboard. (If it is pull focus and type in characters on keyboard)
  3. Submit and the password component will display.
  4. Ensure the password component does not have the characters you typed in on the previous screen when focus was not on the login component.
  • [ ✓] Verify that no errors appear in the JS console

Tested On

  • [ ✓] Web
  • [ ✓] Mobile Web
  • [✓ ] Desktop
  • [ ✓] iOS
  • [ ✓] Android

Screenshots

Web

New-Expensify.mp4

Mobile Web

New-Expensify.1.mp4

iOS

Simulator.Screen.Recording.-.iPhone.13.-.2022-01-07.at.16.48.49.mp4

Android

androidExpensify.mp4

Desktop

Screen.Recording.2022-01-07.at.5.41.24.PM.mov

@Enragedsaturday Enragedsaturday requested a review from a team as a code owner January 7, 2022 22:45
@MelvinBot MelvinBot requested review from marcochavezf and rushatgabhane and removed request for a team January 7, 2022 22:45
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Enragedsaturday
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 7, 2022

@marcochavezf can you please approve the gh actions workflow? thanks!

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Suggested some changes.

@@ -55,7 +55,7 @@ class PasswordForm extends React.Component {
}

componentDidMount() {
if (!this.input) {
if (!this.input || !this.props.isVisible) {
Copy link
Member

@rushatgabhane rushatgabhane Jan 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to check !canFocusInputOnScreenFocus() over here?
Not doing that causes this jarring keyboard bug when you launch the app in the password/login screen.

screen-20220109-004443_2.mp4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, you had asked about this in the issue.

The !canFocusInputOnScreenFocus() is only present on the login form and prevents the autofocus on touch screen devices. It seems counterintuitive to the current solution.

I still think we should keep it in both screens. Because once you open the keyboard on login screen, the keyboard stays open on password screen too. And it avoids the keyboard bug.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rushatgabhane it was never present on the password form but I agree, it should be. Not to make this more complex then it needs to be but the whole intent of the didMount function is autofocus, and it is now being excluded from the most common use cases (all of mobile). That bug is only present on android, and could be addressed on android with platform specific code. I'll edit the current pr and wait for your feedback.

Copy link
Member

@rushatgabhane rushatgabhane Jan 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Then the question is do we want to autofocus on login page for mobile? Because the current user flow is good imo.

If yes, we'll create a new issue for it.

I'm gonna start a discussion on it on slack. Feel free to join the community if you haven't already and give your input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally agreed. I sent two emails to join slack this past week and haven't gotten an invite yet.

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcochavezf PR is ready for review 😃 Tests well on all platforms.

@marcochavezf
Copy link
Contributor

Thank you @Enragedsaturday for creating this PR and thanks @rushatgabhane for reviewing and testing! 😀 The PR also looks good to me, I'm going to merge it once the unit tests are fixed https://github.com/Expensify/Expensify/issues/191675.

@Enragedsaturday
Copy link
Contributor Author

@marcochavezf Thanks. I'm unaware of how to fix the failing test "Decrypt OSbotify GPG Key". I've requested an invite to Slack twice now but haven't gotten an invite sent yet.

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 12, 2022

@Enragedsaturday merge main into this branch, that should fix OSBotify issue.

I'll ask around about the Slack invite, apologies!

@rushatgabhane
Copy link
Member

thanks @rushatgabhane for reviewing and testing!

Cheers! 😃

@rushatgabhane
Copy link
Member

@Enragedsaturday can you please email [email protected] again and include your Upwork profile?
That should get you the invite.

@marcochavezf marcochavezf merged commit 13b7b0b into Expensify:main Jan 13, 2022
@OSBotify
Copy link
Contributor

@Enragedsaturday, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

  1. Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
  2. Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
  3. Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcochavezf in version: 1.1.29-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@kbecciv
Copy link

kbecciv commented Jan 16, 2022

@Enragedsaturday The issue #7199 still reproduced during the Regression.

@Enragedsaturday
Copy link
Contributor Author

Enragedsaturday commented Jan 16, 2022

Not sure what happened there. Only fixed issue #7090.

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 16, 2022

@kbecciv not a regression imho. If I'm not wrong, #7199 was linked to this PR after it was merged.

We didn't test for it or attempt to fix it in this PR 😃

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 16, 2022

@Enragedsaturday can you please undo changes to fixed issues? This PR might fix multiple issues and it needs to be there until that's verified.
Screenshot_20220116-055957

@Enragedsaturday
Copy link
Contributor Author

@rushatgabhane

My bad. Fixed it.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.30-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants