1415022 - [Form Autofill] Provide a profile summary in a door hanger for recognizing the profiles

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Sean Lee [:seanlee][:weilonge]]

Since we need to consider the multiple profiles of Credit Card and Address, one door hanger can be on the top of another one.

The door hangers of "Update" or "Create New" can be merged or used in a simplified way.

The description should be refined as well for use to recognize the door hanger for.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Hi Mark,

Could you provide your ideas for the multiple door hanger cases from the buttons and description of the submit door hanger? Thank you.

Comment 2

[Mark Liang(:mark_liang)]

Hi Sean,

Instead of merging the doorhangers, I propose having additional information on the doorhanger if there are more than one address or credit card doorhangers for now.

Something like this:
Address: https://mozilla.invisionapp.com/share/2TEJR61G7
Credit Card: https://mozilla.invisionapp.com/share/G6EJR6UAT

Any thoughts?

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Hi Mark,

Thank you for providing the spec of multiple doorhanger cases. I have two questions and we had an agreement in our offline chat:
1. When there is only one address or credit card profile needs to be updated/created, is the description still shown in the doorhanger?
Our agreement is to show the description no matter how many profiles need to be handled.

2. Could you provide the detail definition of the description? Per our offline discussion, it should be the same rules with the summary of each profile in "Saved Addresses/CreditCards" Preference.

Could you update the above information to the spec? Thank you.

Comment 4

[Mark Liang(:mark_liang)]

Hi Sean,

Thanks for the questions,
1. We want to extend the additional information to all the form autofill doorhangers, so yes the description will still be shown when there's only one address or credit card.

2. Yes same rules, updated in the spec now.

Address: https://mozilla.invisionapp.com/share/2TEJR61G7
Credit Card: https://mozilla.invisionapp.com/share/G6EJR6UAT

Comment 5

[Ray Lin[:ralin]]

Any idea can make bug 1421173 less painful and about description, is also welcome here. :D

Comment 6

[Ray Lin[:ralin]]

Attachment: Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

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

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

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

Comment 8

[Ray Lin[:ralin]]

The b-c test will be added in a separate comment. Could you help me to review the patch and see the change is on right direction? Thanks.

Comment 9

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review210898

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:263
(Diff revision 2)
> +    let descriptionLabelElement = chromeDoc.createElement("label");
> +    descriptionLabelElement.setAttribute("value", descriptionLabel);
> +    docFragment.appendChild(descriptionLabelElement);
> +
> +    let descriptionWrapper = chromeDoc.createElement("hbox");
> +    descriptionWrapper.style.marginBlockEnd = "12px";

a bit hard coded, any alternative?

