[Form Autofill] Fine tune the first time saving doorhanger

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M4])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
Blocks: 990185
Whiteboard: [form autofill:M4]
(Assignee)

Comment 1

a year ago
Based on bug 1339740 comment 3, we'll try to merge the address even in first time use case.
Comment hidden (mozreview-request)
Assignee: nobody → schung

Comment 3

a year ago
mozreview-review
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.
Attachment #8880281 - Flags: review?(lchang)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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

a year ago
mozreview-review
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
Attachment #8880281 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
Thanks!
(Assignee)

Updated

a year ago
Keywords: checkin-needed
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by lchang@mozilla.com:
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
Keywords: checkin-needed

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/44b5f7d32673
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

11 months ago
Blocks: 1379869
This patch changed Historgrams.json without a data-review. Changing the labels for the telemetry probe caused bug 1411610.
Blocks: 1411610
Flags: needinfo?(schung)
(Assignee)

Comment 13

8 months ago
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.
Flags: needinfo?(schung) → needinfo?(jaws)
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.
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.