Closed Bug 1192081 Opened 4 years ago Closed 4 years ago

Create FormLike from text input

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Iteration:
43.1 - Aug 24
Tracking Status
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: rittme, Assigned: rittme)

References

()

Details

Attachments

(1 file)

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.
Flags: qe-verify-
Flags: firefox-backlog?
Bug 1192081 - Changed createFromPasswordField to also create formlikes from text fields. r=MattN
Attachment #8646066 - Flags: review?(MattN+bmo)
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
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/
Attachment #8646066 - Flags: review?(MattN+bmo) → review+
Attachment #8646066 - Attachment description: MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create formlikes from text fields. r=MattN → MozReview Request: Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN
Attachment #8646066 - Flags: review+ → review?(MattN+bmo)
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
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.
Iteration: 42.3 - Aug 10 → 43.1 - Aug 24
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
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.
Attachment #8646066 - Flags: review?(MattN+bmo) → review+
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!
Flags: firefox-backlog? → firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/87cace3f3eac
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Attachment #8646066 - Flags: approval-mozilla-beta?
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
Attachment #8646066 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
Flags: needinfo?(bernardo)
matt: since this is planned to be in beta, could you take a look
Flags: needinfo?(MattN+bmo)
(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.
Flags: needinfo?(bernardo)
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.