Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. r=lchang
authorsteveck-chung <schung@mozilla.com>
Fri, 01 Sep 2017 10:11:19 +0800
changeset 428513 d281f0c4644ec66af41d50825a830d14c543b603
parent 428512 34d6906c255925a1b1e3a223b2b9f804f4ebf590
child 428514 78cce071b69e26eb55df4da1f66409b3a2226d3b
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslchang
bugs1395519
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 1395519 - [Form Autofill] Keep the original data when record updated via submission. r=lchang 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
@@ -531,37 +531,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"] ||
         !FormAutofillUtils.isCCNumber(data.creditCard.record["cc-number"]))) {
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -365,17 +365,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
@@ -369,36 +369,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_creditCard_doorhanger.js]
 [browser_dropdown_layout.js]
--- a/browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
+++ b/browser/extensions/formautofill/test/browser/browser_update_doorhanger.js
@@ -132,8 +132,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",
@@ -317,16 +324,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: [],
         },
       },
     },
   },
 
 ];