[HOLD for payment 2023-05-23] [$2000] Android - Sign in - The keyboard opens and disappears for a brief moment

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when executing PR #15731

Action Performed:

. Open the App
2. Navigate to the login page
3. Press the Create a new account link in the footer
4. Verify that you are navigated to the login page
5. Enter an email address and press Continue
6. Scroll down to the footer and press Log in
7. Verify that you are navigated back to the login page

Expected Result:

The keyboard doesn't open

Actual Result:

The keyboard opens and disappears for a brief moment

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.88.0

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

Bug5987014_Record_2023-03-21-21-26-21_4f9154176b47c00da84e32064abf1c48.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f770760fee7155f3
• Upwork Job ID: 1640402122644557824
• Last Price Increase: 2023-04-10

[MelvinBot]

Triggered auto assignment to @tjferriss (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01f770760fee7155f3

[MelvinBot]

Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new.

[tjferriss]

Reproduced via BrowserStack

[MelvinBot]

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

[MelvinBot]

Triggered auto assignment to @roryabraham (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[bernhardoj]

Proposal

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

Keyboard is shown for a while when we go to login page from footer link while we are at password/2fa form.

What is the root cause of that problem?

Currently, when we go to the password form, the login form is not unmounted, but just hidden.

App/src/pages/signin/SignInPage.js

Lines 111 to 113 in 10d3d52

	{/* 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={this.props.account.validated === false} />

The login form will only be shown if this condition meets

App/src/pages/signin/SignInPage.js

Lines 56 to 59 in 10d3d52

	// Show the login form if
	// - A login has not been entered yet
	// - AND a validateCode has not been cached with sign in link
	const showLoginForm = !this.props.credentials.login && !this.props.credentials.validateCode;

When we press the Log in footer link, it will call this navigateHome. It does 2 thing, navigate and clearSignInData,

App/src/pages/signin/SignInPageLayout/Footer.js

Lines 24 to 29 in 10d3d52

	const navigateHome = () => {
	Navigation.navigate();
	// We need to clear sign in data in case the user is already in the ValidateCodeForm or PasswordForm pages
	Session.clearSignInData();
	};

Even though we navigate to the same page, the page will remount. However, if we only do the navigate, the password form will still be shown. That's why we need the clearSignInData.

Now, inside LoginForm, we auto focus the input every time the form becomes visible.

App/src/pages/signin/LoginForm.js

Lines 93 to 96 in 10d3d52

	if (prevProps.isVisible || !this.props.isVisible) {
	return;
	}
	this.input.focus();

So, while we are navigating with navigateHome, the LoginForm will be updated with the new value of isVisible is true (because of clearSignInData) and call the focus, thus the keyboard shows. After the navigation is done, the focus is lost. This makes the keyboard shows for a moment.

Update:
Why it happens on Android only? I guess it depends on how the platform handle when we focus a text input than immediately navigate away (blur the input). Here is a snack to reproduce what I mean: https://snack.expo.dev/_volX4DpL

The focus delay (300ms) is for android to always receive the focus, the same thing we do in our app. To test it, just open the details page. On Android, you will notice the keyboard will shows for a while, but on iOS, the keyboard never shows. So, iOS smart enough to know that it's useless to show a keyboard.

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

The solution is to prevent the focus to happen as we are leaving the page, going to the same page. To prevent it, we can check if the current screen/page is focused or not.

1. Wrap LoginForm with withNavigationFocus
2. Check inside the componentDidUpdate, if the screen is not focused anymore, return early.

App/src/pages/signin/LoginForm.js

Lines 93 to 96 in 10d3d52

	if (prevProps.isVisible || !this.props.isVisible) {
	return;
	}
	this.input.focus();

[MelvinBot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[Santhosh-Sellavel]

cc: @luacmartins do we need to treat this as regression?

[luacmartins]

I don’t think this is a regression since the previous behavior opened web instead of linking within the App. However, this is indeed something that we should fix.

[Santhosh-Sellavel]

Thanks @luacmartins

[Santhosh-Sellavel]

@bernhardoj I'm not sure why this happens on android alone root cause doesn't explain this clearly

[bernhardoj]

Added explanation to it on my proposal.

[Santhosh-Sellavel]

Sorry for the delay. @roryabraham Your thoughts on the proposal?

[hellohublot]

Yes, just waiting for Expensify Reviewer to merge #17445

[roryabraham]

Merged!

[melvin-bot]

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

[melvin-bot]

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

• Fix keyboard flicker and input overlap in SignInPage #17445

If no regressions arise, payment will be issued on 2023-05-23. 🎊

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.

• External issue reporter Applause
• Contributor that fixed the issue @hellohublot
• Contributor+ that helped on the issue and/or PR @Santhosh-Sellavel

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

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

[tjferriss]

@Santhosh-Sellavel @hellohublot can you please both apply to the job here: https://www.upwork.com/jobs/~01060f20aaf9a83d49?

There is a 50% penalty for this issue:

[Santhosh-Sellavel]

@tjferriss there is no unnecessary delay here, this issue is on hold for atleast couple of weeks due to other issue and For CME @roryabraham busy with priority internal items. You can check the PR for history.

So penalty should not applicable here, thanks!

[roryabraham]

Looking at the history here:

• @Santhosh-Sellavel asked to put the PR on hold April 21st
• The PR it was on HOLD for was eventually just closed on May 3rd.

So that's 8 business days when the PR was on HOLD. Then @Santhosh-Sellavel final-approved the PR on May 7th, and I merged it on May 11th, so that's an additional 4 business days it was delayed outside of the contributors' control here.

So given that there were 22 business days elapsed between assignment and merge, and 12 of those days were caused by factors outside the contributors' control. That still leaves 10 business days between assignment and merge (after correction).

According to our contributing guidelines:

Merged PR within 9 business days - 50% penalty

So by my reckoning the 50% penalty should still apply here.

[Santhosh-Sellavel]

@roryabraham

I agree it took around 10 days. I am requesting you to waive off here as this was on hold. The hold is the problem where I lost rack of time. Post Off hold I spent 4 days reviewing and testing the PR. That's the average review time (3 - 4 days) for any PR. I had to ensure this does not cause any regressions. Thanks!

cc: @tjferriss

[hellohublot]

@tjferriss
Thanks, I already submitted a proposal on Upwork, but I guess you can't find it (I've encountered it before), so you can send me an offer directly on payment day, thanks again

@roryabraham
Thank you for your feedback. After May 3 and May 4 Hold status was cancelled, since the main code has changed a lot, I and the C+ reviewer need to modify all the test steps again, take screenshots and test all the modifications again, the workload of these modifications is already equivalent to another complete PR

In fact, this is the first time I got -50% penalty, but every time Expensify reviewer and C+ reviewer ask questions, every time I reply all messages within 1 day, yes I always push progress within 1 day , and this issue also consumed me a lot of time and energy, but still got -50% penalty

[roryabraham]

since the main code has changed a lot, I and the C+ reviewer need to modify all the test steps again

I think this is a fair point – the delay added additional work due to merge conflicts. Given that you only missed the mark by 1 day, it's reasonable to assume that you would've completed it in time without the additional work caused by the delay. You were also responsive throughout, and demonstrated a good amount of urgency to push the PR forward.

So given the specific context here, I'm willing to make an exception and void the 50% penalty because it was more my fault than yours.

So, @tjferriss if you agree, that means that we should pay the full bounty amount here.

[tjferriss]

Sorry for the delay as I've been out of office.

I agree with paying the full bounty. Thanks for handling the discussion @roryabraham.

Thanks, I already submitted a proposal on Upwork, but I guess you can't find it (I've encountered it before), so you can send me an offer directly on payment day, thanks again

@hellohublot I don't fully follow your comment. Are you not able to apply to the job posting here: https://www.upwork.com/jobs/~01060f20aaf9a83d49?

Similarly, @Santhosh-Sellavel can you please apply to the job posting?

[Santhosh-Sellavel]

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:

• [@Santhosh-Sellavel] The PR that introduced the bug has been identified. Link to the PR: Fix login page urls #15731
• [@Santhosh-Sellavel] 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: Fix login page urls #15731 (comment)
• [@Santhosh-Sellavel] 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
• [@Santhosh-Sellavel] Determine if we should create a regression test for this bug. Yes just to ensure
• [@Santhosh-Sellavel] 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. [HOLD for payment 2023-05-23] [$2000] Android - Sign in - The keyboard opens and disappears for a brief moment #16357 (comment)
• [@tjferriss] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/286846

cc: @roryabraham @tjferriss Let me know if you differ on any of the above!

[Santhosh-Sellavel]

Regression Test Steps:

1. Open New Dot
2. Logout if already logged in
3. Enter your phone number or email address and click Continue
4. Scroll to the bottom and click Create A New Account or Log In
5. Verify that the keyboard doesn't flicker and Login Page is open again
6. Repeat Step 3
7. Scroll until you can hide the input (Web and desktop can be skipped)
8. Click Go back
9. Verify the page will automatically scroll to the top, and without covering the input

👍 or 👎

[hellohublot]

@tjferriss Similar to this one, The client always can't see my proposal on upwork, so you can send me the offer directly, https://www.upwork.com/freelancers/~01509ca2c74bfc7073 Thanks.

[tjferriss]

Oh duh, I just had to click "Load More". Offers have been sent to both of you. Once they're accepted, I'll process the payment.

[tjferriss]

@hellohublot @Santhosh-Sellavel payments have been made.