1434480 - Add spotlight support to Form Autofill doorhangers

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Matthew N. [:MattN]]

+++ 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

Comment 1

[Maxim Kupfer]

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.

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

[Maxim Kupfer]

Attachment: autofill.png

Attaching screen shot to get clarity on bug

Comment 4

[Maxim Kupfer]

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?

Comment 5

[Maxim Kupfer]

Attachment: autofill-update-doorhanger.png

Comment 6

[Maxim Kupfer]

Also, could you provide instructions on how to replicate the implementation with first-time-use (FTU)address doorhanger.

Comment 7

[Matthew N. [:MattN]]

(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

Comment 8

[Matthew N. [:MattN]]

(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 9

[Maxim Kupfer]

Attachment: Bug 1434480 - added highlighting support via spotlightURL additions to updateAddress and updateCreditCard

Review commit: https://reviewboard.mozilla.org/r/238644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238644/

Comment 10

[Matthew N. [:MattN]]

Does this also handle `addCreditCard` too ([A] from comment 2)?

Comment 11

[Matthew N. [:MattN]]

Can you also make sure browser/extensions/formautofill/test/browser/ tests pass?

Comment 12

[Maxim Kupfer]

(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?

Comment 13

[Matthew N. [:MattN]]

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.

Comment 14

[Matthew N. [:MattN]]

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.

Comment 15

[Maxim Kupfer]

Attachment: Bug 1434480 - added highlighting support via spotlightURL additions to updateAddress and updateCreditCard

Review commit: https://reviewboard.mozilla.org/r/240170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240170/

Comment 16

[Matthew N. [:MattN]]

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",

Comment 17

[Maxim Kupfer]

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?

Comment 18

[Matthew N. [:MattN]]

(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.

Comment 19

[Maxim Kupfer]

(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.

Comment 20

[Maxim Kupfer]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240170/diff/1-2/

Comment 21

[Matthew N. [:MattN]]

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!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0e292452756
added highlighting support via spotlightURL additions to updateAddress and updateCreditCard r=MattN

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e0e292452756