::: browser/extensions/formautofill/FormAutofillParent.jsm:399
(Diff revision 2)
>        }
>  
>        if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record, true)) {
>          this._recordFormFillingTime("address", "autofill-update", timeStartedFillingMS);
>  
>          showDoorhanger = async () => {

Should have a mergeIfPossible check in the callback to get rid of unnecessary doorhanger, and also consider doing record retrieval again to help description up-to-date?

::: browser/extensions/formautofill/locales/en-US/formautofill.properties:30
(Diff revision 2)
>  # this checkbox is displayed on the doorhanger shown when saving credit card.
>  creditCardsSyncCheckbox = Share credit cards with synced devices
>  # LOCALIZATION NOTE (updateAddressMessage, createAddressLabel, updateAddressLabel): Used on the doorhanger
>  # when an address change is detected.
>  updateAddressMessage = Would you like to update your address with this new information?
> +updateAddressDescriptionLabel = Address to update:

Just to remind myself, I guess here will need a note to tell localizer the expected string would looks like.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review210978

::: browser/extensions/formautofill/FormAutofillParent.jsm:399
(Diff revision 2)
>        }
>  
>        if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record, true)) {
>          this._recordFormFillingTime("address", "autofill-update", timeStartedFillingMS);
>  
>          showDoorhanger = async () => {

It's a good question and I think there's 2 differernt approach:
- Just like you said we might need to check merge in this showDoorhanger async function, but it'll be duplicated to the merge checking above and mess up the telemetry result. You'll need to move the merge checking and telemetry inside the showDoorhanger(and rename it to maybeShowDoorhanger)
- Another thought is we apply for loop with await instead of promise.all in `_onFormSubmit`, but you might need to revert to the previous implematation.

Comment 11

[Ray Lin[:ralin]]

Per discussion with Steve offline, we concurred with that we just keep current implementation in terms of the uncertainty of telemetry behavior. We'll file a separate bug to discuss how we can eliminate the redundant doorhangers and come up with smarter telemetry recording strategy.  

> a bit hard coded, any alternative?
Also Steve suggested that it's better if we can use CSS to style the description instead of the hard-coded way.

I'll fix the issues soon later.

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

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

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review211368

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:259
(Diff revision 3)
> +  _appendDescription(content, descriptionLabel, descriptionIcon) {
> +    let chromeDoc = content.ownerDocument;
> +    let docFragment = chromeDoc.createDocumentFragment();
> +
> +    let descriptionLabelElement = chromeDoc.createElement("label");
> +    descriptionLabelElement.className = "desc-message-label";

Do we need this `desc-message-label` class?

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:268
(Diff revision 3)
> +    let descriptionWrapper = chromeDoc.createElement("hbox");
> +    descriptionWrapper.className = "desc-message-box";
> +
> +    if (descriptionIcon) {
> +      let descriptionIconElement = chromeDoc.createElement("image");
> +      descriptionIconElement.className = "desc-message-icon";

Since we already has a wrapper `desc-message-box`, maybe we can simply set the selector like `.desc-message-box > image/description` instead of adding more class (unless you'll need more different style of image/description in `desc-message-box`)

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

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

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review211368

Thanks for review!

> Do we need this `desc-message-label` class?

No, we don't need that.

> Since we already has a wrapper `desc-message-box`, maybe we can simply set the selector like `.desc-message-box > image/description` instead of adding more class (unless you'll need more different style of image/description in `desc-message-box`)

Good idea, more concise indeed. Thanks

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review211644

Only some more few nits and seems no serious issue, thanks

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:282
(Diff revision 3)
> +
> +    content.appendChild(docFragment);
>    },
> +
> +  _updateDescription(content, description) {
> +    let descriptionElement = content.querySelector("description");

nit: maybe we can set textContent directly without  `descriptionElement` (content.querySelector("description").textCoentent = description;)

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:385
(Diff revision 3)
>          // There's no preferences link or other customization in first time use doorhanger.
>          if (type == "firstTimeUse") {
>            return;
>          }
>  
> -        this._appendPrivacyPanelLink(browser, notificationId, linkMessage);
> +        let notificationElementId = notificationId + "-notification";

nit: I guess you would like it to be `const`

::: browser/extensions/formautofill/FormAutofillUtils.jsm:270
(Diff revision 3)
> +      "postal-code",     // Postal code
> +      "tel",             // Phone number
> +      "email",           // Email address
> +    ];
> +
> +    address = {...address};

Any specific reason that we have to do this here?

::: browser/extensions/formautofill/content/formautofill.css:53
(Diff revision 3)
> +/* Form Autofill Doorhanger */
> +#autofill-address-notification > popupnotificationcontent > .desc-message-box,
> +#autofill-credit-card-notification > popupnotificationcontent > .desc-message-box {
> +  margin-block-end: 12px;
> +}
> +#autofill-address-notification > popupnotificationcontent > .desc-message-box > .desc-message-icon,

I think the address doorhanger will not have the icon now(and in the near future), so maybe we can simply set for credit card.
Furthermore, we can even set the src path in css instead of in FormAutofillDoorhanger.js, and treat `descriptionIcon` as boolean(we can treat it as class string if it's necessary to have different src).

Comment 17

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

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

Comment 18

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review211644

Thanks!

> nit: maybe we can set textContent directly without  `descriptionElement` (content.querySelector("description").textCoentent = description;)

fixed

> Any specific reason that we have to do this here?

Just a shallow-copy that avoid original passed address being modified.

> I think the address doorhanger will not have the icon now(and in the near future), so maybe we can simply set for credit card.
> Furthermore, we can even set the src path in css instead of in FormAutofillDoorhanger.js, and treat `descriptionIcon` as boolean(we can treat it as class string if it's necessary to have different src).

agree, thanks. descriptionIcon is now a boolean to determine appending <image>. Like you said, as yet seems no new icon will be added, but we can change it to className string someday when we need it.

Comment 19

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review211750

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:384
(Diff revision 4)
>            return;
>          }
>  
> -        this._appendPrivacyPanelLink(browser, notificationId, linkMessage);
> +        let notificationElementId = notificationId + "-notification";
> +        let notification = chromeDoc.getElementById(notificationElementId);
> +        let notificationcontent = notification.querySelector("popupnotificationcontent") ||

nit: notificationcontent => notificationContent?

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:384
(Diff revision 4)
>            return;
>          }
>  
> -        this._appendPrivacyPanelLink(browser, notificationId, linkMessage);
> +        let notificationElementId = notificationId + "-notification";
> +        let notification = chromeDoc.getElementById(notificationElementId);
> +        let notificationcontent = notification.querySelector("popupnotificationcontent") ||

Just curious, not an issue...

Does this mean that we will reuse the element of `popupnotificationcontent`?
Could this approach work?

```JS
let notificationContent = notification.querySelector("popupnotificationcontent");

if (!notification.contains(notificationContent)) {
  notificationContent = chromeDoc.createElement("popupnotificationcontent");
  ...
}
```

Comment 20

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

https://reviewboard.mozilla.org/r/204810/#review211750

> Just curious, not an issue...
> 
> Does this mean that we will reuse the element of `popupnotificationcontent`?
> Could this approach work?
> 
> ```JS
> let notificationContent = notification.querySelector("popupnotificationcontent");
> 
> if (!notification.contains(notificationContent)) {
>   notificationContent = chromeDoc.createElement("popupnotificationcontent");
>   ...
> }
> ```

> Does this mean that we will reuse the element of popupnotificationcontent?

Yes, the doorhanger will be created at most one time for each type, and do does its content.

> Could this approach work?

I think both work similarly, just subtle differences but with the same effect. My approach is trying to retrieve <popupnotificationcontent> if it's already created, otherwise create a new one. Then I'll test whether the node is dangling and append it if it is.

> nit: notificationcontent => notificationContent?

will change, thanks

Comment 21

[Ray Lin[:ralin]]

s/do does/so does/

Comment 22

[Ray Lin[:ralin]]

Comment on attachment 8933870 [details]
Bug 1415022 - Provide additional description message that help user to indicate the profile to be operated.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204810/diff/5-6/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37c2b0b2cce4
Provide additional description message that help user to indicate the profile to be operated. r=seanlee,steveck

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/37c2b0b2cce4