Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. r=lchang, a=lizzard
authorsteveck-chung <schung@mozilla.com>
Fri, 01 Sep 2017 10:11:19 +0800
changeset 423980 5df6d9e6c8ae7d43f888cf36cd1e23da4a8fb347
parent 423979 1878b936762cbfad0b7586d5d3db699ad02f28d9
child 423981 12cab10e453ed169bced4af91fb1a17357ffb14e
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslchang, lizzard
bugs1395519
milestone56.0
Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. r=lchang, a=lizzard MozReview-Commit-ID: DkVgOlTqVhH
browser/extensions/formautofill/FormAutofillHandler.jsm
browser/extensions/formautofill/FormAutofillParent.jsm
browser/extensions/formautofill/ProfileStorage.jsm
browser/extensions/formautofill/test/browser/browser.ini
browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
browser/extensions/formautofill/test/fixtures/autocomplete_simple_basic.html
browser/extensions/formautofill/test/unit/test_addressRecords.js
browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
--- a/browser/extensions/formautofill/FormAutofillHandler.jsm
+++ b/browser/extensions/formautofill/FormAutofillHandler.jsm
@@ -485,37 +485,42 @@ FormAutofillHandler.prototype = {
         // Try to abbreviate the value of select element.
         if (type == "address" &&
             detail.fieldName == "address-level1" &&
             element instanceof Ci.nsIDOMHTMLSelectElement) {
           // Don't save the record when the option value is empty *OR* there
           // are multiple options being selected. The empty option is usually
           // assumed to be default along with a meaningless text to users.
           if (!value || element.selectedOptions.length != 1) {
+            // Keep the property and preserve more information for address updating
+            data[type].record[detail.fieldName] = "";
             return;
           }
 
           let text = element.selectedOptions[0].text.trim();
           value = FormAutofillUtils.getAbbreviatedStateName([value, text]) || text;
         }
 
         if (!value) {
+          // Keep the property and preserve more information for updating
+          data[type].record[detail.fieldName] = "";
           return;
         }
 
         data[type].record[detail.fieldName] = value;
 
         if (detail.state == "AUTO_FILLED") {
           data[type].untouchedFields.push(detail.fieldName);
         }
       });
     });
 
     if (data.address &&
-        Object.keys(data.address.record).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
+        Object.values(data.address.record).filter(v => v).length <
+        FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) {
       log.debug("No address record saving since there are only",
                      Object.keys(data.address.record).length,
                      "usable fields");
       delete data.address;
     }
 
     if (data.creditCard && !data.creditCard.record["cc-number"]) {
       log.debug("No credit card record saving since card number is empty");
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -307,17 +307,17 @@ FormAutofillParent.prototype = {
           switch (state) {
             case "create":
               if (!changedGUIDs.length) {
                 changedGUIDs.push(this.profileStorage.addresses.add(address.record));
               }
               break;
             case "update":
               if (!changedGUIDs.length) {
-                this.profileStorage.addresses.update(address.guid, address.record);
+                this.profileStorage.addresses.update(address.guid, address.record, true);
                 changedGUIDs.push(address.guid);
               } else {
                 this.profileStorage.addresses.remove(address.guid);
               }
               break;
           }
           changedGUIDs.forEach(guid => this.profileStorage.addresses.notifyUsed(guid));
         });
