Closed
Bug 1395519
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Data Loss in Saved Addresses when submitting to update from a form
Categories
(Toolkit :: Form Manager, defect)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | verified |
People
(Reporter: Gabi, Assigned: steveck)
Details
(Whiteboard: [form autofill:MVP])
Attachments
(3 files)
706 bytes,
text/html
|
Details | |
615.84 KB,
image/gif
|
Details | |
59 bytes,
text/x-review-board-request
|
lchang
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
[Affected versions]: - beta 56.0b7 - latest Nightly 57.0a1 [Affected platforms]: - Windows 10 x64, macOS 10.12.6, Ubuntu 14.04 x64 [Steps to reproduce]: 1. Modify extensions.formautofill.available to "on" in about:config (if necessary) 2. In about:preferences#privacy click on 'Saved Addresses' 3. Create a new profile 4. Access the attached .html 5. Double click on any of the fields to trigger autofill 6. Select the profile to fill in fields 7. Modify a field (e.g change city name) 8. Submit form and tap Update on the door hanger popup 9. Verify the updated profile in Saved Addresses [Expected result]: - All the data entered in Saved address profile is deleted and only the fields submitted by demo form is displayed [Actual result]: - Saved Addresses profile should be updated and the rest of the fields should be displayed in users profile
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
[Regression range]: This is not regression, issue is seen since the door -hanger popup was implemented Updated STR: [Expected result]: - Saved Addresses profile should be updated and the rest of the fields should be displayed in users profile [Actual result]: - All the data entered in Saved address profile is deleted and only the fields submitted by demo form is displayed
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
Updated•7 years ago
|
Whiteboard: [form autofill]
Whiteboard: [form autofill] → [form autofill:MVP]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → schung
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review180280 The patch itself looks good but it causes an autofilled but manually-cleared field unable to be cleared in the storage accordingly via update doorhanger. We probably need to consult UX about this. ::: browser/extensions/formautofill/FormAutofillParent.jsm:381 (Diff revision 1) > break; > case "update": > if (!changedGUIDs.length) { > + // Refill the fields if they are not in the new record. > + for (let field in originalAddress) { > + if (FormAutofillUtils.isAddressField(field) && I don't think we need to check `isAddressField`. Fields in `originalAddress` should be all valid address fields.
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review180280 That's a good point. I have another idea is that we cache the "non_autofilled" fields instead. If the original filed exists and it's not in the "non_autofilled" arrary, we should reset the new record with the original field data. wdyt?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review180320 ::: browser/extensions/formautofill/FormAutofillParent.jsm:363 (Diff revision 2) > _onAddressSubmit(address, target) { > 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]) { > + for (let field in originalAddress) { > + if (!this.profileStorage.addresses.VALID_FIELDS.includes(field)) { It's the same like I did in the previous patch(checking isAddressField) is because the original record will contain some interal fields like guid and it will cause the error while updating the record. I use `VALID_FIELDS` instead in this patch since `isAddressField` might not clear enough. ::: browser/extensions/formautofill/test/fixtures/autocomplete_simple_basic.html:1 (Diff revision 2) > +<!DOCTYPE html> I created another `autocomplete_simple_basic.html` instead of adding another form in `autocomplete_basic.html` because heuristic unit test will rely on the `autocomplete_basic.html` structure.
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review180280 > I don't think we need to check `isAddressField`. Fields in `originalAddress` should be all valid address fields. Explained in another comment
Comment 9•7 years ago
|
||
(In reply to Steve Chung [:steveck] from comment #5) > That's a good point. I have another idea is that we cache the > "non_autofilled" fields instead. If the original filed exists and it's not > in the "non_autofilled" arrary, we should reset the new record with the > original field data. wdyt? Doesn't it cause fields which are in the original record but not in the incoming record to also appear in the new created one in "Create New" case?
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Luke Chang [:lchang] from comment #9) > (In reply to Steve Chung [:steveck] from comment #5) > > That's a good point. I have another idea is that we cache the > > "non_autofilled" fields instead. If the original filed exists and it's not > > in the "non_autofilled" arrary, we should reset the new record with the > > original field data. wdyt? > > Doesn't it cause fields which are in the original record but not in the > incoming record to also appear in the new created one in "Create New" case? Ok, I can't think of any other better solution than adding another array to cache all existing details...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review180384 ::: browser/extensions/formautofill/FormAutofillHandler.jsm:525 (Diff revision 3) > > data[type] = { > guid: this[type].filledRecordGUID, > record: {}, > untouchedFields: [], > + allFields: this.allFieldNames, Not sure if it's ok to add `allFields` for both records because this will have both creditcard and address fields. Maybe move it to upper level or split it into cc/address fields?
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review180936 As we discussed, we need to handle the case that only computed fields are modified. Please also add some test cases for that. Thanks.
Attachment #8903433 -
Flags: review?(lchang)
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review181178 Remember to add a unit test to ensure that changed computed fields DO overwrite its source field if the latter is not present in the content page. ::: browser/extensions/formautofill/FormAutofillHandler.jsm:539 (Diff revision 5) > 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) { > + // We will keep the property and preserve more information for address updating nit: delete "We will". ::: browser/extensions/formautofill/FormAutofillHandler.jsm:549 (Diff revision 5) > let text = element.selectedOptions[0].text.trim(); > value = FormAutofillUtils.getAbbreviatedStateName([value, text]) || text; > } > > if (!value) { > + // We will keep the property and preserve more information for updating nit: ditto. ::: browser/extensions/formautofill/FormAutofillHandler.jsm:562 (Diff revision 5) > data[type].untouchedFields.push(detail.fieldName); > } > }); > }); > > - if (data.address && > + let addrFieldNumber = !data.address ? 0 : nit: `validAddressFieldAmount` ::: browser/extensions/formautofill/FormAutofillHandler.jsm:563 (Diff revision 5) > } > }); > }); > > - if (data.address && > - Object.keys(data.address.record).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) { > + let addrFieldNumber = !data.address ? 0 : > + this.address.fieldDetails.filter( nit: `Object.values(data.address.record).filter(f => f).length` ::: browser/extensions/formautofill/FormAutofillHandler.jsm:566 (Diff revision 5) > > - if (data.address && > - Object.keys(data.address.record).length < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) { > + let addrFieldNumber = !data.address ? 0 : > + this.address.fieldDetails.filter( > + detail => data.address.record[detail.fieldName] > + ).length; > + if (data.address && addrFieldNumber < FormAutofillUtils.AUTOFILL_FIELDS_THRESHOLD) { No need to check `data.address` since it'll be set to 0 in the previous line. ::: browser/extensions/formautofill/ProfileStorage.jsm:377 (Diff revision 5) > + * @param {boolean} preserveOldProperties > + * Preserve old record's properties if they are not existed in new record. nit: `{boolean} [preserveOldProperties=false]` and `... they don't exist in ...` ::: browser/extensions/formautofill/ProfileStorage.jsm:380 (Diff revision 5) > * @param {Object} record > * The new record used to overwrite the old one. > + * @param {boolean} preserveOldProperties > + * Preserve old record's properties if they are not existed in new record. > */ > - update(guid, record) { > + update(guid, record, preserveOldProperties) { nit: Use `preserveOldProperties = false` to explicitly set a default value. ::: browser/extensions/formautofill/ProfileStorage.jsm:397 (Diff revision 5) > for (let field of this.VALID_FIELDS) { > let oldValue = recordFound[field]; > let newValue = recordToUpdate[field]; > > - if (newValue != null) { > - recordFound[field] = newValue; > + // Resume the old field value in the perserve case > + if (preserveOldProperties && newValue == undefined) { use `===`.
Attachment #8903433 -
Flags: review?(lchang) → review+
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review181178 > No need to check `data.address` since it'll be set to 0 in the previous line. The reason I still check the `data.address` here because I don't want showing the misleading debug message about the insufficient field number for no data.address case
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. https://reviewboard.mozilla.org/r/175280/#review181178 > The reason I still check the `data.address` here because I don't want showing the misleading debug message about the insufficient field number for no data.address case I'll remove `validAddressFieldAmount` to avoid this concern
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 21•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d281f0c4644e [Form Autofill] Keep the original data when record updated via submission. r=lchang
Keywords: checkin-needed
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d281f0c4644e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 23•7 years ago
|
||
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. Approval Request Comment [Feature/Bug causing the regression]:Not a regression, this issue existed from the day feature landed. [User impact if declined]: Data might be lost while users updated their profile during form submission. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Not yet but soon [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: Low [Why is the change risky/not risky?]:It only affects the original profile if the submitted profile is changed, and it should be no chance to lose data after this patch. [String changes made/needed]: N/A
Attachment #8903433 -
Flags: approval-mozilla-beta?
Comment 24•7 years ago
|
||
Hi Gabi, Can you help check if this issue was fixed as expected in the latest nightly? Thanks.
Flags: needinfo?(gasofie)
Reporter | ||
Comment 25•7 years ago
|
||
Verified issue as fixed using the latest nightly 57.0a1 20170907100318 on Ubuntu 16.04, Windows 10x64 and MacOS 10.12.6
Flags: needinfo?(gasofie)
Reporter | ||
Updated•7 years ago
|
Comment 26•7 years ago
|
||
Comment on attachment 8903433 [details] Bug 1395519 - [Form Autofill] Keep the original data when record updated via submission. We're shipping this feature in 56 and the fix is verified. OK to uplift for beta 10.
Attachment #8903433 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 27•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5df6d9e6c8ae
Flags: in-testsuite+
Comment 28•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/12cab10e453e
Whiteboard: [form autofill:MVP] → [form autofill:MVP]
You need to log in
before you can comment on or make changes to this bug.
Description
•