Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN
authorBernardo P. Rittmeyer <bernardo@rittme.com>
Tue, 11 Aug 2015 16:03:50 -0700
changeset 257353 87cace3f3eac614c9f3d3a2b1ee8d2c9f17b8fb8
parent 257352 f57621f74e1033ee762a99b714067e7a53c4c42f
child 257354 f7a42f684d2da9da9804ed3f458c4c0b7e288bf4
push id29214
push userryanvm@gmail.com
push dateWed, 12 Aug 2015 13:12:39 +0000
treeherdermozilla-central@8584e1ea8f6a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1192081
milestone43.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1192081 - Changed createFromPasswordField to also create FormLikes from username fields. r=MattN
toolkit/components/passwordmgr/LoginHelper.jsm
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/test/test_formless_submit.html
toolkit/components/passwordmgr/test/unit/test_getFormFields.js
toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -311,9 +311,35 @@ this.LoginHelper = {
       win.setFilter(filterString);
       win.focus();
     } else {
       window.openDialog("chrome://passwordmgr/content/passwordManager.xul",
                         "Toolkit:PasswordManager", "",
                         {filterString : filterString});
     }
   },
+
+  /**
+   * Checks if a field type is username compatible.
+   *
+   * @param {Element} element
+   *                  the field we want to check.
+   *
+   * @returns {Boolean} true if the field type is one
+   *                    of the username types.
+   */
+  isUsernameFieldType(element) {
+    if (!(element instanceof Ci.nsIDOMHTMLInputElement))
+      return false;
+
+    let fieldType = (element.hasAttribute("type") ?
+                     element.getAttribute("type").toLowerCase() :
+                     element.type);
+    if (fieldType == "text"  ||
+        fieldType == "email" ||
+        fieldType == "url"   ||
+        fieldType == "tel"   ||
+        fieldType == "number") {
+      return true;
+    }
+    return false;
+  },
 };
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -289,17 +289,17 @@ var LoginManagerContent = {
     }
 
     let pwField = event.target;
     if (pwField.form) {
       // Handled by onDOMFormHasPassword which is already throttled.
       return;
     }
 
