1374508 - [Form Autofill] Fine tune the first time saving doorhanger

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Steve Chung [:steveck]]

Since the UX spec for doorhanger is finalized and the latest first time saving doorhanger has some differences comparing to the old one. We'll need to apply the up-to-date copy string/icon assets/layout in this bug.

Comment 1

[Steve Chung [:steveck]]

Based on bug 1339740 comment 3, we'll try to merge the address even in first time use case.

Comment 2

[Steve Chung [:steveck]]

Attachment: Bug 1374508 - [Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button.

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

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8880281 [details]
Bug 1374508 - [Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button.

https://reviewboard.mozilla.org/r/151638/#review156622

We should add some integration tests to cover merging strategy.

::: commit-message-0882c:1
(Diff revision 1)
> +Bug 1374508 - [Form Autofill] Fine tune the first time saving doorhanger. r=lchang

Please list down details that will be addressed in this bug in the following lines.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:158
(Diff revision 1)
>        content.options.eventCallback = (topic) => {
>          log.debug("eventCallback:", topic);
>  
>          switch (topic) {
>            // We can only append label element when notification box is shown
>            case "shown":
> +            if (!content.hasPrivacyPanelLink) {
> +              return;
> +            }
>              this._appendPrivacyPanelLink(browser, content.notificationId);
>              break;
>          }
>        };

I think we will have plenty different behaviors among "address-save", "address-update" and "credit-card-save" doorhangers. Using just `hasPrivacyPanelLink` to distinguish them seems not sufficient and might lower readability. I'd recommend to use different functions for each type of doorhangers.

::: browser/extensions/formautofill/FormAutofillParent.jsm:302
(Diff revision 1)
> +        FormAutofillDoorhanger.show(target, "firstTimeUse").then((state) => {
> +          if (state !== "open-pref") {
> -        return;
> +            return;
> -      }
> +          }
>  
> -      let guid = this.profileStorage.addresses.add(address.record);
> +          target.ownerGlobal.openPreferences("privacy");

Isn't it `panePrivacy`? Also, you need to create a new keyword and set it to the `origin`.

::: browser/extensions/formautofill/locale/en-US/formautofill.properties:9
(Diff revision 1)
>  viewAutofillOptions = View Form Autofill options…
> +changeAutofillOptions = Change Form Autofill Options

In the suggestion dropdown footer, we use "preferences" on macOS and "options" on windows/linux. I think we should do that here as well.

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8880281 [details]
Bug 1374508 - [Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button.

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

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8880281 [details]
Bug 1374508 - [Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button.

https://reviewboard.mozilla.org/r/151638/#review156622

I added some tests in update doorhanger WIP https://reviewboard.mozilla.org/r/149290/. Maybe I can move some of the tests to this one.

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8880281 [details]
Bug 1374508 - [Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button.

https://reviewboard.mozilla.org/r/151638/#review157128

::: commit-message-0882c:1
(Diff revision 2)
> +Bug 1374508 - [Form Autofill Doorhanger]Apply copy reviewed string/new anchor icon and move open pref link to button. r=lchang

nit: one space between `]` and `Apply`.

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:53
(Diff revision 2)
> +    },
>      options: {
>        persistWhileVisible: true,
>        popupIconURL: "chrome://formautofill/content/icon-address-save.svg",
>      },
> +    hasPrivacyPanelLink: false,

It's unused now.

::: browser/extensions/formautofill/FormAutofillParent.jsm:303
(Diff revision 2)
> +          if (state !== "open-pref") {
> -        return;
> +            return;
> -      }
> +          }
>  
> -      let guid = this.profileStorage.addresses.add(address.record);
> -      this.profileStorage.addresses.notifyUsed(guid);
> +          target.ownerGlobal.openPreferences("panePrivacy",
> +                                             {origin: "autofillDoorhanger"});

You need to add the keyword to https://dxr.mozilla.org/mozilla-central/rev/b1b9129838ade91684574f42219b2010928d7db4/toolkit/components/telemetry/Histograms.json#6454

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8880281 [details]
Bug 1374508 - [Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button.

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

Comment 8

[Steve Chung [:steveck]]

Thanks!

Comment 9

[Steve Chung [:steveck]]

Comment on attachment 8880281 [details]
Bug 1374508 - [Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151638/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44b5f7d32673
[Form Autofill Doorhanger] Apply copy reviewed string/new anchor icon and move open pref link to button. r=lchang

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44b5f7d32673

Comment 12

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

This patch changed Historgrams.json without a data-review. Changing the labels for the telemetry probe caused bug 1411610.

Comment 13

[Steve Chung [:steveck]]

Hi Jared,

Sorry that I didn't aware that adding new label need data review as well. But base on the comments in bug 1403994, it looks like histogram_aggregator could not aggregate 2 histograms with different label number properly. I can ask for data-review for this patch but I guess the issue is still there without fixing aggregator. I can help to avoid the buggy report temporary if you prefer:

- Create another bug that remove the label for now and resume after bug 1403994 fixed
- Create another bug that using another way to open preferences(that will not trigger telemetry) and resume after bug 1403994 fixed

Or simply ask for data review here.

Comment 14

[(Away until May 24) Jared Wein [:jaws] (please needinfo? me)]

It's OK, I'm not sure the data-review would have caught this bug because I don't think they were aware of it back then. I flagged you needinfo to make sure you saw my comment. I'm fine with "doing nothing". It seems this is a bug in the Telemetry aggregation/display code, not in your patch.