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 373635 c7ca671926a6e9f147a85158c51fdd1cbf3df9e1
parent 373634 abe5a138efd6fa732b3d5a0054e965ca048f32dc
child 373636 c4376773f9cd977c8c450f71ae33e937ed4fd026
push id48354
push userlchang@mozilla.com
push dateWed, 09 Aug 2017 16:26:29 +0000
treeherderautoland@c7ca671926a6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseanlee, steveck
bugs1385216
milestone57.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 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");