-    let formLike = FormLikeFactory.createFromPasswordField(pwField);
+    let formLike = FormLikeFactory.createFromField(pwField);
     log("onDOMInputPasswordAdded:", pwField, formLike);
 
     let deferredTask = this._deferredPasswordAddedTasksByRootElement.get(formLike.rootElement);
     if (!deferredTask) {
       log("Creating a DeferredTask to call _fetchLoginsFromParentAndFillForm soon");
       this._formLikeByRootElement.set(formLike.rootElement, formLike);
 
       deferredTask = new DeferredTask(function* deferredInputProcessing() {
@@ -470,17 +470,17 @@ var LoginManagerContent = {
     let form = topState.loginFormForFill;
     let clobberUsername = true;
     let options = {
       inputElement,
     };
 
     // If we have a target input, fills it's form.
     if (inputElement) {
-      form = FormLikeFactory.createFromPasswordField(inputElement);
+      form = FormLikeFactory.createFromField(inputElement);
       clobberUsername = false;
     }
     this._fillForm(form, true, clobberUsername, true, true, loginsFound, recipes, options);
   },
 
   loginsFound: function({ form, loginsFound, recipes }) {
     let doc = form.ownerDocument;
     let autofillForm = gAutofillForms && !PrivateBrowsingUtils.isContentWindowPrivate(doc.defaultView);
@@ -501,17 +501,17 @@ var LoginManagerContent = {
       return;
 
     var acInputField = event.target;
 
     // This is probably a bit over-conservatative.
     if (!(acInputField.ownerDocument instanceof Ci.nsIDOMHTMLDocument))
       return;
 
-    if (!this._isUsernameFieldType(acInputField))
+    if (!LoginHelper.isUsernameFieldType(acInputField))
       return;
 
     var acForm = acInputField.form; // XXX: Bug 1173583 - This doesn't work outside of <form>.
     if (!acForm)
       return;
 
     // If the username is blank, bail out now -- we don't want
     // fillForm() to try filling in a login without a username
@@ -572,63 +572,49 @@ var LoginManagerContent = {
     } else if (pwFields.length > 3) {
       log("(form ignored -- too many password fields. [ got ", pwFields.length, "])");
       return null;
     }
 
     return pwFields;
   },
 
-  _isUsernameFieldType: function(element) {
-    if (!(element instanceof Ci.nsIDOMHTMLInputElement))
-      return false;
-
-    let fieldType = (element.hasAttribute("type") ?
-                     element.getAttribute("type").toLowerCase() :
-                     element.type);
-    if (fieldType == "text"  ||
-        fieldType == "email" ||
-        fieldType == "url"   ||
-        fieldType == "tel"   ||
-        fieldType == "number") {
-      return true;
-    }
-    return false;
-  },
-
-
   /**
    * Returns the username and password fields found in the form.
    * Can handle complex forms by trying to figure out what the
    * relevant fields are.
    *
    * @param {FormLike} form
    * @param {bool} isSubmission
    * @param {Set} recipes
    * @return {Array} [usernameField, newPasswordField, oldPasswordField]
    *
    * usernameField may be null.
    * newPasswordField will always be non-null.
    * oldPasswordField may be null. If null, newPasswordField is just
    * "theLoginField". If not null, the form is apparently a
    * change-password field, with oldPasswordField containing the password
    * that is being changed.
+   *
+   * Note that even though we can create a FormLike from a text field,
+   * this method will only return a non-null usernameField if the
+   * FormLike has a password field.
    */
   _getFormFields : function (form, isSubmission, recipes) {
     var usernameField = null;
     var pwFields = null;
     var fieldOverrideRecipe = LoginRecipesContent.getFieldOverrides(recipes, form);
     if (fieldOverrideRecipe) {
       var pwOverrideField = LoginRecipesContent.queryLoginField(
         form,
         fieldOverrideRecipe.passwordSelector
       );
       if (pwOverrideField) {
         // The field from the password override may be in a different FormLike.
-        let formLike = FormLikeFactory.createFromPasswordField(pwOverrideField);
+        let formLike = FormLikeFactory.createFromField(pwOverrideField);
         pwFields = [{
           index   : [...formLike.elements].indexOf(pwOverrideField),
           element : pwOverrideField,
         }];
       }
 
       var usernameOverrideField = LoginRecipesContent.queryLoginField(
         form,
@@ -651,17 +637,17 @@ var LoginManagerContent = {
 
     if (!usernameField) {
       // Locate the username field in the form by searching backwards
       // from the first password field, assume the first text field is the
       // username. We might not find a username field if the user is
       // already logged in to the site.
       for (var i = pwFields[0].index - 1; i >= 0; i--) {
         var element = form.elements[i];
-        if (this._isUsernameFieldType(element)) {
+        if (LoginHelper.isUsernameFieldType(element)) {
           usernameField = element;
           break;
         }
       }
     }
 
     if (!usernameField)
       log("(form -- no username field found)");
@@ -1081,17 +1067,16 @@ var LoginUtils = {
     var uriString = form.action;
 
     // A blank or missing action submits to where it came from.
     if (uriString == "")
       uriString = form.baseURI; // ala bug 297761
 
     return this._getPasswordOrigin(uriString, true);
   },
-
 };
 
 // nsIAutoCompleteResult implementation
 function UserAutoCompleteResult (aSearchString, matchingLogins) {
   function loginSort(a,b) {
     var userA = a.username.toLowerCase();
     var userB = b.username.toLowerCase();
 
@@ -1211,40 +1196,40 @@ let FormLikeFactory = {
       formLike[prop] = aForm[prop];
     }
 
     this._addToJSONProperty(formLike);
     return formLike;
   },
 
   /**
-   * Create a FormLike object from an <input type=password>.
+   * Create a FormLike object from a password or username field.
    *
-   * If the <input> is in a <form>, construct the FormLike from the form.
+   * If the field is in a <form>, construct the FormLike from the form.
    * Otherwise, create a FormLike with a rootElement (wrapper) according to
    * heuristics. Currently all <input> not in a <form> are one FormLike but this
    * shouldn't be relied upon as the heuristics may change to detect multiple
    * "forms" (e.g. registration and login) on one page with a <form>.
    *
-   * @param {HTMLInputElement} aPasswordField - a password field in a document
+   * @param {HTMLInputElement} aField - a password or username field in a document
    * @return {FormLike}
-   * @throws Error if aPasswordField isn't a password input in a document
+   * @throws Error if aField isn't a password or username field in a document
    */
-  createFromPasswordField(aPasswordField) {
-    if (!(aPasswordField instanceof Ci.nsIDOMHTMLInputElement) ||
-        aPasswordField.type != "password" ||
-        !aPasswordField.ownerDocument) {
-      throw new Error("createFromPasswordField requires a password field in a document");
+  createFromField(aField) {
+    if (!(aField instanceof Ci.nsIDOMHTMLInputElement) ||
+        (aField.type != "password" && !LoginHelper.isUsernameFieldType(aField)) ||
+        !aField.ownerDocument) {
+      throw new Error("createFromField requires a password or username field in a document");
     }
 
-    if (aPasswordField.form) {
-      return this.createFromForm(aPasswordField.form);
+    if (aField.form) {
+      return this.createFromForm(aField.form);
     }
 
-    let doc = aPasswordField.ownerDocument;
+    let doc = aField.ownerDocument;
     log("Created non-form FormLike for rootElement:", doc.documentElement);
     let formLike = {
       action: LoginUtils._getPasswordOrigin(doc.baseURI),
       autocomplete: "on",
       // Exclude elements inside the rootElement that are already in a <form> as
       // they will be handled by their own FormLike.
       elements: [for (el of doc.documentElement.querySelectorAll("input")) if (!el.form) el],
       ownerDocument: doc,
--- a/toolkit/components/passwordmgr/test/test_formless_submit.html
+++ b/toolkit/components/passwordmgr/test/test_formless_submit.html
@@ -51,16 +51,26 @@ const TESTCASES = [
     formSubmitURL: DEFAULT_ORIGIN,
     usernameFieldValue: null,
     newPasswordFieldValue: "pass1",
     oldPasswordFieldValue: null,
   },
   {
     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,
   },
   {
@@ -131,17 +141,17 @@ let runTest = Task.async(function*() {
   let loginFrame = document.getElementById("loginFrame");
   let frameDoc = loginFrame.contentWindow.document;
 
   for (let tc of TESTCASES) {
     info("Starting testcase: " + JSON.stringify(tc));
     frameDoc.documentElement.innerHTML = tc.document;
     let inputForFormLike = frameDoc.querySelectorAll("input")[tc.inputIndexForFormLike];
 
-    let formLike = FormLikeFactory.createFromPasswordField(inputForFormLike);
+    let formLike = FormLikeFactory.createFromField(inputForFormLike);
 
     info("Calling _onFormSubmit with FormLike");
     let processedPromise = getSubmitMessage();
     LoginManagerContent._onFormSubmit(formLike);
 
     let submittedResult = yield processedPromise;
 
     // Check data sent via RemoteLogins:onFormSubmit
--- a/toolkit/components/passwordmgr/test/unit/test_getFormFields.js
+++ b/toolkit/components/passwordmgr/test/unit/test_getFormFields.js
@@ -13,16 +13,22 @@ const { LoginManagerContent, FormLikeFac
 const TESTCASES = [
   {
     description: "1 password field outside of a <form>",
     document: `<input id="pw1" type=password>`,
     returnedFieldIDs: [null, "pw1", null],
     skipEmptyFields: undefined,
   },
   {
+    description: "1 text field outside of a <form> without a password field",
+    document: `<input id="un1">`,
+    returnedFieldIDs: [null, null, null],
+    skipEmptyFields: undefined,
+  },
+  {
     description: "1 username & password field outside of a <form>",
     document: `<input id="un1">
       <input id="pw1" type=password>`,
     returnedFieldIDs: ["un1", "pw1", null],
     skipEmptyFields: undefined,
   },
   {
     description: "1 username & password field in a <form>",
@@ -65,16 +71,28 @@ const TESTCASES = [
   },
   {
     description: "1 password field in a form, 1 outside (not processed)",
     document: `<form><input id="pw1" type=password></form><input id="pw2" type=password>`,
     returnedFieldIDs: [null, "pw1", null],
     skipEmptyFields: undefined,
   },
   {
+    description: "1 password field in a form, 1 text field outside (not processed)",
+    document: `<form><input id="pw1" type=password></form><input>`,
+    returnedFieldIDs: [null, "pw1", null],
+    skipEmptyFields: undefined,
+  },
+  {
+    description: "1 text field in a form, 1 password field outside (not processed)",
+    document: `<form><input></form><input id="pw1" type=password>`,
+    returnedFieldIDs: [null, null, null],
+    skipEmptyFields: undefined,
+  },
+  {
     description: "2 password fields outside of a <form> with 1 linked via @form",
     document: `<input id="pw1" type=password><input id="pw2" type=password form='form1'>
       <form id="form1"></form>`,
     returnedFieldIDs: [null, "pw1", null],
     skipEmptyFields: undefined,
   },
   {
     description: "2 password fields outside of a <form> with 1 linked via @form + skipEmpty",
@@ -97,20 +115,20 @@ for (let tc of TESTCASES) {
 
   (function() {
     let testcase = tc;
     add_task(function*() {
       do_print("Starting testcase: " + testcase.description);
       let document = MockDocument.createTestDocument("http://localhost:8080/test/",
                                                       testcase.document);
 
-      let input = document.querySelector("input[type='password']");
+      let input = document.querySelector("input");
       MockDocument.mockOwnerDocumentProperty(input, document, "http://localhost:8080/test/");
 
-      let formLike = FormLikeFactory.createFromPasswordField(input);
+      let formLike = FormLikeFactory.createFromField(input);
 
       let actual = LoginManagerContent._getFormFields(formLike,
                                                       testcase.skipEmptyFields,
                                                       new Set());
 
       Assert.strictEqual(testcase.returnedFieldIDs.length, 3,
                          "_getFormFields returns 3 elements");
 
--- a/toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js
+++ b/toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js
@@ -11,17 +11,18 @@ const TESTCASES = [
     description: "Empty document",
     document: ``,
     returnedFieldIDsByFormLike: [],
     skipEmptyFields: undefined,
   },
   {
     description: "Non-password input with no <form> present",
     document: `<input>`,
-    returnedFieldIDsByFormLike: [],
+    // Only the IDs of password fields should be in this array
+    returnedFieldIDsByFormLike: [[]],
     skipEmptyFields: undefined,
   },
   {
     description: "1 password field outside of a <form>",
     document: `<input id="pw1" type=password>`,
     returnedFieldIDsByFormLike: [["pw1"]],
     skipEmptyFields: undefined,
   },
@@ -91,21 +92,17 @@ for (let tc of TESTCASES) {
     let testcase = tc;
     add_task(function*() {
       do_print("Starting testcase: " + testcase.description);
       let document = MockDocument.createTestDocument("http://localhost:8080/test/",
                                                      testcase.document);
 
       let mapRootElementToFormLike = new Map();
       for (let input of document.querySelectorAll("input")) {
-        if (input.type != "password") {
-          continue;
-        }
-
-        let formLike = FormLikeFactory.createFromPasswordField(input);
+        let formLike = FormLikeFactory.createFromField(input);
         let existingFormLike = mapRootElementToFormLike.get(formLike.rootElement);
         if (!existingFormLike) {
           mapRootElementToFormLike.set(formLike.rootElement, formLike);
           continue;
         }
 
         // If the formLike is already present, ensure that the properties are the same.
         do_print("Checking if the new FormLike for the same root has the same properties");