[$1000] 'Phone or email' field is not autofocused after logout of account

kavimuru commented 1 year ago

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:

Open the app
Login with any account
Open settings and sign out

Expected Result:

App should focus back on 'Phone or email' textbox as it does on mac safari, mac chrome, windows chrome

Actual Result:

App does not focus on 'Phone or email' textbox on sign out

Workaround:

unknown

Platforms:

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

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.2.85-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:

https://user-images.githubusercontent.com/43996225/225316915-03dc55e8-1304-4698-8e2e-e27b6d8ea84e.mp4

https://user-images.githubusercontent.com/43996225/225316920-79a026c1-fb5e-4ec4-8a63-9f566008fa9e.mp4

https://user-images.githubusercontent.com/43996225/225316986-e2afbba6-6e42-4c4d-99b2-6a4c4e01efb4.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1678728571031189

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01984166ddcdbf5972
Upwork Job ID: 1638328536890818560
Last Price Increase: 2023-03-21

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[x] 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
[x] 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.
[x] 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.
[x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

alexpensify commented 1 year ago

I'll try to review this one by EOW

alexpensify commented 1 year ago

Ops GHs have been a higher priority, I haven't had time to test yet.

alexpensify commented 1 year ago

Focused on another urgent task, I'll try to get to this one tomorrow.

MelvinBot commented 1 year ago

Triggered auto assignment to @hayata-suenaga (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

alexpensify commented 1 year ago

@hayata-suenaga - I tested and am taken back to log in. I need clarification about what the UX changes are asked for here or the problem. I'd love some feedback if you agree with this one. Thanks!

hayata-suenaga commented 1 year ago

I'll check this today.

daraksha-dk commented 1 year ago

Proposal

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

'Phone or email' field is not autofocused after logout of account for Native devices

What is the root cause of that problem?

This is the intended behaviour as per our code.

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

If we don't want this behaviour anymore then we'll have to remove the use of canFocusInputOnScreenFocus from LoginForm.js

https://github.com/Expensify/App/blob/f8d7a3a8e291cd216b26b22e9e8a69c93bb15e9e/src/pages/signin/LoginForm.js#L83-88

What alternative solutions did you explore? (Optional)

Additionally, if we can remove other uses of canFocusInputOnScreenFocus if it's not needed anymore (Currently, it's being used in 3 places only)

hayata-suenaga commented 1 year ago

I completed the triage checklist and I was able to repro

@daraksha-dk do you have any idea for what reasons we used to use canFocusInputOnScreenFocus?

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01984166ddcdbf5972

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

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

MelvinBot commented 1 year ago

Current assignee @hayata-suenaga is eligible for the External assigner, not assigning anyone new.

Snehal-Techforce commented 1 year ago

Proposal

Root Cause:

Missing autofocus support in TextInput component

Solution:

The autoFocus property of the TextInput component that used to focus the text input without any user interaction is added at line no 180 in LoginForm.js file to auto focus 'Phone or email' textbox in mobile app.

Expected Output:

tienifr commented 1 year ago

Proposal

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

App does not focus on 'Phone or email' textbox on sign out

What is the root cause of that problem?

The root cause is from here https://github.com/Expensify/App/blob/ebc03dbb14d1689e35301278833a82716ae53034/src/pages/signin/LoginForm.js#L83 where we're preventing the auto focus if we're on touch screen.

The reason for that can be traced back from here https://github.com/Expensify/App/pull/2363#discussion_r612643154. Back then we still have marketing stuff (image and welcome text) and we don't want to open the keyboard by default to hide them. (cc @hayata-suenaga this answers your question)

Now since the marketing stuff (image and welcome text) is no longer there and we only have a bare login form, it's safe to open the keyboard even on touch screens.

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

We need to remove the in this line https://github.com/Expensify/App/blob/ebc03dbb14d1689e35301278833a82716ae53034/src/pages/signin/LoginForm.js#L83 And also the same condition on the PasswordForm here and on the ValidateCodeForm here. The canFocusInputOnScreenFocus function itself can be removed because it's no longer needed.

After doing this, we'll see that it feels janky when we go to the Login page directly from URL on native. This is because the .focus opens the keyboard while the animations on the screen hasn't finished. To fix this we need to wrap the .focus call here inside InteractionManager.runAfterInteractions. We can create an util to focus the input with that logic and reuse it whenever we want to trigger .focus on the input similar to the ReportActionCompose here.

What alternative solutions did you explore? (Optional)

After the above fix, the input will be focused but the keyboard will not open if you navigate directly to the page by URL on iOS mWeb. iOS mWeb will not allow opening the keyboard without user interaction. We have a few ways to handle this:

Do nothing since the user can still click on the input to open the keyboard
Only trigger .focus if the user goes to the screen by logging out, this can be done by utilizing the authenticated state when the user log out.
Do not trigger .focus automatically on mWeb, if we want it to be consistent across interactions for mWeb.

s77rt commented 1 year ago

@daraksha-dk Thanks for the proposal. Your RCA is correct and the fix makes sense. However I think we need to get further confirmation if want to remove that condition.

s77rt commented 1 year ago

@Snehal-Techforce Thanks for the proposal. I don't think your RCA is correct as it does not explain why the auto focus work on Web.

s77rt commented 1 year ago

@tienifr Thanks for the proposal.Your RCA is correct and the fix looks good as well. It seems safe to remove that condition now but let's get further confirmation on this.

s77rt commented 1 year ago

@jasperhuangg @marcaaron Since you were involved in preventing auto focus on login form on mWeb/Native. Do you think it's safe to revert that change? Based on this https://github.com/Expensify/App/pull/2363#discussion_r612643154 it was only added not to hide the marketing stuff? Or also because the keyboard may be annoying for the user at that stage?

daraksha-dk commented 1 year ago

@hayata-suenaga As mentioned by @tienifr, this was the reason for using this approach -

The reason for that can be traced back from here https://github.com/Expensify/App/pull/2363#discussion_r612643154. Back then we still have marketing stuff (image and welcome text) and we don't want to open the keyboard by default to hide them.

I believe another reason for taking this approach was to prevent the jolting keyboard issue that occurs when you try to open all three forms directly. While this may be a separate issue or add-on, it could potentially be solved by utilizing the InteractionManager in all three forms.

hayata-suenaga commented 1 year ago

@tienifr's solution seems to be most comprehensive.

for Safari mweb not showing keyboard, i think it's fine we don't do nothing. The input will be still focused. the user just have to click the input field. I think that's an inconsistency minor enough that we can ignore.

if @jasperhuangg or @marcaaron confirm that canFocusInputOnScreenFocus can be removed, we can go with @tienifr's proposal.

marcaaron commented 1 year ago

Sorry, what change are we talking about? Can someone please ask a more specific question?

s77rt commented 1 year ago

@marcaaron

Open App on Native or mWeb
Logout
Should we auto focus the login field or not?

On Web/Desktop we do. But on Native and mWeb we don't and that seems intended due to the use of a conditional canFocusInputOnScreenFocus. At the time of the implementation it seemed intended not to hide marketing stuff but now we don't have any but still thinking maybe we should still not auto focus that field as it may be annoying?

marcaaron commented 1 year ago

Well, if you scroll down there is a bunch of content here...

IMO if there is no other action a user can take on a page besides to fill out an input then it might make sense to auto-focus otherwise we should use it sparingly. Though, that's just my opinion - I feel we are dealing with these autofocus questions quite often and the answer is usually that there's not a ton of value in doing A over B.

s77rt commented 1 year ago

Indeed, not much value on adding auto focus here. @hayata-suenaga What do you think? Given there is still content to be explored besides filling the login field, should we close the issue?

hayata-suenaga commented 1 year ago

ohhh I completely forgot we have footer section on the login screen. yes I think we can close this issue without any action.

tienifr commented 1 year ago

@hayata-suenaga @s77rt just to note the content to be explored is only for web (desktop, web, mWeb). I don't think we have such content on native. So we might consider auto focus on native if you think it's worth it.

hayata-suenaga commented 1 year ago

@tienifr thank you for the info. i believe we should have the same behavior across all platforms, so we can leave the current behavior

Expensify / App