Closed Bug 1415022 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Autofill, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: selee, Assigned: ralin)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [form autofill:V2])

Attachments

(1 file)

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.
Component: Form Manager → Form Autofill
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.
Flags: needinfo?(mliang)
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?
Flags: needinfo?(mliang) → needinfo?(selee)
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.
Flags: needinfo?(selee) → needinfo?(mliang)
Depends on: 1415073
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
Flags: needinfo?(mliang)
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Blocks: 1421173
Any idea can make bug 1421173 less painful and about description, is also welcome here. :D
Summary: [Form Autofill] Adjust the door hanger showing logic to satisfy the multiple section design → [Form Autofill] Provide a profile summary in a door hanger for recognizing the profiles
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 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 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.
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 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 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 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).
Attachment #8933870 - Flags: review?(schung) → review+
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 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");
  ...
}
```
Attachment #8933870 - Flags: review?(selee) → review+
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
s/do does/so does/
Pushed by ralin@mozilla.com:
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
Blocks: 1425526
https://hg.mozilla.org/mozilla-central/rev/37c2b0b2cce4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.