Add spotlight support to Form Autofill doorhangers

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: MattN, Assigned: mbkupfer, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [lang=js], URL)

Attachments

(2 attachments, 2 obsolete attachments)

+++ This bug was initially created as a clone of Bug #1429017 +++

The Spotlight feature makes it possible to open and highlight a section in preferences:
about:preferences#privacy-address-autofill

Form Autofill doorhangers should implement this feature. Bug 1429017 implemented this only for the first-time-use (FTU) address doorhanger. This will cover the others.

(Quoting Matthew N. [:MattN] from bug 1429017 comment #1)
> 2) Change "about:preferences#privacy" to
> "about:preferences#privacy-address-autofill" or
> "about:preferences#privacy-credit-card-autofill" depending on which
> doorhanger is showing:
> https://dxr.mozilla.org/mozilla-central/rev/8b1f0ca44c7be9a15bfbedf0cde540831f2e2aec/browser/extensions/formautofill/FormAutofillDoorhanger.jsm#38-39,74,79,101,106,156,161,241
(Assignee)

Comment 1

a year ago
Hi Matt, not clear on what needs to be done here. From what I understand, when I head over to: about:preferences#privacy-address-autofill the hbox element with id=addressAutofill is highlighted. Everything else is Greek to me. Not sure what doorhangers and what a firs-time-use address is. 

Could you provide some more explanation for a simpleton like myself that is still getting started with the firefox project.
Flags: needinfo?(MattN+bmo)
The issue is there is code linking to about:preferences#privacy, not the autofill-specific fragments that they should point to. A doorhanger is the arrow panel that appears connected to the address bar for prompting. In this case we're talking about the prompts to save and update a credit card and the prompt to save a correction to a saved address. See the highlighted lines in the link from comment 0.

You can use the test forms from https://luke-chang.github.io/autofill-demo/ to trigger these three prompts
A) Save a credit card: Submit a fake credit card on https://luke-chang.github.io/autofill-demo/basic_cc_2.html
B) Update a credit card: Once you have a saved credit card, autofill it on the test page from (1) and then change the expiration date before saving.
C) Save an address: Similar to (B) but for an address instead of a a credit card. Autofill one in an address form such as https://luke-chang.github.io/autofill-demo/basic.html and then edit a field or two so that it's no longer yellow and then submit to get the last doorhanger.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 3

a year ago
Posted image autofill.png (obsolete) —
Attaching screen shot to get clarity on bug
(Assignee)

Comment 4

a year ago
Ok, let me make sure I got this right before I get cracking. From what I understand: after I create an address/credit card and then update one of the auto-fill details when filling out a new form, a notification (doorhanger?) pops up. (see screenshot titled autofill-update-doorhanger.png)

We have three options in that notification: Form Autofill Options, Update Address, and Create New Address. 

Is the idea for us to have the "Form Autofill Options" takes us to the link that has the spotlight? Is the point to assist users in finding the correct location to make the changes?
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 5

a year ago
Attachment #8969735 - Attachment is obsolete: true
(Assignee)

Comment 6

