Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. r=seanlee, steveck
authorLuke Chang <lchang@mozilla.com>
Fri, 28 Jul 2017 19:08:30 +0800
changeset 421375 8cc5993a9104c4bea3fb476baf3cbe0bca9007f0
parent 421374 303b2be541c6c4411da0e5dfaac9a645944d0afd
child 421376 64e427f215f75bd6415e6e3de4f3f839854315f8
push id7664
push usermozilla@noorenberghe.ca
push dateThu, 24 Aug 2017 16:38:56 +0000
treeherdermozilla-beta@33a4e7ead964 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseanlee, steveck
bugs1385216
milestone56.0
Bug 1385216 - [Form Autofill] Avoid triggering update on fields that aren't changed after autofilling or contain concatenated street address. r=seanlee, steveck MozReview-Commit-ID: KI2ns7XDiHa
browser/extensions/formautofill/FormAutofillContent.jsm
browser/extensions/formautofill/FormAutofillHandler.jsm
browser/extensions/formautofill/FormAutofillParent.jsm
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
--- a/browser/extensions/formautofill/FormAutofillContent.jsm
+++ b/browser/extensions/formautofill/FormAutofillContent.jsm
@@ -380,39 +380,22 @@ var FormAutofillContent = {
     }
 
     let handler = this._formsDetails.get(formElement);
     if (!handler) {
       this.log.debug("Form element could not map to an existing handler");
       return true;
     }
 
-    let {addressRecord, creditCardRecord} = handler.createRecords();
-
-    if (!addressRecord && !creditCardRecord) {
+    let records = handler.createRecords();
+    if (!Object.keys(records).length) {
       return true;
     }
 
-    let data = {};
-    if (addressRecord) {
-      data.address = {
-        guid: handler.address.filledRecordGUID,
-        record: addressRecord,
-      };
-    }
-
-    if (creditCardRecord) {
-      data.creditCard = {
-        guid: handler.creditCard.filledRecordGUID,
-        record: creditCardRecord,
-      };
-    }
-
-    this._onFormSubmit(data, domWin);
-
+    this._onFormSubmit(records, domWin);
     return true;
   },
 
   receiveMessage({name, data}) {
     switch (name) {
       case "FormAutofill:enabledStatus": {
         if (data) {
           ProfileAutocomplete.ensureRegistered();
--- a/browser/extensions/formautofill/FormAutofillHandler.jsm
+++ b/browser/extensions/formautofill/FormAutofillHandler.jsm
@@ -446,49 +446,63 @@ FormAutofillHandler.prototype = {
 
     fieldDetail.state = nextState;
   },
 
   /**
    * Return the records that is converted from address/creditCard fieldDetails and
    * only valid form records are included.
    *
-   * @returns {Object} The new profile that convert from details with trimmed result.
+   * @returns {Object}
+   *          Consists of two record objects: address, creditCard. Each one can
+   *          be omitted if there's no valid fields. A record object consists of
+   *          three properties:
+   *            - guid: The id of the previously-filled profile or null if omitted.
+   *            - record: A valid record converted from details with trimmed result.
+   *            - untouchedFields: Fields that aren't touched after autofilling.
    */
   createRecords() {
-    let records = {};
+    let data = {};
 
     ["address", "creditCard"].forEach(type => {
       let details = this[type].fieldDetails;
       if (!details || details.length == 0) {
         return;
       }
 
-      let recordName = `${type}Record`;
-      records[recordName] = {};
+      data[type] = {
+        guid: this[type].filledRecordGUID,
+        record: {},
+        untouchedFields: [],
+      };
+
       details.forEach(detail => {
         let element = detail.elementWeakRef.get();
         // Remove the unnecessary spaces
         let value = element && element.value.trim();
         if (!value) {
           return;
         }
 
-        records[recordName][detail.fieldName] = value;
+        data[type].record[detail.fieldName] = value;
+
+        if (detail.state == "AUTO_FILLED") {
+          data[type].untouchedFields.push(detail.fieldName);
+        }
       });
     });
 
-    if (records.addressRecord &&
-        Object.keys(records.addressRecord).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
+    if (data.address &&
+        Object.keys(data.address.record).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
       log.debug("No address record saving since there are only",
-                     Object.keys(records.addressRecord).length,
+                     Object.keys(data.address.record).length,
                      "usable fields");
-      delete records.addressRecord;
+      delete data.address;
     }
 
-    if (records.creditCardRecord && !records.creditCardRecord["cc-number"]) {
+    if (data.creditCard && !data.creditCard.record["cc-number"]) {
       log.debug("No credit card record saving since card number is empty");
-      delete records.creditCardRecord;
+      delete data.creditCard;
     }
 
-    return records;
+    return data;
   },
 };
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -281,16 +281,24 @@ FormAutofillParent.prototype = {
                                         Services.ppmm.initialProcessData.autofillSavedFieldNames);
     this._updateStatus();
   },
 
   _onFormSubmit(data, target) {
     let {address} = data;
 
     if (address.guid) {
+      // Avoid updating the fields that users don't modify.
+      let originalAddress = this.profileStorage.addresses.get(address.guid);
+      for (let field in address.record) {
+        if (address.untouchedFields.includes(field) && originalAddress[field]) {
+          address.record[field] = originalAddress[field];
+        }
+      }
+
       if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
         FormAutofillDoorhanger.show(target, "update").then((state) => {
           let changedGUIDs = this.profileStorage.addresses.mergeToStorage(address.record);
           switch (state) {
             case "create":
               if (!changedGUIDs.length) {
                 changedGUIDs.push(this.profileStorage.addresses.add(address.record));
               }
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -1370,20 +1370,35 @@ class Addresses extends AutofillRecords 
       throw new Error("No matching address.");
     }
 
     let addressToMerge = this._clone(address);
     this._normalizeRecord(addressToMerge);
     let hasMatchingField = false;
 
     for (let field of this.VALID_FIELDS) {
-      if (addressToMerge[field] !== undefined && addressFound[field] !== undefined) {
-        if (addressToMerge[field] != addressFound[field]) {
-          this.log.debug("Conflicts: field", field, "has different value.");
-          return false;
+      let existingField = addressFound[field];
+      let incomingField = addressToMerge[field];
+      if (incomingField !== undefined && existingField !== undefined) {
+        if (incomingField != existingField) {
+          // Treat "street-address" as mergeable if their single-line versions
+          // match each other.
+          if (field == "street-address" &&
+              FormAutofillUtils.toOneLineAddress(existingField) == FormAutofillUtils.toOneLineAddress(incomingField)) {
+            // Keep the value in storage if its amount of lines is greater than
+            // or equal to the incoming one.
+            if (existingField.split("\n").length >= incomingField.split("\n").length) {
+              // Replace the incoming field with the one in storage so it will
+              // be further merged back to storage.
+              addressToMerge[field] = existingField;
+            }
+          } else {
+            this.log.debug("Conflicts: field", field, "has different value.");
+            return false;
+          }
         }
         hasMatchingField = true;
       }
     }
 
     // We merge the address only when at least one field has the same value.
     if (!hasMatchingField) {
       this.log.debug("Unable to merge because no field has the same value");
--- a/browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
+++ b/browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
@@ -14,17 +14,17 @@ add_task(async function test_update_addr
       await openPopupOn(browser, "form #organization");
       await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
       await BrowserTestUtils.synthesizeKey("VK_RETURN", {}, browser);
 
       await ContentTask.spawn(browser, null, async function() {
         let form = content.document.getElementById("form");
         let org = form.querySelector("#organization");
         await new Promise(resolve => setTimeout(resolve, 1000));
-        org.value = "Mozilla";
+        org.setUserInput("Mozilla");
 
         // Wait 1000ms before submission to make sure the input value applied
         await new Promise(resolve => setTimeout(resolve, 1000));
         form.querySelector("input[type=submit]").click();
       });
 
       await promiseShown;
       await clickDoorhangerButton(MAIN_BUTTON_INDEX);
@@ -47,31 +47,31 @@ add_task(async function test_create_new_
       await openPopupOn(browser, "form #tel");
       await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
       await BrowserTestUtils.synthesizeKey("VK_RETURN", {}, browser);
 
       await ContentTask.spawn(browser, null, async function() {
         let form = content.document.getElementById("form");
         let tel = form.querySelector("#tel");
         await new Promise(resolve => setTimeout(resolve, 1000));
-        tel.value = "+1-234-567-890";
+        tel.setUserInput("+1234567890");
 
         // Wait 1000ms before submission to make sure the input value applied
         await new Promise(resolve => setTimeout(resolve, 1000));
         form.querySelector("input[type=submit]").click();
       });
 
       await promiseShown;
       await clickDoorhangerButton(SECONDARY_BUTTON_INDEX);
     }
   );
 
   addresses = await getAddresses();
   is(addresses.length, 2, "2 addresses in storage");
-  is(addresses[1].tel, "+1-234-567-890", "Verify the tel field");
+  is(addresses[1].tel, "+1234567890", "Verify the tel field");
 });
 
 add_task(async function test_create_new_address_merge() {
   let addresses = await getAddresses();
   is(addresses.length, 2, "2 addresses in storage");
 
   await BrowserTestUtils.withNewTab({gBrowser, url: FORM_URL},
     async function(browser) {
@@ -80,23 +80,61 @@ add_task(async function test_create_new_
       await openPopupOn(browser, "form #tel");
       await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
       await BrowserTestUtils.synthesizeKey("VK_RETURN", {}, browser);
 
       // Choose the latest address and revert to the original phone number
       await ContentTask.spawn(browser, null, async function() {
         let form = content.document.getElementById("form");
         let tel = form.querySelector("#tel");
-        tel.value = "+1 617 253 5702";
+        tel.setUserInput("+16172535702");
 
         // Wait 1000ms before submission to make sure the input value applied
         await new Promise(resolve => setTimeout(resolve, 1000));
         form.querySelector("input[type=submit]").click();
       });
 
       await promiseShown;
       await clickDoorhangerButton(SECONDARY_BUTTON_INDEX);
     }
   );
 
   addresses = await getAddresses();
   is(addresses.length, 2, "Still 2 addresses in storage");
 });
+
+add_task(async function test_submit_untouched_fields() {
+  let addresses = await getAddresses();
+  is(addresses.length, 2, "2 addresses in storage");
+
+  await BrowserTestUtils.withNewTab({gBrowser, url: FORM_URL},
+    async function(browser) {
+      let promiseShown = BrowserTestUtils.waitForEvent(PopupNotifications.panel,
+                                                       "popupshown");
+      await openPopupOn(browser, "form #organization");
+      await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
+      await BrowserTestUtils.synthesizeKey("VK_RETURN", {}, browser);
+
+      await ContentTask.spawn(browser, null, async function() {
+        let form = content.document.getElementById("form");
+        let org = form.querySelector("#organization");
+        await new Promise(resolve => setTimeout(resolve, 1000));
+        org.setUserInput("Organization");
+
+        let tel = form.querySelector("#tel");
+        await new Promise(resolve => setTimeout(resolve, 1000));
+        tel.value = "12345"; // ".value" won't change the highlight status.
+
+        // Wait 1000ms before submission to make sure the input value applied
+        await new Promise(resolve => setTimeout(resolve, 1000));
+        form.querySelector("input[type=submit]").click();
+      });
+
+      await promiseShown;
+      await clickDoorhangerButton(MAIN_BUTTON_INDEX);
+    }
+  );
+
+  addresses = await getAddresses();
+  is(addresses.length, 2, "Still 2 addresses in storage");
+  is(addresses[0].organization, "Organization", "organization should change");
+  is(addresses[0].tel, "+16172535702", "tel should remain unchanged");
+});
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -97,16 +97,111 @@ const MERGE_TESTCASES = [
     },
     expectedAddress: {
       "given-name": "Timothy",
       "street-address": "331 E. Evelyn Avenue",
       "tel": "+16509030800",
       country: "US",
     },
   },
+  {
+    description: "Merge an address with multi-line street-address in storage and single-line incoming one",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2",
+      "tel": "+16509030800",
+    },
+    addressToMerge: {
+      "street-address": "331 E. Evelyn Avenue Line2",
+      "tel": "+16509030800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2",
+      "tel": "+16509030800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge an address with 3-line street-address in storage and 2-line incoming one",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2\nLine3",
+      "tel": "+16509030800",
+    },
+    addressToMerge: {
+      "street-address": "331 E. Evelyn Avenue\nLine2 Line3",
+      "tel": "+16509030800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2\nLine3",
+      "tel": "+16509030800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge an address with single-line street-address in storage and multi-line incoming one",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue Line2",
+      "tel": "+16509030800",
+    },
+    addressToMerge: {
+      "street-address": "331 E. Evelyn Avenue\nLine2",
+      "tel": "+16509030800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2",
+      "tel": "+16509030800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge an address with 2-line street-address in storage and 3-line incoming one",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2 Line3",
+      "tel": "+16509030800",
+    },
+    addressToMerge: {
+      "street-address": "331 E. Evelyn Avenue\nLine2\nLine3",
+      "tel": "+16509030800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2\nLine3",
+      "tel": "+16509030800",
+      country: "US",
+    },
+  },
+  {
+    description: "Merge an address with the same amount of lines",
+    addressInStorage: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2\nLine3",
+      "tel": "+16509030800",
+    },
+    addressToMerge: {
+      "street-address": "331 E. Evelyn\nAvenue Line2\nLine3",
+      "tel": "+16509030800",
+      country: "US",
+    },
+    expectedAddress: {
+      "given-name": "Timothy",
+      "street-address": "331 E. Evelyn Avenue\nLine2\nLine3",
+      "tel": "+16509030800",
+      country: "US",
+    },
+  },
 ];
 
 let do_check_record_matches = (recordWithMeta, record) => {
   for (let key in record) {
     do_check_eq(recordWithMeta[key], record[key]);
   }
 };
 
--- a/browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
+++ b/browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
@@ -51,16 +51,17 @@ const TESTCASES = [
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Trigger credit card saving",
     formValue: {
       "cc-name": "John Doe",
@@ -74,16 +75,17 @@ const TESTCASES = [
         creditCard: {
           guid: null,
           record: {
             "cc-name": "John Doe",
             "cc-number": "1234567812345678",
             "cc-exp-month": 12,
             "cc-exp-year": 2000,
           },
+          untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Trigger address and credit card saving",
     formValue: {
       "street-addr": "331 E. Evelyn Avenue",
@@ -99,25 +101,27 @@ const TESTCASES = [
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
         creditCard: {
           guid: null,
           record: {
             "cc-name": "John Doe",
             "cc-number": "1234567812345678",
             "cc-exp-month": 12,
             "cc-exp-year": 2000,
           },
+          untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Profile saved with trimmed string",
     formValue: {
       "street-addr": "331 E. Evelyn Avenue  ",
@@ -129,16 +133,17 @@ const TESTCASES = [
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Eliminate the field that is empty after trimmed",
     formValue: {
       "street-addr": "331 E. Evelyn Avenue",
@@ -151,16 +156,17 @@ const TESTCASES = [
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
       },
     },
   },
 ];
 
 add_task(async function handle_earlyformsubmit_event() {
   do_print("Starting testcase: Test an invalid form element");