Closed Bug 1303515 Opened 7 years ago Closed 6 years ago

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

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: lchang, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3])

Attachments

(3 files)

Bug 1303515 - Part 1: Add form autofill update doorhanger.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1303515 - Part 2: Add browser test for autofill update doorhanger.
59 bytes, text/x-review-board-request
lchang
: review+
Details
Bug 1303515 - Part 3: Add plain mochitest for address autofill and merge.
59 bytes, text/x-review-board-request
lchang
: review+
Details 
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]
QA Whiteboard: [form autofill] [form autofill:M1]
Whiteboard: [form autofill:MVP]
Whiteboard: [form autofill:MVP] → [form autofill:M3]
Depends on: 1303510
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+
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 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 :/
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 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+
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+
Keywords: checkin-needed
Part 1 has unresolved issues in MozReview that are preventing Autoland from pushing these patches.
Keywords: checkin-needed
Keywords: checkin-needed
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
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
Closed: 6 years ago
status-firefox56: --- → fixed
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)
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
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.