[Form Autofill] Display the updating door hanger to let users save a new address or update choosen address

VERIFIED FIXED in Firefox 56

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: lchang, Assigned: steveck)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [form autofill:M3])

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Clicking on drop down key will show up "create it as a new profile". Clicking on it will save the current profile as a new profile.
QA Whiteboard: [form autofill] [form autofill:M1]
(Reporter)

Updated

2 years ago
QA Whiteboard: [form autofill] [form autofill:M1]
Whiteboard: [form autofill:MVP]
(Reporter)

Updated

2 years ago
Whiteboard: [form autofill:MVP] → [form autofill:M3]
(Assignee)

Updated

2 years ago
Depends on: 1303510
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → schung
Status: NEW → ASSIGNED
Comment on attachment 8877112 [details]
Bug 1303515 - Part 1: Add form autofill update doorhanger.

https://reviewboard.mozilla.org/r/148462/#review153162

::: browser/extensions/formautofill/FormAutofillParent.jsm:277
(Diff revision 1)
> +            case "create":
> +              this.profileStorage.addresses.add(address.record);
> +              break;
> +            case "update":
> +              this.profileStorage.addresses.update(address.guid, address.record);
> +              break;

Please add a test that the buttons work. You can look at the existing popup notification tests for ideas.

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:9
(Diff revision 1)
>  
>  preferenceGroupTitle = Form Autofill
>  enableProfileAutofill = Enable Profile Autofill
>  savedProfiles = Saved Profiles…
>  saveAddressMessage = Firefox now saves your form data to help you fill out forms faster!
> +updateAddressMessage = Looks like some of your information has changed. Want to update your address autofill?

I think this string can be improved a bit:

> It looks like some of your information has changed. Do you want to update the address saved in Autofill?

Michelle will need to decide (perhaps with Marketing) what we are going to call the feature ("Form Autofill" or "Autofill") and whether that should be capitalized.
Attachment #8877112 - Flags: review?(MattN+bmo) → review+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1303513
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8877533 [details]
Bug 1303515 - Part 2: Add browser test for autofill update doorhanger.

https://reviewboard.mozilla.org/r/148996/#review153748

::: browser/extensions/formautofill/test/browser/browser_update_doorhanger.js:26
(Diff revision 2)
> +      await ContentTask.spawn(browser, null, async function() {
> +        async function runKeyEvent(element, keyCode) {
> +          // Wait 500ms to make sure previous action is completed
> +          await new Promise(resolve => setTimeout(resolve, 500));
> +          let ev = content.document.createEvent("KeyboardEvent");
> +          ev.initKeyEvent("keypress", true, true, null, false, false, false, false,

It should be KeyboardEvent constructor actually, but applying the new level 3 constructor here will cause unexpected error that it will submit the form immediately after dispatching enter event... Not sure if it's bug in ContentTask or new KeyboardEvent API.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8877533 [details]
Bug 1303515 - Part 2: Add browser test for autofill update doorhanger.

https://reviewboard.mozilla.org/r/148996/#review154058

::: browser/extensions/formautofill/test/browser/browser_update_doorhanger.js:24
(Diff revision 3)
> +      let promiseShown = BrowserTestUtils.waitForEvent(PopupNotifications.panel,
> +                                                       "popupshown");
> +      await ContentTask.spawn(browser, null, async function() {
> +        async function runKeyEvent(element, keyCode) {
> +          // Wait 1000ms to make sure previous action is completed
> +          await new Promise(resolve => setTimeout(resolve, 1000));

I know it looks really fragile that we always need to pause for a magic number, but I couldn't find any better solution in ContentTask :/
(Assignee)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8877112 [details]
Bug 1303515 - Part 1: Add form autofill update doorhanger.

https://reviewboard.mozilla.org/r/148462/#review154060

::: browser/extensions/formautofill/FormAutofillParent.jsm:278
(Diff revision 2)
>      if (address.guid) {
>        if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) {
> -        // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200)
> +        FormAutofillDoorhanger.show(target, "update").then((state) => {
> +          switch (state) {
> +            case "create":
> +              if (!this.profileStorage.addresses.mergeToStorage(address.record)) {

I changed this part after discussion with Juwie because she thiught that we should try to avoid any unnecessary profile saving even in address updating.
Comment on attachment 8877533 [details]
Bug 1303515 - Part 2: Add browser test for autofill update doorhanger.

https://reviewboard.mozilla.org/r/148996/#review154058

> I know it looks really fragile that we always need to pause for a magic number, but I couldn't find any better solution in ContentTask :/

It seems like we should be refactoring the parent_utils file into a testing JSM so that it can be used by bc tests too.
Comment on attachment 8877533 [details]
Bug 1303515 - Part 2: Add browser test for autofill update doorhanger.

https://reviewboard.mozilla.org/r/148996/#review154324

This test looks super fragile as-is. I don't understand why you can't wait to see the selected row change in autocomplete and wait for the DOMAutocomplete event when RETURN is pressed? Can you try to re-use the helpers from mochitest-plain either by moving them to a test JSM and/or using loadSubScript?

::: browser/extensions/formautofill/test/browser/browser_update_doorhanger.js:5
(Diff revision 3)
> +registerCleanupFunction(async function() {
> +  let addresses = await getAddresses();
> +  if (addresses.length) {
> +    await removeAddresses(addresses.map(address => address.guid));
> +  }
> +});

Ray is moving this to head.js in his footer bug btw. so we don't need to repeat it everywhere.
Attachment #8877533 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8877868 [details]
Bug 1303515 - Part 3: Add plain mochitest for address autofill and merge.

https://reviewboard.mozilla.org/r/149290/#review158498
Attachment #8877868 - Flags: review?(lchang) → review+
(Reporter)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8877533 [details]
Bug 1303515 - Part 2: Add browser test for autofill update doorhanger.

https://reviewboard.mozilla.org/r/148996/#review158538

Looks good. Thanks.
Attachment #8877533 - Flags: review?(lchang) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Part 1 has unresolved issues in MozReview that are preventing Autoland from pushing these patches.
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/571481e5562a
Part 1: Add form autofill update doorhanger. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2854c4f6e026
Part 2: Add browser test for autofill update doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/bf0d8b3a56b7
Part 3: Add plain mochitest for address autofill and merge. r=lchang
Keywords: checkin-needed

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/571481e5562a
https://hg.mozilla.org/mozilla-central/rev/2854c4f6e026
https://hg.mozilla.org/mozilla-central/rev/bf0d8b3a56b7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Ni for Vance as agreed, since this bug appears not to reflect the current auto-fill behavior.
Flags: needinfo?(vchen)
(Assignee)

Comment 24

2 years ago
Hi Adrian, sorry about the misleading title. It's simply a doorhanger feature that need to be displayed when user submitted a modified autofilled form.
Flags: needinfo?(vchen)
Summary: [Form Autofill] A drop down menu in the updating door hanger to let users save a new profile instead of updating → [Form Autofill] Display the updating door hanger to let users save a new address or update choosen address
Thanks Steve for the clarification. 
Marking as verified on Ubuntu 16.04, OSX 10.12 and Win 10 x 64 - 56.0a1 / 20170713100259.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.