When users first time submit a form, a door hanger will be prompted form the top left to ask users to save the form as autofill profile. Users can click on "save" or "Don't save" to decide whether to save the profile or not.

According to the latest UX Spec, the door hanger of saving profile will only appear once.

Hi Matt, is there any way we can inject the assets/string into browser.xul for the new doorhanger, or we should modify the browser.xul directly? I didn't see anyone tried to implement a new one as an addon, but maybe there's a better way like reusing the existing doorhanger?

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. Hi Matt, it's a WIP about the first time saving doorhanger. It's simple popupNotifications wrap up object instead of prototype or class, which I don't think we can have much benefit from. It still has some problem in preferences link and lack of visual assets, but other basic functions should work. I'll raise some questions in the patch later.

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review145202 ::: browser/extensions/formautofill/FormAutofillContent.jsm:323 (Diff revision 2) > * Send the profile to parent for doorhanger and storage saving/updating. > * > * @param {Object} profile Submitted form's address/creditcard guid and record. > */ > - _onFormSubmit(profile) { > - Services.cpmm.sendAsyncMessage("FormAutofill:OnFormSubmit", profile); > + _onFormSubmit(profile, domWin) { > + let win = this.messageManagerFromWindow(domWin); Not sure if we have better way to reference the browser and chrome window in parent process. Here I align the solution in passwordmgr that sending message via content message manager. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:42 (Diff revision 2) > + accessKey: "A", > + callbackState: "apply-autofill", > + }], > + options: { > + persistWhileVisible: true, > + learnMoreURL: "about:preferences#privacy", We probably can't leverage the existing learnMoreURL element in popupNotification since the the string and way of opening the url seems unable to change. Should we get the learnMoreURL element the overwrite it, or create another learnMoreURL-like element and insert it? ::: browser/extensions/formautofill/FormAutofillParent.jsm:270 (Diff revision 2) > }, > > _onFormSubmit(data, target) { > let {address} = data; > > + FormAutofillDoorhanger.init(target); Using init here seems not appropriate because the browser and chrome win might vary. Maybe we only need show method and set the browser as one of the parameter?

I'm still working on the mochitest part, but the overall doorhanger's functionality should be ready in the 2 commits.

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review148598 ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:5 (Diff revision 6) > +/* > + * Implements doorhanger singleton that wraps up the PopupNotifications and handles > + * the doorhager UI for formautofill related features. > + */ We probably shouldn't add another JSM and instead put this in the Parent JSM but I believe there are plans to share the JSM compartment which will remove the overhead IIUC. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:32 (Diff revision 6) > + > +const CONTENT = { > + firstTimeUse: { > + notificationId: "autofill-address", > + message: GetStringFromName("saveAddressMessage"), > + anchorId: null, As we discussed before, you can append the anchor <image> to the window (see the link I sent you on IRC a while ago) to use as the anchor. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:66 (Diff revision 6) > +let FormAutofillDoorhanger = { > + _win: null, > + _browser: null, > + _type: null, > + _notificationcontent: null, Why is this a singleton when there can be multiple instances at once? This JSM is shared with the whole parent process but there can be multiple (non-FTU) doorhangers at once in multiple windows. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:109 (Diff revision 6) > + this._notificationcontent = chromeDoc.createElement("popupnotificationcontent"); > + let privacyLinkElement = chromeDoc.createElement("label"); > + privacyLinkElement.className = "text-link"; > + privacyLinkElement.setAttribute("value", GetStringFromName("viewAutofillOptions")); > + privacyLinkElement.addEventListener("click", this); > + this._notificationcontent.appendChild(privacyLinkElement); Why do this programmatically when you can do it declaratively like usual? You can put the binding in https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/content/formautofill.xml I guess this keeps the code closer together and makes it easier to use .properties so it can stay ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:113 (Diff revision 6) > + if (!this._notificationcontent) { > + this._notificationcontent = chromeDoc.createElement("popupnotificationcontent"); > + let privacyLinkElement = chromeDoc.createElement("label"); > + privacyLinkElement.className = "text-link"; > + privacyLinkElement.setAttribute("value", GetStringFromName("viewAutofillOptions")); > + privacyLinkElement.addEventListener("click", this); For XUL elements you should use the "command" event so that it also handles keyboard activation ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:126 (Diff revision 6) > + init(browser) { > + log.debug("init"); > + this._win = browser.ownerGlobal; > + this._browser = browser; > + }, I suspect we could make this work without state specific to the window or browser if we pass the browser to `show` ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:153 (Diff revision 6) > + show(type) { > + log.debug("show doorhanger with type:", type); > + this._type = type; > + return new Promise((resolve) => { Use an `async` function? ::: browser/extensions/formautofill/FormAutofillParent.jsm:50 (Diff revision 6) > > this.log = null; > FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]); > > const ENABLED_PREF = "extensions.formautofill.addresses.enabled"; > +const FIRST_TIME_USE = "extensions.formautofill.firstTimeUse"; I would just inline the pref as it reduces the indirection when looking usage up. ::: browser/extensions/formautofill/FormAutofillParent.jsm:293 (Diff revision 6) > + this.profileStorage.addresses.getAll().forEach((record) => { > + this.profileStorage.addresses.remove(record.guid); Last time I discussed this with Juwei she said we're not supposed to remove from storage when disabled from preferences… why would this be different? If a user has sync setup and gets this doorhanger on one computer and doesn't want to use autofill on this computer, it shouldn't delete the sync profiles on all their devices. Can we instead delay the .add until the doorhanger is dismissed? Or if we really have to, use the return value of .add to only delete the record that was added? ::: browser/extensions/formautofill/content/icon-address-save.svg:2 (Diff revision 6) > +<?xml version="1.0" encoding="utf-8"?> > +<!-- Generator: Adobe Illustrator 21.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0) --> Nit: Remove this. You should probably follow https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines which include style guidelines and some tools to help with cleaning SVG files up. ::: browser/extensions/formautofill/locale/en-US/formautofill.properties:8 (Diff revision 6) > +saveAddressMessage = Firefox now saves your form data to help you fill out forms faster! > +saveAddressMainLabel = Don't Use Autofill > +saveAddressSecondaryLabel = Use Autofill > +viewAutofillOptions = View Form Autofill options... We should probably wait for Michelle to confirm these before landing to avoid having to change the string IDs later after translations already started.

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review148598 > For XUL elements you should use the "command" event so that it also handles keyboard activation It seems like the elements supported command event are limited(at least label element could not support). Do we need to changed the element to button or something else(which will need less style changes to make it looks like a link)?

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review148598 > Use an `async` function? Not sure how we can apply await/async inside the show function... or you mean await doorhanger.show in parent?

