[HOLD for payment 2023-09-06] [$1000] Android - Sign In - Get started below page is cut off if switch tab than come back to app & start typing

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open the login page
2. Tap on the email field to open the soft keyboard
3. Scroll down and click on any link (Expense report in my case)
4. Switch back to the login page

Expected Result:

When start typing I should see the complete login page without any cut offs

Actual Result:

When start typing after Step 4 the login page is not scrolled to the top - "Get started below" page is cut off

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.3.46-0
Reproducible in staging?: y
Reproducible in production?: y
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

Bug6142828_az_recorder_20230726_223014.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/~010bbebff5900a82ee
• Upwork Job ID: 1686501797532385280
• Last Price Increase: 2023-08-01
• Automatic offers:
• mollfpr | Reviewer | 25998264
• joh42 | Contributor | 25998265

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

Proposal

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

Android - Sign In - Get started below page is cut off if switch tab than come back to app & start typing

What is the root cause of that problem?

we don't have the logic to scroll top if user start typing. On mweb platform, the input will be shown when users start typing but the whole get started below page is not shown

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

• In SignInPageLayout, we have scrollPageToTop function, so we can use it in SignInPage by changing props.children to {props.children(scrollPageToTop)}

Change

App/src/pages/signin/SignInPage.js

Lines 147 to 155 in f8bc305

	{/* LoginForm must use the isVisible prop. This keeps it mounted, but visually hidden
	so that password managers can access the values. Conditionally rendering this component will break this feature. */}
	<LoginForm
	isVisible={shouldShowLoginForm}
	blurOnSubmit={account.validated === false}
	/>
	{shouldShowValidateCodeForm && <ValidateCodeForm />}
	{shouldShowUnlinkLoginForm && <UnlinkLoginForm />}
	{shouldShowEmailDeliveryFailurePage && <EmailDeliveryFailurePage />}

to

(scrollToTop) => (
<>
    <LoginForm
        scrollPageToTop = {scrollPageToTop}
    />
...
</>
)

• Finally In LoginForm we call scrollPageToTop in onTextInput function.

In order to prevent calling scrollPageToTop too many times we can check the scrollTop is 0 or not. If scrollTop is 0 we don't need to call scrollPageToTop

What alternative solutions did you explore? (Optional)

We can use useImperativeHandle to allow SignInPage to use scrollPageToTop from SignInPageLayout

[melvin-bot]

@tjferriss Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010bbebff5900a82ee

[melvin-bot]

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

[tjferriss]

Reproducible.

[melvin-bot]

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

[daanish-shaikh]

Proposal

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

Keyboard is shown but textinput not visible, while switching back to the app from another app.

What is the root cause of that problem?

While switching between apps, when user switches back to the app then the textinput gets focused and keyboard is shown but the screen is positioned to where it was before leaving the app. Ideally if the keyboard is shown user should be scrolled to top of the screen.

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

Add a listener for keyboard to check when it is shown and according to that we invoke scrollPageToTop() to make the textinput field visible while typing.

Add listener for keyboard in src/pages/signin/SignInPageLayout/index.js

import {View, ScrollView, Keyboard} from 'react-native';

    useEffect(() => {
        this.keyboardDidShowListener = Keyboard.addListener('keyboardDidShow', _keyboardDidShow);
        return () => {
            this.keyboardDidShowListener.remove();
        };
    }, []);

    const _keyboardDidShow = () => {
        scrollPageToTop();
    };

[mollfpr]

@tienifr

In SignInPageLayout, we have scrollPageToTop function, so we can use it in SignInPage by changing props.children to {props.children(scrollPageToTop)}

I try your solution, but doing the above suggestion makes the LoginForm disappear.

In order to prevent calling scrollPageToTop too many times we can check the scrollTop is 0 or not. If scrollTop is 0 we don't need to call scrollPageToTop

If I scroll down the page slightly and then start typing, will that make the page jump up?

---

I think we're looking at similar behavior from the mWeb.

mWeb/Safari	mWeb/Chrome
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-04.at.21.13.15.mp4	Screen_Recording_20230804_211406_Chrome.mp4

[joh42]

Proposal

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

The problem is that when we reopen the app on the SignInPage after clicking on a link, the app does not scroll back to the top like on mWeb.

What is the root cause of that problem?

We do not listen for when the app becomes active again. Rather, we use default scrolling whenever we start typing, which means that only the input field is scrolled into view when we type.

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

If we want to replicate the behavior on mWeb, I suggest we use the AppStateMonitor.addBecameActiveListener to listen for when the app becomes active. If we are not at the top when the user goes back to the app on the SignInPage, we scroll to the top. Also, it's worth noting that on mWeb there is no scrolling animation, rather we're immediately sent to the top.

Link to the AppStateMonitor: https://github.com/Expensify/App/blob/6730662b0c89c0f32045e57273ff39e0d78661a7/src/libs/AppStateMonitor/index.js

Edit as of 2023-08-04 roughly 09:00 PM UTC

To be clear, at a high level I am proposing that we listen to the AppState, and when the AppState is set to active, we make sure the screen is at the top so that both the email input field and everything above is visible.

This may be done by listening to the AppState directly, using the AppStateMonitor linked above, or perhaps using the Visibility lib which seems to have similar functionality.

Regarding implementation, it may or may not include splitting the LoginForm into a base file and a platform-specific file (android or native, depending on whether or not this issue is present on ios).

I will provide specific implementation details if we do decide to implement this.

[mollfpr]

