Fix the keyboard overlapping buttons on the sign in form #12848

tgolen · 2022-11-17T23:14:13Z

Details

The page was originally built with using a top margin set to a percentage of the page height. Because of that, when the keyboard opens, the page wasn't properly collapsing the top margin in order to reveal the entire form. I restructured the entire layout to properly use flex techniques to allow the section to collapse as intended.

Fixed Issues

$ #11701

Tests

Be signed out
Have your email address entered so that you see the password form
For native platforms, verify the password form is not covered by the software keyboards
For all other platforms, verify that the form and page layout looks correct (best to compare it to production).

For web specifically:

Test the page at widths less than 800 (form should always be centered and have a max width of 400px. The form should NOT span the entire width of the page for widths between 400 and 800).
Test the page at widths between 801 and 1024 (the form should no longer take up the top half of the page and it should look just like any width greater than 1024)
Test the page at widths greater than 1024 (form should be a fixed width of ~400px on the left side of the page)

Test keyboard dismissing on native (iOS and Android):

On both the enter email and enter password forms, tap into the text input
Tap outside of the input and verify the keyboard is dismissed and verify the input does not have focus (it does not have a blue border)
On the enter email form, enter the email of an existing account and either tap continue or press enter
Verify the keyboard remains shown on the password form and the password text input has focus
On the email form, enter an email address that doesn't have an account yet and press enter (or click continue)
Verify that you are shown the message that a link has been sent to you and verify the keyboard has been dismissed

Verify that no errors appear in the JS console

Offline tests

Go offline
Verify there is an offline indicator under the sign in button
Go online
Verify the offline indicator goes away

QA Steps

Same as the tests above

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot · 2022-11-18T16:50:13Z

@aimane-chnaif @cristipaval One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

rushatgabhane · 2022-11-18T17:33:49Z

@tgolen could i please review this PR because i was assigned to the issue for this PR

rushatgabhane · 2022-11-18T17:42:05Z

@tgolen this might be a dumb question - how to dismiss keyboard on iOS? (i tried drag outside text input)

usecase: See the footer which has ToS and language selector

a.mov

tgolen · 2022-11-18T20:42:38Z

@rushatgabhane Yeah, feel free to review! I've gone and added you.

I have fixed the keyboard dismissing issue (had to add back in the scrollview).

melvin-bot · 2022-11-18T20:42:56Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

aimane-chnaif · 2022-11-18T20:43:15Z

Shall I review this PR as I am assigned?

tgolen · 2022-11-18T21:12:22Z

Anyone can review it :D The more the merrier!

cristipaval

Hey @tgolen ! I tested your branch and looks like the submit button is still partially covered by the keyboard.

Screen.Recording.2022-11-21.at.17.19.51.mov

This is NexusOne with Android 10

rushatgabhane · 2022-11-21T15:37:12Z

@cristipaval could you please re-upload the recording, thanks!

cristipaval · 2022-11-21T16:08:04Z

@cristipaval could you please re-upload the recording, thanks!

should work now

tgolen · 2022-11-21T16:14:57Z

Thanks @cristipaval! I think that will be OK. I don't think that's a supported device or android version? Is that what you normally test with?

cristipaval · 2022-11-21T16:30:59Z

This is an emulator on which I usually test to see how the App behaves on smaller screens. The Android 10 it's not old I'd say, so we should support it.

I do agree that the behaviour is still fine, even if the button is completely covered by the keyboard, as long as we have the submit action on the keyboard working as a physical tap on the submit button.

tgolen · 2022-11-21T17:16:25Z

I checked with QA and they are only required to test on Android 11 and above, so I think what you're seeing there is OK.

cristipaval · 2022-11-21T17:43:15Z

Yes it works better on newer Android versions.

Looks like howsVerticalScrollIndicator={false} does not work on my Pixel 5 emulator with Android 12.

Screen.Recording.2022-11-21.at.19.35.57.mov

aimane-chnaif · 2022-11-21T17:44:27Z

signin.mp4

In video, 00:00-00:06 is before fix, 00:07-00:13 is after fix
@tgolen 3 issues found here (tested on Samsung Galaxy S10, Android 12 - popular device)

