Make login form accessible to Password Managers #5275

parasharrajat · 2021-09-15T19:55:01Z

Details

Fixed Issues

$ #3855
$ #5956

Tests | QA Steps

Navigate to NewDot on the web.
Right-click on the email field.
Click on the 1Password option to show the autofill.
1Password Autofill option should be shown on the input.

Tested On

Screenshots

Web

Updating Shortly...

Mobile Web

Desktop

iOS

Android

…form

AndrewGable · 2021-09-29T16:25:48Z

Conflicts!

src/libs/TextInputUtils/index.js

src/libs/TextInputUtils/index.native.js

marcaaron · 2021-09-29T23:25:43Z

src/pages/signin/PasswordForm.js

                        textContentType="password"
+                        nativeID={getPasswordAutocompleteType()}


Just curious but what do we need to set the nativeID for ?

For Attaching the label with input. This could be optional. There are a lot of uncertainties in the case of password Managers so I used everything that we generally do for login forms. Another reason was to just point out these things in the review and decide if they are necessary.

marcaaron · 2021-09-29T23:26:19Z

src/pages/signin/SignInPageLayout/SignInPageLayoutNarrow.js

+
+    componentDidMount() {
+        // These native props are needed for Password Managers like LastPass
+        if (this.form) {


early return please

marcaaron · 2021-09-29T23:30:45Z

src/pages/signin/SignInPageLayout/SignInPageLayoutNarrow.js

+        // These native props are needed for Password Managers like LastPass
+        if (this.form) {
+            setNativePropsWeb(this.form, 'method', 'post');
+            setNativePropsWeb(this.form, 'action', '/');


We should encapsulate this logic in a new kind of view then pass these as props. The component can then handle setting these native props with this componentDidMount() logic instead of having to repeat it everywhere we want to do this.

Also please add some comments about why method and action are required in this case

marcaaron · 2021-09-29T23:33:02Z

src/components/ExpensiTextInput/BaseExpensiTextInput.js

@@ -44,6 +45,9 @@ class BaseExpensiTextInput extends Component {
        if (this.props.autoFocus && this.input) {
            this.input.focus();
        }
+        if (this.input && this.props.name) {


If there is no input we should return early instead of checking on like 48 and 45

Yes, absolutely.

parasharrajat · 2021-10-01T16:31:43Z

Thanks for the review Marc. I will resolve them as soon as possible.

marcaaron · 2021-10-18T21:29:45Z

What's the status on this one @parasharrajat ?

parasharrajat · 2021-10-18T21:42:53Z

Not yet updated. I will ping you when done.

parasharrajat · 2021-11-07T07:51:07Z

@rushatgabhane Could you please review this and tell me if it works for you?

rushatgabhane · 2021-11-07T09:53:27Z

Ideally, once 1P is used for entering the email address, the Password on the following page will be autofilled as well.

I tested this branch with Bitwarden as my password manager.
The password field in next page isn't autofilled.

Screencast.from.07-11-21.11-47-33.AM.+03.mp4

rushatgabhane · 2021-11-07T09:59:43Z

For Android, I'm getting this error on launch.

Warning: Failed prop type: ExpensiTextInputLabel: prop type `for` is invalid; it must be a function, usually from the `prop-types` package, but received `string`.This often happens because of typos such as `PropTypes.function` instead of `PropTypes.func`.
    in ExpensiTextInputLabel (at BaseExpensiTextInput.js:189)
    in RCTView (at View.js:34)
    in View (at BaseExpensiTextInput.js:177)
    in TouchableWithoutFeedback (at BaseExpensiTextInput.js:176)
    in RCTView (at View.js:34)
    in View (at BaseExpensiTextInput.js:170)
    in RCTView (at View.js:34)
    in View (at BaseExpensiTextInput.js:169)
    in BaseExpensiTextInput (at ExpensiTextInput/index.android.js:7)

parasharrajat · 2021-11-07T11:15:16Z

@rushatgabhane Thanks for testing it. I have pushed new changes, please check again. Let's see I tested with lastpass.

rushatgabhane · 2021-11-07T16:12:38Z

@parasharrajat Works perfectly! 💯

parasharrajat · 2021-11-07T16:22:40Z

Thanks, @rushatgabhane. It would have not been possible without your help.

If you are curious to know, what was the issue?

Password Managers do not track dom changes unless page changes. So when we were switching between the login field component to the password field component, the selection of login you made in the login page from the password manager is also lost during this transition.

To fix this, we have to keep all the form fields, login, and password in the dom, keeping the relation between them.

Also, as we use the multistep form, I have to visually hide the other dom nodes based on the current step to keep the UI intact.

rushatgabhane · 2021-11-07T16:31:51Z

Thanks for the clarity. It makes sense now :)

parasharrajat · 2021-11-07T20:23:10Z

I will soon finalize the PR. All changes are done. Cleanup is left.

parasharrajat · 2021-12-02T16:48:10Z

@marcaaron Ready for review. Major refactor... 😄

marcaaron · 2021-12-02T17:44:18Z

src/components/CollapsibleView.js

+    isVisible: false,
+};
+
+const CollapsibleView = props => (


ToggleVisibilityView maybe? Collapsible makes me think of things like this -> https://github.com/Eliav2/react-native-collapsible-view

Yup, Thanks for the better name

marcaaron · 2021-12-02T17:47:08Z

src/components/TextInputWithName/index.js

+import textInputWithNamepropTypes from './textInputWithNamepropTypes';
+
+/**
+ * On web we need to set the native attribure name for accessiblity.


attribute

marcaaron · 2021-12-02T17:51:17Z

src/pages/signin/SignInPage.js

+                    </CollapsibleView>
+                    <CollapsibleView isVisible={showPasswordForm}>
+                        {isVisible => <PasswordForm isVisible={isVisible} />}
+                    </CollapsibleView>


After seeing this I think this utility would be a bit cleaner as an HOC. Passing the prop only to immediately consume it looks kind of funny.

Hmm. Let me do it that way.

Actually, prop is used to manage side effects based on visibility.

Could maybe pass the prop through to the wrapped component?

src/pages/signin/PasswordForm.js

src/pages/signin/LoginForm.js

parasharrajat · 2021-12-02T19:45:43Z

Ready.

parasharrajat · 2021-12-03T18:14:19Z

Ready for review.

marcaaron

Looks good - just have a few more suggestions.

src/components/withToggleVisibilityView.js

src/pages/signin/LoginForm.js

marcaaron · 2021-12-03T21:31:46Z

src/pages/signin/LoginForm.js

+     * Clear Login from the state
+     */
+    clearLogin() {
+        this.setState({login: ''}, this.input.clear);


Why call clear on this? Is ExpensiTextInput not a controlled component?

Yeah. It's not. I just saw that we don't pass the value directly. ExpensiTextInput is using value={this.value}. Actually, we never faced a situation to make it controlled.

Oh hmm weird I don't get it. We are passing a value - is it used or not ? 🤔

It seems like a bad habit to call this.input.clear every time we update a value passed to ExpensifyTextInput to be an empty string. Why do we need to do this?

I wasn't using it before but after recent changes, ExpensifyTextInput value is not clearing on updating the state variable login. But as soon as you blur it, values clear. So I think that using this.value instead of the state variable is causing this issue on ExpensifyTextInput.
As far as I know, it's not affecting other parts of the app so I don't think that refactor is necessary. If we face this issue again then I would suggest making the ExpensifyTextInput controlled.

src/pages/signin/PasswordForm.js

src/pages/signin/SignInPage.js

…typos

parasharrajat · 2021-12-03T23:04:51Z

Changes Done.

marcaaron

LGTM, I'm not too sure about that this.input.clear usage - feels like something we should not have to worry about.

parasharrajat · 2021-12-06T19:08:03Z

@Jag96 This is ready for review and merge.

Jag96

Looks good and tests well, looks like just one copy-paste mistake

Jag96 · 2021-12-06T19:52:49Z

src/components/ExpensiTextInput/ExpensiTextInputLabel/index.native.js

@@ -36,6 +36,7 @@ class ExpensiTextInputLabel extends PureComponent {
    }
 }

-ExpensiTextInputLabel.propTypes = propTypes;
+ExpensiTextInputLabel.propTypes = expensiTextInputLabelPropTypes.propTypes;
+ExpensiTextInputLabel.propTypes = expensiTextInputLabelPropTypes.defaultProps;


ExpensiTextInputLabel.defaultProps =

Yeah, Nice catch.

OSBotify · 2021-12-06T21:08:55Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

parasharrajat · 2021-12-06T21:42:51Z

Unfortunately, found an issue with this PR.

The keyboard does not close when you switch between the login and password pages on Android. As a side-effect, View is not correctly placed above the keyboard when the page changes. It happens due to keyboardShouldPersistTaps="handled" prop. Previously unmounting the input was used to close the Keyboard.

I guess, we will have to manually blur the input when isvisible is false in LoginForm and PasswordForm. @Jag96 . I will spin up the PR tomorrow.

OSBotify · 2021-12-07T00:20:14Z

🚀 Deployed to staging by @Jag96 in version: 1.1.17-8 🚀

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

OSBotify · 2021-12-08T20:12:06Z

🚀 Deployed to production by @roryabraham in version: 1.1.18-3 🚀

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

parasharrajat added 4 commits September 16, 2021 00:53

make Input accessible

5b66bdc

fix: input for password Managers

8fcf1fd

adjust the Form for Password Managers

228b1b2

Merge branch 'main' of github.com:Expensify/Expensify.cash into login…

69f0e1d

…form

parasharrajat requested a review from a team as a code owner September 15, 2021 19:55

MelvinBot requested review from marcaaron and removed request for a team September 15, 2021 19:55

parasharrajat changed the title ~~Make login for accessible to Password Managers~~ Make login form accessible to Password Managers Sep 15, 2021

marcaaron requested changes Sep 29, 2021

View reviewed changes

parasharrajat mentioned this pull request Oct 20, 2021

[HOLD #3855]1Password - user is prompted to enter the master password 2 times (for email and password) #5956

Closed

parasharrajat added 4 commits November 6, 2021 21:59

Merge latest chagnes from master

31f6ebb

Refactor code

0c95acf

Refactor Form

08a441e

last touch up

f5c1709

parasharrajat added 2 commits November 7, 2021 16:24

Enable multi steo form

28b22f1

Multi step form support

bf7859e

parasharrajat changed the title ~~Make login form accessible to Password Managers~~ [WIP]Make login form accessible to Password Managers Nov 7, 2021

refactor page to set the focus correctly

b180d80

Clear form

a1c5ac0

marcaaron requested changes Dec 2, 2021

View reviewed changes

parasharrajat added 2 commits December 3, 2021 01:09

Review -2

530627f

review-3

b594059

parasharrajat requested a review from marcaaron December 2, 2021 19:45

Extra prop definition removed

db6b4b6

marcaaron requested changes Dec 3, 2021

View reviewed changes

Reafactor code to remove isvisible dependency in PasswordForm UI and …

4da3cb1

…typos

parasharrajat requested a review from marcaaron December 3, 2021 23:04

marcaaron previously approved these changes Dec 4, 2021

View reviewed changes

parasharrajat requested a review from Jag96 December 4, 2021 11:08

Jag96 suggested changes Dec 6, 2021

View reviewed changes

fix prop definition

d39fa49

parasharrajat dismissed marcaaron’s stale review via d39fa49 December 6, 2021 20:22

parasharrajat requested a review from Jag96 December 6, 2021 20:22

Jag96 approved these changes Dec 6, 2021

View reviewed changes

Jag96 merged commit 89d9dc1 into Expensify:main Dec 6, 2021

OSBotify mentioned this pull request Dec 6, 2021

Deploy Checklist: New Expensify 2021-12-06 #6605

Closed

95 tasks

botify mentioned this pull request Dec 8, 2021

[HOLD for payment 2021-12-15] Unable to use 1Password option for autofill on login #3855

Closed

parasharrajat mentioned this pull request Dec 12, 2021

Clean Old password when user click goBack #6723

Merged

5 tasks

rushatgabhane mentioned this pull request Jan 7, 2022

Password doesn't autofill when using lastpass and the autofill icon disappears - Reported by @rushatgabhane #7092

Closed

		textContentType="password"
		nativeID={getPasswordAutocompleteType()}

Make login form accessible to Password Managers #5275

Make login form accessible to Password Managers #5275

Conversation

parasharrajat commented Sep 15, 2021 • edited by Jag96 Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

AndrewGable commented Sep 29, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Oct 1, 2021

marcaaron commented Oct 18, 2021

parasharrajat commented Oct 18, 2021

parasharrajat commented Nov 7, 2021

rushatgabhane commented Nov 7, 2021

rushatgabhane commented Nov 7, 2021

parasharrajat commented Nov 7, 2021

rushatgabhane commented Nov 7, 2021

parasharrajat commented Nov 7, 2021

rushatgabhane commented Nov 7, 2021

parasharrajat commented Nov 7, 2021

parasharrajat commented Dec 2, 2021

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

parasharrajat Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat commented Dec 2, 2021

parasharrajat commented Dec 3, 2021

marcaaron 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

parasharrajat Dec 4, 2021 • edited Loading

Choose a reason for hiding this comment

parasharrajat commented Dec 3, 2021

marcaaron left a comment

Choose a reason for hiding this comment

parasharrajat commented Dec 6, 2021 • edited Loading

Jag96 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

OSBotify commented Dec 6, 2021

parasharrajat commented Dec 6, 2021

OSBotify commented Dec 7, 2021

OSBotify commented Dec 8, 2021

parasharrajat commented Sep 15, 2021 •

edited by Jag96

Loading

parasharrajat Dec 2, 2021 •

edited

Loading

parasharrajat Dec 4, 2021 •

edited

Loading

parasharrajat commented Dec 6, 2021 •

edited

Loading