a year ago
Also, could you provide instructions on how to replicate the implementation with first-time-use (FTU)address doorhanger.
(In reply to Maxim Kupfer from comment #4)
> Ok, let me make sure I got this right before I get cracking. From what I
> understand: after I create an address/credit card and then update one of the
> auto-fill details when filling out a new form, a notification (doorhanger?)
> pops up. (see screenshot titled autofill-update-doorhanger.png)

Correct

> We have three options in that notification: Form Autofill Options, Update
> Address, and Create New Address. 
> 
> Is the idea for us to have the "Form Autofill Options" takes us to the link
> that has the spotlight? 

Yes

> Is the point to assist users in finding the correct location to make the changes?

Yes
Flags: needinfo?(MattN+bmo)
(In reply to Maxim Kupfer from comment #6)
> Also, could you provide instructions on how to replicate the implementation
> with first-time-use (FTU)address doorhanger.

That was already fixed by bug 1429017. See bug 1429017 comment 1 for instructions to see reset the preference to see it again.
Comment hidden (mozreview-request)
Assignee: nobody → mbkupfer
Status: NEW → ASSIGNED
Does this also handle `addCreditCard` too ([A] from comment 2)?
Can you also make sure browser/extensions/formautofill/test/browser/ tests pass?
(Assignee)

Comment 12

a year ago
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #10)
> Does this also handle `addCreditCard` too ([A] from comment 2)?

I believe it does. When I use that link and make a change I get the spotlight url. Is it not working for you?
(Reporter)

Comment 13

a year ago
mozreview-review
Comment on attachment 8969802 [details]
Bug 1434480 - added highlighting support via spotlightURL additions to updateAddress and updateCreditCard

https://reviewboard.mozilla.org/r/238644/#review244508

Hey Maxim, you're very close but the add credit card dialog isn't working yet though that's probably just one missing line.

Thanks

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:100
(Diff revision 1)
>    addCreditCard: {
>      notificationId: "autofill-credit-card",
>      message: formatStringFromName("saveCreditCardMessage", [brandShortName], 1),
>      descriptionLabel: GetStringFromName("saveCreditCardDescriptionLabel"),
>      descriptionIcon: true,
>      linkMessage: GetStringFromName(autofillSecurityOptionsKey),
>      anchor: {

(In reply to Maxim Kupfer from comment #12)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #10)
> > Does this also handle `addCreditCard` too ([A] from comment 2)?
> 
> I believe it does. When I use that link and make a change I get the
> spotlight url. Is it not working for you?

No, since you didn't set `spotlightURL` here, www.undefined.com is loaded instead of the spotlight URL.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:241
(Diff revision 1)
> -  _appendPrivacyPanelLink(content, message) {
> +  _appendPrivacyPanelLink(content, message, link) {
>      let chromeDoc = content.ownerDocument;
>      let privacyLinkElement = chromeDoc.createElement("label");
>      privacyLinkElement.className = "text-link";
>      privacyLinkElement.setAttribute("useoriginprincipal", true);
> -    privacyLinkElement.setAttribute("href", "about:preferences#privacy");
> +    privacyLinkElement.setAttribute("href", link);

Maybe do `link || "about:preferences#privacy"` so that we never load undefined.com if we get `undefined` from a new consumer.
Attachment #8969802 - Flags: review?(MattN+bmo)
(Reporter)

Comment 14

a year ago
mozreview-review
Comment on attachment 8969802 [details]
Bug 1434480 - added highlighting support via spotlightURL additions to updateAddress and updateCreditCard

https://reviewboard.mozilla.org/r/238644/#review244514

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a32da2a191195d818bf4d8d51774fc24024fca76

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:229
(Diff revision 1)
>    /**
>     * Append the link label element to the popupnotificationcontent.
>     * @param  {XULElement} content
>     *         popupnotificationcontent
>     * @param  {string} message
>     *         The localized string for link title.
>     */
> -  _appendPrivacyPanelLink(content, message) {
> +  _appendPrivacyPanelLink(content, message, link) {

I also noticed an eslint failure because you didn't document the `link` argument:

> FormAutofillDoorhanger.jsm:229:3 | Missing JSDoc for parameter 'link'. (valid-jsdoc)

You can run `./mach eslint --outgoing -w` to run it locally.
(Assignee)

Updated

a year ago
Attachment #8969802 - Attachment is obsolete: true
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8971417 [details]
Bug 1434480 - added highlighting support via spotlightURL additions to updateAddress and updateCreditCard

https://reviewboard.mozilla.org/r/240170/#review245974

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:100
(Diff revision 1)
>    addCreditCard: {
>      notificationId: "autofill-credit-card",
>      message: formatStringFromName("saveCreditCardMessage", [brandShortName], 1),
>      descriptionLabel: GetStringFromName("saveCreditCardDescriptionLabel"),
>      descriptionIcon: true,
>      linkMessage: GetStringFromName(autofillSecurityOptionsKey),
>      anchor: {

You still need to add the following here:
> spotlightURL: "about:preferences#privacy-credit-card-autofill",
Attachment #8971417 - Flags: review?(MattN+bmo)
(Assignee)

Comment 17

a year ago
I thought (In reply to Matthew N. [:MattN] (at work week; PM if requests are blocking you) from comment #16)
> Comment on attachment 8971417 [details]
> Bug 1434480 - added highlighting support via spotlightURL additions to
> updateAddress and updateCreditCard
> 
> https://reviewboard.mozilla.org/r/240170/#review245974
> 
> ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:100
> (Diff revision 1)
> >    addCreditCard: {
> >      notificationId: "autofill-credit-card",
> >      message: formatStringFromName("saveCreditCardMessage", [brandShortName], 1),
> >      descriptionLabel: GetStringFromName("saveCreditCardDescriptionLabel"),
> >      descriptionIcon: true,
> >      linkMessage: GetStringFromName(autofillSecurityOptionsKey),
> >      anchor: {
> 
> You still need to add the following here:
> > spotlightURL: "about:preferences#privacy-credit-card-autofill",

1) I thought that this was going to be caught in the 'link || "about:preferences#privacy"'?
2) Could you explain where the addCreditCard doorhanger would pop-up and provide a link to the #privacy pane?
Flags: needinfo?(MattN+bmo)
(In reply to Maxim Kupfer from comment #17)
> I thought (In reply to Matthew N. [:MattN] (at work week; PM if requests are
> blocking you) from comment #16)
> > Comment on attachment 8971417 [details]
> > Bug 1434480 - added highlighting support via spotlightURL additions to
> > updateAddress and updateCreditCard
> > 
> > https://reviewboard.mozilla.org/r/240170/#review245974
> > 
> > ::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:100
> > (Diff revision 1)
> > >    addCreditCard: {
> > >      notificationId: "autofill-credit-card",
> > >      message: formatStringFromName("saveCreditCardMessage", [brandShortName], 1),
> > >      descriptionLabel: GetStringFromName("saveCreditCardDescriptionLabel"),
> > >      descriptionIcon: true,
> > >      linkMessage: GetStringFromName(autofillSecurityOptionsKey),
> > >      anchor: {
> > 
> > You still need to add the following here:
> > > spotlightURL: "about:preferences#privacy-credit-card-autofill",
> 
> 1) I thought that this was going to be caught in the 'link ||
> "about:preferences#privacy"'?

It will but that's just a fallback for future errors to avoid undefined.com… we want to link to about:preferences#privacy-credit-card-autofill for the add credit card case.

> 2) Could you explain where the addCreditCard doorhanger would pop-up and
> provide a link to the #privacy pane?

See comment 2 step (A). The prompt to ask to save has the link.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 19

a year ago
(In reply to Matthew N. [:MattN] (at work week; PM if requests are blocking you) from comment #18)
> (In reply to Maxim Kupfer from comment #17)


> > 2) Could you explain where the addCreditCard doorhanger would pop-up and
> > provide a link to the #privacy pane?
> 
> See comment 2 step (A). The prompt to ask to save has the link.

Strange, I don't get any prompt when I add a new card. I only get the prompt when I update one of the autofilled options.
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
Flags: needinfo?(MattN+bmo)
(Reporter)

Comment 21

a year ago
mozreview-review
Comment on attachment 8971417 [details]
Bug 1434480 - added highlighting support via spotlightURL additions to updateAddress and updateCreditCard

https://reviewboard.mozilla.org/r/240170/#review246020

Thanks!
Attachment #8971417 - Flags: review?(MattN+bmo) → review+

Comment 22

a year ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/e0e292452756
added highlighting support via spotlightURL additions to updateAddress and updateCreditCard r=MattN

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0e292452756
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.