[HOLD for payment 2021-11-04] Sign in - Cursor does not remain in place after wrong password is entered: reported by @kidroca

[isagoico]

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:

1. Navigate to New Expensify
2. Enter a existing user email
3. In Password box enter an incorrect password.
4. Press enter

Edit: Please, consider the same steps and expected result for Two factor authentication fields too. These two flows are tightly connected and the solution will be basically same so it does not make much sense to create another issue.

Expected Result:

Cursor should stay in password box so user can correct the mistake without the need to lift the hands off the keyboard.

Actual Result:

Cursor no longer remains in password box, user must manually click into password box to try again.

Workaround:

Manually click into password box

Platform:

Where is this issue occurring?

• Web
• Desktop App

Version Number: 1.0.96-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

New.Expensify.-.Google.Chrome.2021-09-09.19-36-49.mp4

Expensify/Expensify Issue URL:

Issue reported by: @kidroca (Thanks for the video! 🙇‍♀️)
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1631205732121400

View all open jobs on GitHub

[MelvinBot]

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

[alex-mechler]

Looks like a good external issue. cc @kidroca if you wanted to submit a proposal for this

[MelvinBot]

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

[PrashantMangukiya]

Proposed Solution

We have to set password input field reference. Thereafter we can set focus to password input field if empty or invalid password.

Below code should be added/updated within src/pages/signin/PasswordForm.js

 
  constructor(props) {
      super(props);
     ....
      this.passwordInputRef = null;             //  *** Added this line
      this.twoFactorAuthCodeInputRef = null;   //  *** Added this line
  }

  // *** Added componentDidUpdate
  componentDidUpdate(prevProps) {
      if (this.props.account.error !== prevProps.account.error) {
          if (this.props.account.error === this.props.translate('passwordForm.error.incorrectLoginOrPassword')) {
              this.passwordInputRef.focus();
              return;
          } 
          if (this.props.account.error === this.props.translate('passwordForm.error.twoFactorAuthenticationEnabled')) {
              this.twoFactorAuthCodeInputRef.focus();
          }
      }
  }

  validateAndSubmitForm() { 
    if (!this.state.password.trim() && this.props.account.requiresTwoFactorAuth && !this.state.twoFactorAuthCode.trim()) {
        ....
        this.passwordInputRef.focus();  //  *** Added this line
        ....
    }

    if (!this.state.password.trim()) {
        ....
        this.passwordInputRef.focus();     //  *** Added this line
        ....
    }

    if (this.props.account.requiresTwoFactorAuth && !this.state.twoFactorAuthCode.trim()) {
        ....
        this.twoFactorAuthCodeInputRef.focus();   //  *** Added this line
        ....
    }

    ...
  }

  <ExpensiTextInput
     .... 
      ref={el => this.passwordInputRef = el}    // ** added this line
  />

  <ExpensiTextInput
      ...
      ref={el => this.twoFactorAuthCodeInputRef = el}   // ** added this line
  />

Here is how It works once code updated. (EN)

Screen.Recording.2021-09-11.at.11.25.15.AM.mov

Here is how It works once code updated. (ES)

Screen.Recording.2021-09-11.at.11.29.31.AM.mov

Other Suggestion I can see twoFactorAuthCode related input field also there in this form. It will be used if 2fa requited for the account. But In this issue 2fa input related things not mentioned, So in my proposal focus to 2fa input field related solution not included.

If set focus to 2fa input fields also taken care in this issue then kindly inform me, I will consider in this proposal.

If 2fa input fields focus needs to taken care via separate issue then I will suggest @isagoico to create another issue for 2fa input focus and kindly mention me as bug suggested by. I will also provide solution if issue assigned to me immediately, I have solution ready for it.

Thank you.

[kidroca]

I think we should find out why the field looses focus in the first place, and address that - it might be easier to keep focus from leaving the field instead of programatically triggering focus on different occasions

[MelvinBot]

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

[stephanieelliott]