Thanks @joh42 for the proposal. I confirm that the issue is also happening on iOS, although we might want the page to go to the top only when the input is focused. Is it doable?

[joh42]

Thanks for your reply @mollfpr. That makes sense, it might be confusing to users if the page scrolls even if the input field is not focused. I just threw together a quick test, and yes we can check if the input is focused when the app is opened:

Screen.Recording.2023-08-05.at.22.39.49.mov

Screen.Recording.2023-08-05.at.22.40.13.mov

[mollfpr]

Looks great, thanks @joh42

Let's go with @joh42 proposal. It will scroll up when the input focus and the app is active.

🎀 👀 🎀 C+ reviewed!

[melvin-bot]

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

[daanish-shaikh]

Proposal

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

Keyboard is shown but textinput not visible, while switching back to the app from another app.

What is the root cause of that problem?

While switching between apps, when user switches back to the app then the textinput gets focused and keyboard is shown but the screen is positioned to where it was before leaving the app. Ideally if the keyboard is shown user should be scrolled to top of the screen.

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

Add a listener for keyboard to check when it is shown and according to that we invoke scrollPageToTop() to make the textinput field visible while typing.

Add listener for keyboard in src/pages/signin/SignInPageLayout/index.js

import {View, ScrollView, Keyboard} from 'react-native';

    useEffect(() => {
        this.keyboardDidShowListener = Keyboard.addListener('keyboardDidShow', _keyboardDidShow);
        return () => {
            this.keyboardDidShowListener.remove();
        };
    }, []);

    const _keyboardDidShow = () => {
        scrollPageToTop();
    };

Hi @mollfpr, There was no comment or feedback on my proposal, was it not reviewed. And I feel I have correctly identified the root cause and solution for this issue before @joh42. Would like to here your feedback.

[joh42]

While I certainly agree that you identified the root cause as well @daanish-shaikh, listening to the keyboard wouldn't actually work (on my phone at least):

XRecorder_07082023_095044.mp4

The keyboard doesn't actually open when I go back to the app, even though the input field still has focus. For context I'm on a Oneplus running Android, and I tried with the default keyboard as well which behaved identically.

[mollfpr]

@daanish-shaikh Sorry, I forget to post the feedback on your proposal. I already tested your proposal, but I feel using the keyboardDidShow is not the correct solution. On Android, the keyboard can be closed while the input is still in focus. As given example above, the input is focused but the keyboard is closed.

[francoisl]

I feel like this was almost not a bug in the first place (the text input is visible when you start typing), but since the behavior is already different on mWeb, let's go ahead and make it consistent on all platforms.

+1 for joh42's solution, let's go with it.

[joh42]

The PR is now ready for review @mollfpr, I apologize for the delays.

The unit test check currently fails as there is a failing unit test on main. I've written this on the PR that I think introduced this, and also reported it as a bug in Slack. The broken unit test has now been fixed.

[joh42]

According to Slack @francoisl is OOO until the 28th. How do we proceed with the PR? @tjferriss @mollfpr

[tjferriss]

I'm working on finding someone to review the PR.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @joh42 got assigned: 2023-08-07 22:48:54 Z
• when the PR got merged: 2023-08-24 18:31:56 UTC
• days elapsed: 12

On to the next one 🚀

[tjferriss]

The PR was merged to staging today.

[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.58-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix scroll on native when going back to the login page  #25590

If no regressions arise, payment will be issued on 2023-09-06. 🎊

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
• Contributor that fixed the issue @joh42 $500
• Contributor+ that helped on the issue and/or PR @mollfpr $500

For reference, here are some details about the assignees on this issue:

• @mollfpr requires payment offer (Reviewer)
• @joh42 requires payment offer (Contributor)

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:

• [@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
• [@mollfpr] 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:
• [@mollfpr] 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:
• [@mollfpr] Determine if we should create a regression test for this bug.
• [@mollfpr] 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]

PR was merged more than 9 days after the assignment so we'll have to apply the 50% penalty.

@mollfpr can you please get started on the checklist when you get a chance?

[mollfpr]

PR was merged more than 9 days after the assignment so we'll have to apply the 50% penalty.

Sorry for this; I got sick the last few weeks 🙏

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR:
[@mollfpr] 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:

No offending PR; this is a feature request to scroll the page on native while the input is focused and the app back active.

[@mollfpr] 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:

The regression step should be enough.

[@mollfpr] Determine if we should create a regression test for this bug.
[@mollfpr] 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.

Tests only for iOS and Android

With focus

1. Go to the login page
2. Tap on the phone/email input field to open the keyboard
3. Scroll down a bit
4. Exit the app (either by clicking on a link or by clicking on the home button)
5. Reopen the app
6. Verify that you are scrolled back to the top of the login page

Without focus

1. Go to the login page.
2. Make sure the phone/email input field is blurred (click outside the input field; the line under it should not be green)
3. Scroll down a bit.
4. Exit the app (either by clicking on a link or by clicking on the home button)
5. Reopen the app
6. Verify that you are not scrolled back to the top of the login page

[tjferriss]

We're all set for payments to go out tomorrow.

[melvin-bot]

@francoisl, @tjferriss, @luacmartins, @mollfpr, @joh42 Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@francoisl, @tjferriss, @luacmartins, @mollfpr, @joh42 Huh... This is 4 days overdue. Who can take care of this?

[luacmartins]

@tjferriss are we done with payments?

[tjferriss]

We are now. Regression test issue has also been created. So we're all set here.