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

Input does not handle Tab focus and autofocus correctly. #4514

Closed
parasharrajat opened this issue Aug 9, 2021 · 19 comments · Fixed by #4593
Closed

Input does not handle Tab focus and autofocus correctly. #4514

parasharrajat opened this issue Aug 9, 2021 · 19 comments · Fixed by #4593
Assignees
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering External Added to denote the issue can be worked on by a contributor Hourly KSv2 Reviewing Has a PR in review

Comments

@parasharrajat
Copy link
Member

parasharrajat commented Aug 9, 2021

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


Issues:

  1. New text input requires extra keystroke when using tab to get focus
  2. Autofocus is not set correctly.

Action Performed:

  1. Open the Login page.
  2. Observe the email input.
  3. Go to the password page, observe the password input. It is auto-filled, but input is not focused.
  4. Now try to navigate use the Tab key. Users have to press tag two times to set the focus on Input.

Expected Result:

When autofocus is used, Input should be focused and the border should be shown on the input.
The label should float top when input is focused.
Single Tab keystroke should set the focus in the input.

Actual Result:

Input is not focused and the label is overriding the Input text. The cursor is placed on the Input but the label is not placed correctly and there is no blue border that shows the focused state. Users have to press the tab two times to get focus on the input.

Workaround:

Clear the input and refocus.

Platform:

Where is this issue occurring?

  • Web
  • Desktop App
  • Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
image

Video

https://expensify.slack.com/files/UDQLCFDRA/F02AZA1V0GL/2021-08-10_10-31-21.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

Slack => https://expensify.slack.com/archives/C01GTK53T8Q/p1628627644189200

Upwork Job

@parasharrajat parasharrajat added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 9, 2021
@MelvinBot
Copy link

Triggered auto assignment to @strepanier03 (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 9, 2021
@strepanier03
Copy link
Contributor

Unique - wasn't able to find another issue similar
Issue hygiene - sections are filled out
Clarity of problem - The problem is not as clear as it could be as it was a little tough to parse out what I think you're seeing. I wasn't able to reproduce the behavior that I believe you're describing so next time it might be better to be a little more clear with what is happening or provide additional visual examples to illustrate.
Value - Polished new dot is critical so passing along.

@strepanier03 strepanier03 removed their assignment Aug 9, 2021
@MelvinBot
Copy link

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

@roryabraham
Copy link
Contributor

Nice triage checklist @strepanier03! 👏

@roryabraham
Copy link
Contributor

This can be external, but @parasharrajat can you take a minute to clean up the issue template to get rid of the prompt questions and get a bit more specific about the expected result?

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Aug 10, 2021
@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member Author

parasharrajat commented Aug 10, 2021

Proposal

  1. Autofocus is not working well one ExpensiTextInput. It does not call the onFocus event when autofocused. So we can move this logic to componentDidMount to focus the input and ignore passing the autofocus prop to TextInput which is not needed after this change. We would not want to use the InterractionManager as this is a low-level component and motive to mimic the autofocus behavior.
    componentDidMount() {
        if (this.props.autoFocus && this.input) {
            this.input.focus();
        }
    }

Which fixes the autofocus issue.

  1. We just need to bypass the Outer Element. It is only used to capture click and pass the focus to the main input. Adding focusable:false fix it.
    <TouchableWithoutFeedback onPress={() => this.input.focus()} focusable={false}>

Before Autofocus fix

focus.mp4

After Autofocus fix

focus-fix.mp4

@parasharrajat parasharrajat changed the title Input does not show correctly on autofocus & autofill Input does not handle Tab focus and autofocus correctly. Aug 10, 2021
@roryabraham
Copy link
Contributor

Hmmm @parasharrajat won't removing the autoFocus prop passed to the RN TextInput break auto-focusing entirely on iOS Safari? IIRC iOS Safari won't let you do this:

// This won't actually open the software keyboard on iOS Safari
this.input.focus();

@parasharrajat
Copy link
Member Author

parasharrajat commented Aug 11, 2021

It does when we do it synchronously which is exactly the case here in componentDidMount. And I just tested it on safari. Works well.
Sorry misinterpreted it. I checked on Web safari. Let me check on the IOS safari.

Edit:
@roryabraham Yup it works on both Web safari and IOS safari.

@roryabraham
Copy link
Contributor

Okay, thanks for checking that. In that case, your proposal for the autofocus issue LGTM 👍

What about the other formatting issue? cc @kakajann, because it seems that this is a case where this PR didn't work as expected.

@kakajann
Copy link
Contributor

@roryabraham I can open a new PR to fix any bugs occured in #3414

@roryabraham

This comment has been minimized.

@parasharrajat

This comment has been minimized.

@MitchExpensify
Copy link
Contributor

Upwork Job created

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 11, 2021
@MitchExpensify MitchExpensify removed their assignment Aug 11, 2021
@parasharrajat
Copy link
Member Author

I think this is not a regression(this issue is coming due to browser handling autofocus) but moreover missing cases from the new Input.

@roryabraham
Copy link
Contributor

@parasharrajat Feel free to submit a PR once you've been hired on Upwork.

@roryabraham roryabraham added DeployBlockerCash This issue or pull request should block deployment and removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 12, 2021
@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Aug 12, 2021
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@francoisl francoisl added the Reviewing Has a PR in review label Aug 12, 2021
@MitchExpensify
Copy link
Contributor

Hired in upwork @parasharrajat

@MitchExpensify
Copy link
Contributor

Paid! cc @parasharrajat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering External Added to denote the issue can be worked on by a contributor Hourly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants