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

--HG--
extra : rebase_source : cf7fd4b5e7f8ef89c9a37c84600a7b7a12574f72

browser/extensions/formautofill/FormAutofillContent.jsm

@@ -385,29 +385,12 @@ var FormAutofillContent = {
       return true;
     }
 
-    let {addressRecord, creditCardRecord} = handler.createRecords();
+    let records = handler.createRecords();
-
+    if (!Object.keys(records).length) {
-    if (!addressRecord && !creditCardRecord) {
       return true;
     }
 
-    let data = {};
+    this._onFormSubmit(records, domWin);
-    if (addressRecord) {
-      data.address = {
-        guid: handler.address.filledRecordGUID,
-        record: addressRecord,
-      };
-    }
-
-    if (creditCardRecord) {
-      data.creditCard = {
-        guid: handler.creditCard.filledRecordGUID,
-        record: creditCardRecord,
-      };
-    }
-
-    this._onFormSubmit(data, domWin);
-
     return true;
   },
 

browser/extensions/formautofill/FormAutofillHandler.jsm

@@ -451,10 +451,16 @@ FormAutofillHandler.prototype = {
    * 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;
@@ -462,8 +468,12 @@ FormAutofillHandler.prototype = {
         return;
       }
 
-      let recordName = `${type}Record`;
+      data[type] = {
-      records[recordName] = {};
+        guid: this[type].filledRecordGUID,
+        record: {},
+        untouchedFields: [],
+      };
+
       details.forEach(detail => {
         let element = detail.elementWeakRef.get();
         // Remove the unnecessary spaces
@@ -472,23 +482,27 @@ FormAutofillHandler.prototype = {
           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 &&
+    if (data.address &&
-        Object.keys(records.addressRecord).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
+        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;
   },
 };

browser/extensions/formautofill/FormAutofillParent.jsm

@@ -286,6 +286,14 @@ FormAutofillParent.prototype = {
     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);

browser/extensions/formautofill/ProfileStorage.jsm

@@ -1375,10 +1375,25 @@ class Addresses extends AutofillRecords {
     let hasMatchingField = false;
 
     for (let field of this.VALID_FIELDS) {
-      if (addressToMerge[field] !== undefined && addressFound[field] !== undefined) {
+      let existingField = addressFound[field];
-        if (addressToMerge[field] != addressFound[field]) {
+      let incomingField = addressToMerge[field];
-          this.log.debug("Conflicts: field", field, "has different value.");
+      if (incomingField !== undefined && existingField !== undefined) {
-          return false;
+        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;
       }

browser/extensions/formautofill/test/browser/browser_update_doorhanger.js

@@ -19,7 +19,7 @@ add_task(async function test_update_address() {
         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));
@@ -52,7 +52,7 @@ add_task(async function test_create_new_address() {
         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));
@@ -66,7 +66,7 @@ add_task(async function test_create_new_address() {
 
   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() {
@@ -85,7 +85,7 @@ add_task(async function test_create_new_address_merge() {
       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));
@@ -100,3 +100,41 @@ add_task(async function test_create_new_address_merge() {
   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");
+});

browser/extensions/formautofill/test/unit/test_addressRecords.js

@@ -102,6 +102,101 @@ const MERGE_TESTCASES = [
       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) => {

browser/extensions/formautofill/test/unit/test_onFormSubmitted.js

@@ -56,6 +56,7 @@ const TESTCASES = [
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
       },
     },
@@ -79,6 +80,7 @@ const TESTCASES = [
             "cc-exp-month": 12,
             "cc-exp-year": 2000,
           },
+          untouchedFields: [],
         },
       },
     },
@@ -104,6 +106,7 @@ const TESTCASES = [
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
         creditCard: {
           guid: null,
@@ -113,6 +116,7 @@ const TESTCASES = [
             "cc-exp-month": 12,
             "cc-exp-year": 2000,
           },
+          untouchedFields: [],
         },
       },
     },
@@ -134,6 +138,7 @@ const TESTCASES = [
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
       },
     },
@@ -156,6 +161,7 @@ const TESTCASES = [
             "country": "USA",
             "tel": "1-650-903-0800",
           },
+          untouchedFields: [],
         },
       },
     },