Bernardo Rittmeyer [:rittme] Assignee
	Description • 9 years ago

We want to be able to create a formLike object from a username field. 
To do that that we need to change the FormLikeFactory.createFromPasswordField method to also accept input fields of type text.

	Bernardo Rittmeyer [:rittme] Assignee
	Comment 1 • 9 years ago

Bug 1192081 - Changed createFromPasswordField to also create formlikes from text fields. r=MattN

	Bernardo Rittmeyer [:rittme] Assignee
	Comment 2 • 9 years ago

Comment on attachment 8646066 [details]
MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

Bug 1192081 - Changed createFromPasswordField to also create formlikes from text fields. r=MattN

	Matthew N. [:MattN]
	Comment 3 • 9 years ago

Comment on attachment 8646066 [details]
MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

https://reviewboard.mozilla.org/r/15607/#review13983

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1237
(Diff revision 2)
> -        aPasswordField.type != "password" ||
> +        (aInputField.type != "password" && aInputField.type != "text") ||

Please use the existing username field rules which support more than type=text like I mentioned in our 1:1. See `_isUsernameFieldType`

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1235
(Diff revision 2)
> -  createFromPasswordField(aPasswordField) {
> +  createFromInputField(aInputField) {

"input" and "field" are somewhat redundant. How about createFromField with aField (which can include other fields like <select> and <textarea> that I think we will want to support eventually)?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1223
(Diff revision 2)
>     * Create a FormLike object from an <input type=password>.

This is stale

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:615
(Diff revision 2)
> +   * Note that even than we can create a formLike from a text field,

s/even than/even though/

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:617
(Diff revision 2)
> +   * formLike have a password field.

s/have/has/

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:615
(Diff revision 2)
> +   * Note that even than we can create a formLike from a text field,
> +   * this method will only return a non-null usernameField if the
> +   * formLike have a password field.

NitL Capitalize FormLike when referring to the data type.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1233
(Diff revision 2)
> -   * @throws Error if aPasswordField isn't a password input in a document
> +   * @throws Error if aInputField isn't a password input in a document

Stale comment

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1231
(Diff revision 2)
> -   * @param {HTMLInputElement} aPasswordField - a password field in a document
> +   * @param {HTMLInputElement} aInputField - a password field in a document

Stale comment. This should be a username or password input?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1239
(Diff revision 2)
> -      throw new Error("createFromPasswordField requires a password field in a document");
> +      throw new Error("createFromInputField requires an input field in a document");

username or password field

::: toolkit/components/passwordmgr/test/test_formless_submit.html:57
(Diff revision 2)
>      document: `<input value="user1">
>        <input type=password value="pass1">`,
> +    inputIndexForFormLike: 0,
> +    hostname: DEFAULT_ORIGIN,
> +    formSubmitURL: DEFAULT_ORIGIN,
> +    usernameFieldValue: "user1",
> +    newPasswordFieldValue: "pass1",
> +    oldPasswordFieldValue: null,
> +  },
> +  {
> +    document: `<input value="user1">
> +      <input type=password value="pass1">`,
>      inputIndexForFormLike: 1,
>      hostname: DEFAULT_ORIGIN,
>      formSubmitURL: DEFAULT_ORIGIN,
>      usernameFieldValue: "user1",
>      newPasswordFieldValue: "pass1",
>      oldPasswordFieldValue: null,
>    },

These two cases looks the same. What am I missing?

::: toolkit/components/passwordmgr/test/unit/test_getFormFields.js:21
(Diff revision 2)
> +    description: "1 text field outside of a <form>",

"…without a password field"

(be explicit as I can see someone expecting this to work)

::: toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js:19
(Diff revision 2)
> -    returnedFieldIDsByFormLike: [],
> +    // Only the IDs of password fields should be at this array

Nit: s/at/in/

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1230
(Diff revision 2)
>     *

Might as well update the commit message too:
* update method name
* s/text/username/
* s/formlikes/FormLikes/

	Bernardo Rittmeyer [:rittme] Assignee
	Comment 4 • 9 years ago

Comment on attachment 8646066 [details]
MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

	Bernardo Rittmeyer [:rittme] Assignee
	Comment 5 • 9 years ago

https://reviewboard.mozilla.org/r/15607/#review13983

Thank you for your review, Matt!

I moved `_isUsernameFieldType` from the `LoginManagerContent` object to the `LoginUtils` object, as it's a usefull function outside that object too.

> These two cases looks the same. What am I missing?

In the first case we use the username input to create the FormLike.
In the second case we use the password input to create the FormLike. 
They are almost the same, but this difference allow us to know if createFromField is doing the right thing for usernames too.

	Bernardo Rittmeyer [:rittme] Assignee
	Comment 6 • 9 years ago

Comment on attachment 8646066 [details]
MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

	Bernardo Rittmeyer [:rittme] Assignee
	Comment 7 • 9 years ago

New revision with the `_isUsernameFieldType` method moved to LoginHelper, so we can also use it elsewhere. 
I will need to use this on `nsContextMenu.js` for bug 1188719.

	Matthew N. [:MattN]
	Comment 8 • 9 years ago

Comment on attachment 8646066 [details]
MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

https://reviewboard.mozilla.org/r/15607/#review14105

Ship It!

	Pulsebot
	Comment 9 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/87cace3f3eac

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/87cace3f3eac

	Matthew N. [:MattN]
	Comment 11 • 9 years ago

Comment on attachment 8646066 [details]
MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

Approval Request Comment
[Feature/regressing bug #]: Dependency of bug 1188719
[User impact if declined]: Not able to fill logins from a non-password field e.g. username field.
[Describe test coverage new/current, TreeHerder]: Updated unit tests
[Risks and why]: Low risk change to make non-password fields work just like password fields already do.
[String/UUID change made/needed]: None

	Sylvestre Ledru [:Sylvestre]
	Comment 12 • 9 years ago

Comment on attachment 8646066 [details]
MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN

Seems like it didn't cause any regression, taking it for the other patch.
Should be in 42 beta 5

	Carsten Book [:Tomcat]
	Comment 13 • 9 years ago

This cause conflicts while uplifting to beta:

grafting 257353:87cace3f3eac "Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN"
merging toolkit/components/passwordmgr/LoginHelper.jsm
warning: conflicts during merge.
merging toolkit/components/passwordmgr/LoginHelper.jsm incomplete! (edit conflicts, then use 'hg resolve --mark')
merging toolkit/components/passwordmgr/LoginManagerContent.jsm
abort: unresolved conflicts, can't continue


could you take a look, thanks!

	Carsten Book [:Tomcat]
	Comment 14 • 9 years ago

matt: since this is planned to be in beta, could you take a look

	Matthew N. [:MattN]
	Comment 15 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/6398da8b9482

	Matthew N. [:MattN]
	Comment 16 • 9 years ago

(In reply to Carsten Book [:Tomcat] from comment #14)
> matt: since this is planned to be in beta, could you take a look

I was already trying to land this yesterday but the tree was closed.