scroll indicator position - this can be moved to edge
screen is a bit pushed to top compared to old version
design is different from old one, i.e. horizontal paddings
Totally, I found no flaws with old native android app but while you're fixing it to be responsive to mChrome, I feel screen looks worse than before.

tgolen · 2022-11-21T17:51:47Z

OK, thanks for the testing! I'll work on those today and tomorrow and see what I come up with.

tgolen · 2022-11-21T17:55:01Z

Oh, ha... typo in the prop for howsVerticalScrollIndicator 🤦

src/pages/signin/SignInPageLayout/SignInPageContent.js

tgolen · 2022-11-22T17:04:18Z

Branch updated! I believe I got everything addressed:

Keyboard dismisses when tapping outside of inputs (and stays open when transitioning between the email and password form)
Fixed the visible scrollbar (by removing the scroll view entirely)
Fixed the padding to match production
Updated tests to try and cover all the cases that need to be accounted for

jasperhuangg

just noticed some typos, overall looks good

jasperhuangg · 2022-11-22T17:16:22Z

src/pages/signin/SignInPageLayout/SignInPageContent.js

-                <SignInPageForm style={[
+const SignInPageContent = (props) => {
+    const dismissKeyboardWhenTappedOutsideOfInput = () => {
+        // This prop comes from with


Suggested change

// This prop comes from with

// This prop comes from withKeyboardState

jasperhuangg · 2022-11-22T17:16:43Z

src/pages/signin/SignInPageLayout/SignInPageContent.js

-                    props.isSmallScreenWidth ? styles.ph5 : styles.ph4,
+                    styles.signInPageLeftContainer,
+
+                    // Restrict the width if the left container only for large screens. For smaller screens, the width needs to be fluid to span the entire width of the page.


Suggested change

// Restrict the width if the left container only for large screens. For smaller screens, the width needs to be fluid to span the entire width of the page.

// Restrict the width of the left container only for large screens. For smaller screens, the width needs to be fluid to span the entire width of the page.

jasperhuangg · 2022-11-22T17:17:26Z

src/pages/signin/SignInPageLayout/SignInPageContent.js

+                    styles.signInPageLeftContainer,
+
+                    // Restrict the width if the left container only for large screens. For smaller screens, the width needs to be fluid to span the entire width of the page.
+                    !props.isMediumScreenWidth && !props.isSmallScreenWidth && styles.signInPageWideLeftContainer,


NAB: Wondering why we don't have a isLargeScreenWidth prop for times like these

Actually, that's a great point. I want to refactor it a little to have this.

tgolen · 2022-12-01T01:24:28Z

Ack, I just realized I didn't push up the code for that last change. Anyway, the code is there now 😅

aimane-chnaif · 2022-12-01T11:13:07Z

src/styles/styles.js

+    signInPageLeftContainerMediumScreen: {
+        maxWidth: 400,
+        marginLeft: 'auto',
+        marginRight: 'auto',


+ marginLeft: 'auto', + marginRight: 'auto',

What is this change for?
Because of this, horizontal padding is not consistent between email page and password page

tested on iPhone SE
you can see button width are different

Ugh, this stuff is so finicky.

The reason I added the auto-margins is:

Shawn requested to have the form use a max-width

Setting a maxWidth works for the form, but for medium-sized screens, the form is aligned on the left side of the container and looks bad. The form needs to be horizontally centered in the container.

My first thought was to use alignSelfCenter, and that works. BUT at small screen widths, that style also centers the content vertically 😡 and adds a lot of unintended margin to the top and bottom

So I went with the auto margin route, which makes the form horizontally centered AND doesn't mess up any vertical margins

I have fiddled with it again today to remove the margin auto and get the alignSelfCenter working.

I narrowed the problem you saw with iPhone SE down to the props.welcomeText. It seems the difference in text was causing the width of the container to change size... so I have restricted that text to 300px wide always.

Updated!

Could you (or a reviewer) share a quick video of the sign in page going from small to wide? Just want to check and see how the responsive styles are working, thanks!

Here you go:

2022-12-01_12-50-20.mp4

Cool, looks good to me, thanks for making the video!

aimane-chnaif · 2022-12-02T03:44:30Z

@tgolen please pull from latest main and fix conflict once more

tgolen · 2022-12-02T15:51:22Z

Updated to resolve conflicts.

aimane-chnaif · 2022-12-02T15:57:09Z

doing final test now

aimane-chnaif · 2022-12-02T16:22:46Z

The keyboard "glitch" where the keyboard hides and then reshows quickly when going from the username to password form
IGNORED I was not able to solve this. I went so far as to remove every usage of Keyboard.dismiss() in our app, and I couldn't find anything that was causing the keyboard to dismiss. It's possible it's happening as part of our navigation library. It is also happening in main so it's not a regression in this PR and I think it's OK to merge.

NOTE: This is already logged here and will be fixed in this PR

aimane-chnaif · 2022-12-02T16:32:50Z

Looks good to me and tests well on all platforms with various screen sizes (except android chrome on very small device commented here)

jasperhuangg

Code looks great and tests well on all platforms! Unable to replicate the broken small-screen android case, but I imagine that's an edge case so I'm approving. Let's create a follow-up issue for that?

JmillsExpensify · 2022-12-05T13:19:56Z

Quick follow-up up coming from the main issue. This PR looks ready to merge aside from @jasperhuangg's point above. Let's get this one merged today!

tgolen · 2022-12-05T14:49:38Z

Yep, since it's been twice approved, I'll go ahead and self-merge this so we can finally get it done.

OSBotify · 2022-12-05T14:53:59Z

✋ 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 · 2022-12-13T22:41:08Z

🚀 Deployed to production by @chiragsalian in version: 1.2.38-6 🚀

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

chiragsalian · 2022-12-14T23:18:32Z

Just FYI that this PR caused #13488.

eVoloshchak · 2023-02-06T12:54:17Z

Leaving a note that this PR caused #13920

This is some weird quirk with RNPickerSelect where when you enclose them with Touchables it makes them untouchable.

rushatgabhane · 2023-02-06T12:59:06Z

@eVoloshchak could you please explain why it caused it?

eVoloshchak · 2023-02-06T13:17:00Z

@rushatgabhane, sure!
On this line the sign-in page is wrapped with TouchableWithoutFeedback. It seems like there is an issue with RNPickerSelect when you enclose in with touchable on Android. It seems like it was enclosed with touchable to fix the issue specific to iOS

 * On iOS sometimes the keyboard doesn't dismiss itself
 * when you tap outside of the input that pulled it up.
 * Therefore this component is necessary to ensure users
 * can still dismiss the keyboard on iOS.

So it was resolved by using platform-specific files and using touchable only on iOS

tgolen self-assigned this Nov 17, 2022

tgolen mentioned this pull request Nov 17, 2022

Software keyboard covers part of the submit button on login page #11701

Closed

tgolen changed the title ~~Tgolen fix signin keyboard avoiding~~ Fix the keyboard overlapping buttons on the sign in form Nov 18, 2022

tgolen marked this pull request as ready for review November 18, 2022 16:49

tgolen requested a review from a team as a code owner November 18, 2022 16:49

melvin-bot bot requested review from aimane-chnaif and cristipaval and removed request for a team November 18, 2022 16:50

tgolen requested a review from rushatgabhane November 18, 2022 19:02

tgolen mentioned this pull request Nov 21, 2022

[HOLD for payment 2022-12-23] [$2000] [Bug] Keyboard being open/closed changes position of ToS on certain mobile platforms - reported by @gadhiyamanan #11277

Closed

cristipaval reviewed Nov 21, 2022

View reviewed changes

jasperhuangg reviewed Nov 21, 2022

View reviewed changes

src/pages/signin/SignInPageLayout/SignInPageContent.js Outdated Show resolved Hide resolved

jasperhuangg previously approved these changes Nov 22, 2022

View reviewed changes

Keep the form a fixed width on small to medium screens

8ffc298

aimane-chnaif reviewed Dec 1, 2022

View reviewed changes

tgolen added 2 commits December 1, 2022 10:15

Center the form better and fix iphone SE

efd0738

Always keep the form the same width

7208991

tgolen added 2 commits December 2, 2022 08:50

Merge branch 'main' into tgolen-fix-signin-keyboard-avoiding

5c2d8d2

Merge branch 'main' into tgolen-fix-signin-keyboard-avoiding

bbba5e5

aimane-chnaif approved these changes Dec 2, 2022

View reviewed changes

jasperhuangg approved these changes Dec 2, 2022

View reviewed changes

tgolen merged commit 5530ea2 into main Dec 5, 2022

tgolen deleted the tgolen-fix-signin-keyboard-avoiding branch December 5, 2022 14:49

francoisl mentioned this pull request Dec 8, 2022

Deploy Checklist: New Expensify 2022-12-07 #13431

Closed

72 tasks

daraksha-dk mentioned this pull request Dec 10, 2022

[HOLD for payment 2022-12-22][$1000] Workspace - general settings - Save Button is not at the bottom #13488

Closed

jasperhuangg mentioned this pull request Jan 3, 2023

Allow sign in page picker to be touchable everywhere on Android #13954

Merged

52 tasks

0xmiros mentioned this pull request Jan 8, 2023

[$2000] Login page content jumps when the connection status is changed #13876

Closed

1 task

eVoloshchak mentioned this pull request Feb 6, 2023

[HOLD Internationalization Beta] [Android] Sign in - Language picker is not clickable in sign in page #13920

Closed

1 task

parasharrajat mentioned this pull request Apr 15, 2023

[$1000] Extra horizontal spacing (extra 13px on left and right side) on the resend link password page #17177

Closed

6 tasks

	// This prop comes from with
	// This prop comes from withKeyboardState

	// Restrict the width if the left container only for large screens. For smaller screens, the width needs to be fluid to span the entire width of the page.
	// Restrict the width of the left container only for large screens. For smaller screens, the width needs to be fluid to span the entire width of the page.

Fix the keyboard overlapping buttons on the sign in form #12848

Fix the keyboard overlapping buttons on the sign in form #12848

Conversation

tgolen commented Nov 17, 2022 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots/Videos

melvin-bot bot commented Nov 18, 2022

rushatgabhane commented Nov 18, 2022

rushatgabhane commented Nov 18, 2022 • edited Loading

tgolen commented Nov 18, 2022

melvin-bot bot commented Nov 18, 2022

aimane-chnaif commented Nov 18, 2022 • edited Loading

tgolen commented Nov 18, 2022

cristipaval left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane commented Nov 21, 2022

cristipaval commented Nov 21, 2022

tgolen commented Nov 21, 2022

cristipaval commented Nov 21, 2022 • edited Loading

tgolen commented Nov 21, 2022

cristipaval commented Nov 21, 2022

aimane-chnaif commented Nov 21, 2022

tgolen commented Nov 21, 2022

tgolen commented Nov 21, 2022

tgolen commented Nov 22, 2022

jasperhuangg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aimane-chnaif commented Dec 2, 2022

tgolen commented Dec 2, 2022

aimane-chnaif commented Dec 2, 2022

aimane-chnaif commented Dec 2, 2022

aimane-chnaif commented Dec 2, 2022

jasperhuangg left a comment • edited Loading

Choose a reason for hiding this comment

JmillsExpensify commented Dec 5, 2022

tgolen commented Dec 5, 2022

OSBotify commented Dec 5, 2022

OSBotify commented Dec 13, 2022

chiragsalian commented Dec 14, 2022

eVoloshchak commented Feb 6, 2023 • edited Loading

rushatgabhane commented Feb 6, 2023

eVoloshchak commented Feb 6, 2023

tgolen commented Nov 17, 2022 •

edited

Loading

rushatgabhane commented Nov 18, 2022 •

edited

Loading

aimane-chnaif commented Nov 18, 2022 •

edited

Loading

cristipaval left a comment •

edited

Loading

cristipaval commented Nov 21, 2022 •

edited

Loading

jasperhuangg left a comment •

edited

Loading

eVoloshchak commented Feb 6, 2023 •

edited

Loading