Posted to Upwork: https://www.upwork.com/jobs/~01d33acc9f122f85b8

@mountiny we've got a few proposals already, see above comments!

[princeofdev]

Hello, it is Sasha from Upwork.
I could solve your problem.

[mananjadhav]

Proposal
The recommended way of solving this would be to addblurOnSubmit to the field.

<ExpensiTextInput
                        label={this.props.translate('common.password')}
                        secureTextEntry
                        autoCompleteType="password"
                        textContentType="password"
                        value={this.state.password}
                        onChangeText={text => this.setState({password: text})}
                        onSubmitEditing={this.validateAndSubmitForm}
                        autoFocus
                        translateX={-18}
                        blurOnSubmit={false}
                    />

Working Example:

Screen.Recording.2021-09-13.at.11.32.07.PM.mov

[kidroca]

I don't mind having someone else propose and implement a solution

IMO what @mananjadhav proposed looks good if it doesn't affect how ios and Android work, e.g. We should not have regressions like the onscreen keyboard staying open after submit.

[mananjadhav]

IMO what @mananjadhav proposed looks good if it doesn't affect how ios and Android work, e.g. We should not have regressions like the onscreen keyboard staying open after submit.

Well, it does, but if we don't want this behavior, all we need to do is TextInputFocusable/index.os.js and TextInputFocusable/index.android.js we don't pass the prop.

For example:

// Selection Property not worked in IOS properly, So removed from props.
        const {selection, blurOnSubmit, ...newProps} = this.props;
        return (
            <TextInput
                placeholderTextColor={themeColors.placeholderText}
                ref={el => this.textInput = el}
                maxHeight={CONST.COMPOSER_MAX_HEIGHT}
                rejectResponderTermination={false}
                editable={!this.props.isDisabled}
                /* eslint-disable-next-line react/jsx-props-no-spreading */
                {...newProps}
            />
        );

[PrashantMangukiya]

I tested with blurOnSubmit={false} within Google chrome, Safari. It is not working as expected i.e. Focus not remain in text input after submit. Tested within Expensify version 1.0.97-0

Google chrome (v93.0.4577.82)

Screen.Recording.2021-09-14.at.9.48.29.AM.mov

Google chrome (v93.0.4577.82) Incognito

Screen.Recording.2021-09-14.at.9.45.35.AM.mov

Safari v14.1.1.

Screen.Recording.2021-09-14.at.9.46.26.AM.mov

[princeofdev]

Hello all, I know it could be resolved by setting an initial state of password input and resetting it once the password was wrong.

…

On Mon, Sep 13, 2021 at 9:30 PM PrashantKumar Mangukiya < ***@***.***> wrote: I tested with blurOnSubmit={false} within Google chrome, Safari. It is not working as expected i.e. Focus not remain in text input after submit. Tested within Expensify version 1.0.97-0 Google chrome (v93.0.4577.82) https://user-images.githubusercontent.com/7823358/133194730-e4bdb30d-d25a-42f6-b42a-6cc8ba8a7afb.mov Google chrome (v93.0.4577.82) Incognito https://user-images.githubusercontent.com/7823358/133194792-7f32abae-187f-457b-96f0-10866a1b3075.mov Safari v14.1.1. https://user-images.githubusercontent.com/7823358/133194859-6f375467-3580-4af8-9dbb-05ab6b3b8952.mov — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5191 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUWEV7BY3YSGS4SFRZV35JDUB3FVTANCNFSM5DZWNCLA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[mountiny]

Thank you for your proposals! It would nice to figure out why this is happening at first place (as @kidroca mentioned), so we can dig in a bit more since this is not really a blocker but it is rather nice-to-have.

@mananjadhav Could you please double check what @PrashantMangukiya raised about your proposal? Also additionally if you could add screen recording of iOS and Android implementation of your proposal since you wrote you could prevent the keyboard showing up. Thank you! 🙇