--- a/browser/extensions/formautofill/ProfileStorage.jsm
+++ b/browser/extensions/formautofill/ProfileStorage.jsm
@@ -367,36 +367,44 @@ class AutofillRecords {
 
   /**
    * Update the specified record.
    *
    * @param  {string} guid
    *         Indicates which record to update.
    * @param  {Object} record
    *         The new record used to overwrite the old one.
+   * @param  {boolean} [preserveOldProperties = false]
+   *         Preserve old record's properties if they don't exist in new record.
    */
-  update(guid, record) {
+  update(guid, record, preserveOldProperties = false) {
     this.log.debug("update:", guid, record);
 
     let recordFound = this._findByGUID(guid);
     if (!recordFound) {
       throw new Error("No matching record.");
     }
 
-    let recordToUpdate = this._clone(record);
+    // Clone the record by Object assign API to preserve the property with empty string.
+    let recordToUpdate = Object.assign({}, record);
     this._normalizeRecord(recordToUpdate);
 
     for (let field of this.VALID_FIELDS) {
       let oldValue = recordFound[field];
       let newValue = recordToUpdate[field];
 
-      if (newValue != null) {
+      // Resume the old field value in the perserve case
+      if (preserveOldProperties && newValue === undefined) {
+        newValue = oldValue;
+      }
+
+      if (!newValue) {
+        delete recordFound[field];
+      } else {
         recordFound[field] = newValue;
-      } else {
-        delete recordFound[field];
       }
 
       this._maybeStoreLastSyncedField(recordFound, field, oldValue);
     }
 
     recordFound.timeLastModified = Date.now();
     let syncMetadata = this._getSyncMetaData(recordFound);
     if (syncMetadata) {
--- a/browser/extensions/formautofill/test/browser/browser.ini
+++ b/browser/extensions/formautofill/test/browser/browser.ini
@@ -1,13 +1,14 @@
 [DEFAULT]
 head = head.js
 
 support-files =
   ../fixtures/autocomplete_basic.html
+  ../fixtures/autocomplete_simple_basic.html
   ../fixtures/autocomplete_creditcard_basic.html
 
 [browser_autocomplete_footer.js]
 [browser_autocomplete_marked_back_forward.js]
 [browser_autocomplete_marked_detached_tab.js]
 [browser_check_installed.js]
 [browser_editAddressDialog.js]
 [browser_first_time_use_doorhanger.js]
--- a/browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
+++ b/browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
@@ -131,8 +131,43 @@ add_task(async function test_submit_unto
     }
   );
 
   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");
 });
+
+add_task(async function test_submit_reduced_fields() {
+  let addresses = await getAddresses();
+  is(addresses.length, 2, "2 addresses in storage");
+
+  let url = BASE_URL + "autocomplete_simple_basic.html";
+  await BrowserTestUtils.withNewTab({gBrowser, url},
+    async function(browser) {
+      let promiseShown = BrowserTestUtils.waitForEvent(PopupNotifications.panel,
+                                                       "popupshown");
+      await openPopupOn(browser, "form#simple input[name=tel]");
+      await BrowserTestUtils.synthesizeKey("VK_DOWN", {}, browser);
+      await BrowserTestUtils.synthesizeKey("VK_RETURN", {}, browser);
+
+      await ContentTask.spawn(browser, null, async function() {
+        let form = content.document.querySelector("form#simple");
+        let tel = form.querySelector("input[name=tel]");
+        await new Promise(resolve => setTimeout(resolve, 1000));
+        tel.setUserInput("123456789");
+
+        // 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);
+    }
+  );
+
+  addresses = await getAddresses();
+  is(addresses.length, 2, "Still 2 addresses in storage");
+  is(addresses[0].tel, "123456789", "tel should should be changed");
+  is(addresses[0]["postal-code"], "02139", "postal code should be kept");
+});
new file mode 100644
--- /dev/null
+++ b/browser/extensions/formautofill/test/fixtures/autocomplete_simple_basic.html
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Form Autofill Demo Page for Simplified Form Case</title>
+</head>
+<body>
+  <h1>Form Autofill Demo Page for Simplified Form Case</h1>
+
+  <form id="simple">
+    <p><label>Organization: <input type="text" /></label></p>
+    <p><label>Telephone: <input id="simple_tel" name="tel" /></label></p>
+    <p><label>Email: <input type="text" id="simple_email" name="email" /></label></p>
+    <p><input type="submit" /></p>
+    <p><button type="reset">Reset</button></p>
+  </form>
+
+</body>
+</html>
--- a/browser/extensions/formautofill/test/unit/test_addressRecords.js
+++ b/browser/extensions/formautofill/test/unit/test_addressRecords.js
@@ -21,27 +21,34 @@ const TEST_ADDRESS_1 = {
 };
 
 const TEST_ADDRESS_2 = {
   "street-address": "Some Address",
   country: "US",
 };
 
 const TEST_ADDRESS_3 = {
+  "given-name": "Timothy",
+  "family-name": "Berners-Lee",
   "street-address": "Other Address",
   "postal-code": "12345",
 };
 
 const TEST_ADDRESS_4 = {
   "given-name": "Timothy",
   "additional-name": "John",
   "family-name": "Berners-Lee",
   organization: "World Wide Web Consortium",
 };
 
+const TEST_ADDRESS_FOR_UPDATE = {
+  "name": "Tim Berners",
+  "street-address": "",
+};
+
 const TEST_ADDRESS_WITH_INVALID_FIELD = {
   "street-address": "Another Address",
   invalidField: "INVALID",
 };
 
 const MERGE_TESTCASES = [
   {
     description: "Merge a superset",
@@ -351,16 +358,32 @@ add_task(async function test_update() {
 
   let address = profileStorage.addresses.get(guid, {rawData: true});
 
   do_check_eq(address.country, undefined);
   do_check_neq(address.timeLastModified, timeLastModified);
   do_check_record_matches(address, TEST_ADDRESS_3);
   do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 1);
 
+  // Test preserveOldProperties parameter and field with empty string.
+  profileStorage.addresses.update(guid, TEST_ADDRESS_FOR_UPDATE, true);
+  await onChanged;
+  await profileStorage._saveImmediately();
+
+  profileStorage.addresses.pullSyncChanges(); // force sync metadata, which we check below.
+
+  address = profileStorage.addresses.get(guid, {rawData: true});
+
+  do_check_eq(address["given-name"], "Tim");
+  do_check_eq(address["family-name"], "Berners");
+  do_check_eq(address["street-address"], undefined);
+  do_check_eq(address["postal-code"], "12345");
+  do_check_neq(address.timeLastModified, timeLastModified);
+  do_check_eq(getSyncChangeCounter(profileStorage.addresses, guid), 2);
+
   Assert.throws(
     () => profileStorage.addresses.update("INVALID_GUID", TEST_ADDRESS_3),
     /No matching record\./
   );
 
   Assert.throws(
     () => profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_INVALID_FIELD),
     /"invalidField" is not a valid field\./
--- a/browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
+++ b/browser/extensions/formautofill/test/unit/test_onFormSubmitted.js
@@ -61,17 +61,20 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "",
+            "address-level2": "",
             "country": "USA",
+            "email": "",
             "tel": "1-650-903-0800",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
@@ -111,17 +114,20 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "",
+            "address-level2": "",
             "country": "USA",
+            "email": "",
             "tel": "1-650-903-0800",
           },
           untouchedFields: [],
         },
         creditCard: {
           guid: null,
           record: {
             "cc-name": "John Doe",
@@ -143,17 +149,20 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "",
+            "address-level2": "",
             "country": "USA",
+            "email": "",
             "tel": "1-650-903-0800",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
@@ -166,17 +175,20 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "",
+            "address-level2": "",
             "country": "USA",
+            "email": "",
             "tel": "1-650-903-0800",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
@@ -188,18 +200,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "address-level1": "CA",
+            "address-level2": "",
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
+            "email": "",
+            "tel": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Save state with lowercase value",
@@ -210,18 +225,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "address-level1": "CA",
+            "address-level2": "",
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
+            "email": "",
+            "tel": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Save state with a country code prefixed to the label",
@@ -232,18 +250,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "address-level1": "AR",
+            "address-level2": "",
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
+            "email": "",
+            "tel": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Save state with a country code prefixed to the value",
@@ -254,18 +275,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "address-level1": "CA",
+            "address-level2": "",
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
+            "email": "",
+            "tel": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Save state with a country code prefixed to the value and label",
@@ -276,18 +300,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "address-level1": "AZ",
+            "address-level2": "",
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
+            "email": "",
+            "tel": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Should save select label instead when failed to abbreviate the value",
@@ -298,18 +325,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "address-level1": "Arizonac",
+            "address-level2": "",
             "street-address": "331 E. Evelyn Avenue",
             "country": "USA",
+            "email": "",
+            "tel": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Shouldn't save select with multiple selections",
@@ -321,18 +351,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "",
+            "address-level2": "",
             "country": "USA",
             "tel": "1-650-903-0800",
+            "email": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
   {
     description: "Shouldn't save select with empty value",
@@ -344,18 +377,21 @@ const TESTCASES = [
     },
     expectedResult: {
       formSubmission: true,
       records: {
         address: {
           guid: null,
           record: {
             "street-address": "331 E. Evelyn Avenue",
+            "address-level1": "",
+            "address-level2": "",
             "country": "USA",
             "tel": "1-650-903-0800",
+            "email": "",
           },
           untouchedFields: [],
         },
       },
     },
   },
 
 ];