Comment on attachment 8870765 [details] Bug 1303510 - Part 2: Add browser test for doorhanger part, https://reviewboard.mozilla.org/r/142264/#review150596 Thanks

Comment on attachment 8871636 [details] Bug 1303510 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/143146/#review150600 ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:9 (Diff revision 3) > +const VALID_ADDRESS_FIELDS = [ > + "given-name", > + "additional-name", > + "family-name", > + "organization", > + "street-address", > + "address-level2", > + "address-level1", > + "postal-code", > + "country", > + "tel", > + "email", > +]; This seems to be unused. If we need this then we should probably access it from ProfileStorage ::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:91 (Diff revision 3) > +function checkAddresses(expectedAddresses) { > + return new Promise(resolve => { Hmm… are async functions supported here?

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review150498 ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:42 (Diff revision 8) > + /* > + update: { > + notificationId: "autofill-address", > + message: > + anchorId: > + mainAction: { > + label: > + accessKey: > + callbackState: > + }, > + secondaryAction: [{ > + label: > + accessKey: > + callbackState: > + }], > + options: { > + persistWhileVisible:, > + popupIconURL: "chrome://formautofill/content/icon-address-save.svg", > + }, > + }, > + */ Let's try not to land too much commented out code as we're starting to stabilize more. Can you move this to a different commit in another bug. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:66 (Diff revision 8) > + }, > + */ > +}; > + > +let FormAutofillDoorhanger = { > + _notificationcontent: null, This is stil going to be incorrectly shared across windows I think. Can you either make this a WeakMap with the chromewindow/browser.xul document as the key or simply lookup the created element by ID in the appropriate browser document each time? Since `_appendPrivacyPanelLink` is the only API that needs this then it shouldn't be too much of a perf. issue to do the one getElementById every time especially since it's not often shown. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:94 (Diff revision 8) > + secondaryActionParams.forEach((params) => { > + let cb = resolve.bind(null, params.callbackState); Why not use for…of? ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:96 (Diff revision 8) > + secondaryActions.push( > + {label: params.label, accessKey: params.accessKey, callback: cb} > + ); Nit: One property per line with a trailing comma to make it easier to follow blame for single line changes ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:118 (Diff revision 8) > + privacyLinkElement.className = "text-link"; > + privacyLinkElement.setAttribute("value", GetStringFromName("viewAutofillOptions")); > + this._notificationcontent.appendChild(privacyLinkElement); If you set the attribute "useoriginprincipal" to "true" then you can simply set the "href" attribute to "about:preferences#privacy" and avoid the event listener. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:125 (Diff revision 8) > + this._notificationcontent.appendChild(privacyLinkElement); > + } > + notification.append(this._notificationcontent); > + }, > + /** > + * Create an image element for notification anchor. …if it doesn't already exist. ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:165 (Diff revision 8) > + show(browser, type) { > + log.debug("show doorhanger with type:", type); > + return new Promise((resolve) => { Nit: Make this an async function since it returns a promise? ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:178 (Diff revision 8) > + case "shown": > + this._appendPrivacyPanelLink(browser, content.notificationId); Doesn't this have to be during "showing" instead of "shown" in order to avoid a flicker? ::: browser/extensions/formautofill/content/icon-address-save.svg:1 (Diff revision 8) > +<svg id="Layer_1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32"> Nit: Remove @id ::: browser/extensions/formautofill/content/icon-address-save.svg:1 (Diff revision 8) > +<svg id="Layer_1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32"> Double-check if SVG files are supposed to have a license ::: browser/extensions/formautofill/content/icon-address-save.svg:3 (Diff revision 8) > + .st0{fill:#999899;} > + </style> > + <path class="st0" d="M22 13.7H9.4c-.6 0-1.2.5-1.2 1.2 0 .6.5 1.2 1.2 1.2H22c.6 0 1.2-.5 1.2-1.2s-.6-1.2-1.2-1.2zM6.1 26.6V5.5c0-.8.7-1.5 1.5-1.5h16c.9 0 1.5.6 1.5 1.5V16h2V3.8c0-1-.7-1.8-1.8-1.8H5.9c-1 0-1.8.8-1.8 1.8v24.5c0 1 .8 1.7 1.8 1.7h9.3v-2H7.6c-.8 0-1.5-.6-1.5-1.4zm21.1-1.9h-2.5V20c0-.4-.3-.8-.8-.8h-3.1c-.4 0-.8.3-.8.8v4.6h-2.5c-.6 0-.8.4-.3.8l4.3 4.2c.2.2.5.3.8.3s.6-.1.8-.3l4.3-4.2c.6-.4.4-.7-.2-.7zm-11.3-5.6H9.4c-.6 0-1.2.5-1.2 1.2s.5 1.2 1.2 1.2h6.5c.6 0 1.2-.5 1.2-1.2s-.6-1.2-1.2-1.2zM22 7.8H9.4c-.6 0-1.2.5-1.2 1.2s.5 1.2 1.2 1.2H22c.6 0 1.2-.5 1.2-1.2s-.6-1.2-1.2-1.2z"/> Nit: You can remove the class and move the fill property to an attribute of the <path> I think ::: browser/extensions/formautofill/locale/en-US/formautofill.properties:9 (Diff revision 8) > +saveAddressMainLabel = Don't Use Autofill > +saveAddressSecondaryLabel = Use Autofill Please remove the unused strings ::: browser/extensions/formautofill/locale/en-US/formautofill.properties:12 (Diff revision 8) > savedProfiles = Saved Profiles… > +saveAddressMessage = Firefox now saves your form data to help you fill out forms faster! > +saveAddressMainLabel = Don't Use Autofill > +saveAddressSecondaryLabel = Use Autofill > +viewAutofillOptions = View Form Autofill options... > +openAutofillMessagePanel = Open autofill message panel Nit: Open Form Autofill message panel This should really be part of the spec and reviewed by Michelle. I think Form Autofill should be capitalized but I'm not sure.

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review150602 ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:80 (Diff revision 8) > + _createActions(mainActionParams, secondaryActionParams, resolve) { > + if (!mainActionParams) { > + return [null, null]; > + } I guess we don't really need this method now and it doesn't get tested. It's probably okay to land now.

Comment on attachment 8871636 [details] Bug 1303510 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/143146/#review150644 ::: browser/extensions/formautofill/test/mochitest/test_address_submission.html:54 (Diff revision 3) > + <p><label>organization: <input id="organization" name="organization" autocomplete="organization" type="text"></label></p> > + <p><label>streetAddress: <input id="street-address" name="street-address" autocomplete="street-address" type="text"></label></p> > + <p><label>tel: <input id="tel" name="tel" autocomplete="tel" type="text"></label></p> > + <p><label>country: <input id="country" name="country" autocomplete="country" type="text"></label></p> > + <p><input type="submit"></p> > + <p><button type="reset">Reset</button></p> Please remove any unused attributes/elements as they make the test harder to read.

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review150498 > Doesn't this have to be during "showing" instead of "shown" in order to avoid a flicker? I tried to append the content while showing before but sadly the notification element seems not appended to doc... the notification element is reachable only when the state is shown :/ > Double-check if SVG files are supposed to have a license Seems like not all the svg assets have license... Thanks for the reminder

Comment on attachment 8869372 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/141022/#review151370 ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:42 (Diff revision 9) > + /* TODO: Implement update in bug 1303515 > + update: { > + }, > + */ It would be better to just omit this. ::: browser/extensions/formautofill/FormAutofillParent.jsm:278 (Diff revision 9) > if (address.guid) { > // TODO: Show update doorhanger(bug 1303513) and set probe(bug 990200) > - // if (!profileStorage.addresses.mergeIfPossible(address.guid, address.record)) { > + // if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record)) { > // } > } else { > - // TODO: Add first time use probe(bug 990199) and doorhanger(bug 1303510) > + this.profileStorage.addresses.add(address.record); IIUC, this should be a mergeIfPossible otherwise we will save duplicates of manually created profiles.

Comment on attachment 8871636 [details] Bug 1303510 - Part 3: Add mochitest-plain for saving submitted form. https://reviewboard.mozilla.org/r/143146/#review151380

Thanks for all the reviews! Although the UX spec might be changed later, we could still apply these changes in bug 1303515

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s e406ff2a2f82 -d 99fe8edb8296: rebasing 400974:e406ff2a2f82 "Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. r=MattN" merging browser/app/profile/firefox.js warning: conflicts while merging browser/app/profile/firefox.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df527bcc28dc Part 1: Implement doorhanger helper and parameter for first time use doorhanger. r=MattN https://hg.mozilla.org/integration/autoland/rev/0d885ad1419d Part 2: Add browser test for doorhanger part, r=MattN https://hg.mozilla.org/integration/autoland/rev/19bc87470994 Part 3: Add mochitest-plain for saving submitted form. r=MattN

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=105770765&repo=autoland

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a12026007a59 Backed out changeset 19bc87470994 https://hg.mozilla.org/integration/autoland/rev/5eeca8c89ac2 Backed out changeset 0d885ad1419d https://hg.mozilla.org/integration/autoland/rev/7fc5e5d91891 Backed out changeset df527bcc28dc for test failures in browser_first_time_use_doorhanger.js

Comment on attachment 8876455 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. Hi Matt, this review was cleared accidentally but I didn't change code(only an intermittent error fixing) after your review. So I will redirect the review to Luke for meeting the 612 deadline.

Comment on attachment 8876455 [details] Bug 1303510 - Part 1: Implement doorhanger helper and parameter for first time use doorhanger. https://reviewboard.mozilla.org/r/147792/#review152296

Comment on attachment 8876456 [details] Bug 1303510 - Part 2: Add browser test for doorhanger part, https://reviewboard.mozilla.org/r/147794/#review152298

Carry over Matt's r+.

Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/24c1512f3991 Part 1: Implement doorhanger helper and parameter for first time use doorhanger. r=lchang https://hg.mozilla.org/integration/autoland/rev/5e5c3a085c86 Part 2: Add browser test for doorhanger part, r=lchang https://hg.mozilla.org/integration/autoland/rev/b9b8f73f50a1 Part 3: Add mochitest-plain for saving submitted form. r=MattN

https://hg.mozilla.org/mozilla-central/rev/24c1512f3991 https://hg.mozilla.org/mozilla-central/rev/5e5c3a085c86 https://hg.mozilla.org/mozilla-central/rev/b9b8f73f50a1