If the blurOnSubmit proposal proves to introduce regressions on iOS/Android, the proposal from @PrashantMangukiya is also quite nice. However, we should first make sure there is not more elegant solution available.

Feel free to add some new proposals if you can think of anything useful 🚀

[mananjadhav]

@mountiny It seems @PrashantMangukiya's findings are correct blurOnSubmit doesn't seem to work well.

Here's my updated Proposal:
In PasswordForm, we import:

import withWindowDimensions, {windowDimensionsPropTypes} from '../../components/withWindowDimensions';

and then in validateAndSubmitForm we just focus the field again with the following block:

validateAndSubmitForm() {
        if (!this.props.isSmallScreenWidth) {
            this.passwordInputRef.focus();
        }
        ...
        ...
        ...
}

and finally, add a reference to the field.

<ExpensiTextInput
     label={this.props.translate('common.password')}
     ref={el => this.passwordInputRef = el}
     ...
     ...
     ...
/>                     

Web Video
https://user-images.githubusercontent.com/3069065/133326763-4b6bdfb5-e52f-4774-adab-85f673badbfb.mov

iOS Video

Screen.Recording.2021-09-15.at.1.49.34.AM.mov

[mountiny]

@mananjadhav Thank you for updating the proposal. I would say I prefer this approach a bit more than @PrashantMangukiya's proposal. It is a bit more clearer and straightforward without a compromise (as far as what we can see now).

I realize the 2FA field has not been mentioned in the original issue report, however, I would say it makes sense to make these changes together as it will be basically same code (and it is tightly couple with the password flow). Would you please be able to update the proposal so the same expected for password field applies for 2FA field too?

Let me know what you think about that and I think after that update your proposal should be good to go ahead 🎉

Update: As per @PrashantMangukiya comment below, I have overlooked this solution is not really considering what is happening when focusing the field, but focuses it on every submit. Which is not exactly what we want.

[kadiealexander]

Hi @mananjadhav, I've placed this issue on hold as per this update, as we are prioritising issues related to a feature release scheduled for 9/24. As an apology for the delay, we will add a $100 bonus as a thank you for waiting.

[mountiny]

Adding Reviewing label, the PR has been reviewed and is ready to be merged once we are out with N6

[mananjadhav]

Thanks @mountiny

[kadiealexander]

Please refer to this post for updated information on the n6 hold, we've raised the bonus to $250 for all issues where a PR is created before the N6 hold is lifted.

[MelvinBot]

This issue has not been updated in over 15 days. @mananjadhav, @stephanieelliott, @mountiny eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[mountiny]

Weekly, waiting for the n6-hold to be lifted.

[stephanieelliott]

N6 hold is lifted, payment for this job will include a $250 bonus. PR is currently undergoing QA

[stephanieelliott]

This was deployed to prod yesterday, starting 7-day regression period. Note to include $250 bonus with job payment.

[mountiny]

Created an issue to investigate/fix why the deploy comment has not been added here. https://github.com/Expensify/Expensify/issues/183206

[stephanieelliott]

All paid up!

[kidroca]

Hey @stephanieelliott according to the docs I should be getting a bonus for reporting the issue

https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#proposing-a-job-that-expensify-hasnt-posted

After review in #expensify-open-source, if we choose to implement your proposal, a GitHub issue will be created and your Slack handle will be included in the original post after Issue reported by:
If an external contributor other than yourself is hired to work on the issue, you will also be hired for the same job in Upwork. No additional work is needed. If the issue is fixed internally, a dedicated job will be created to hire and pay you after the issue is fixed.

[stephanieelliott]

Ah, that info is usually included in the issue title. thanks @kidroca!

I created a new job posting in Upwork to pay you out for reporting. Here it is: https://www.upwork.com/jobs/~014c5a6b4de03beeba

[kidroca]

Thank you, I've applied on Upwork.
I think this is the first issue I've reported and the guidelines weren't fully defined back then

[stephanieelliott]

All paid up, thanks @kidroca!