[Form Autofill] Add credit card sync checkbox in its doorhanger

VERIFIED FIXED in Firefox 58

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: lchang, Assigned: steveck)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox58 verified)

Details

(Whiteboard: [form autofill:M4])

Attachments

(3 attachments)

According to UX spec [1], there should be a checkbox to enable the credit card sync in its door hanger when a user has enabled sync feature. Note that "Don't Save" button in the door hanger should be disabled if a user turns on the credit card sync.


[1] https://mozilla.invisionapp.com/share/7ZA4WEK9W#/screens/215537981
Depends on: 1395122
Assignee: nobody → schung
Depends on: 1395123
Comment on attachment 8905885 [details]
Bug 1395028 - Part 1: Add credit card sync checkbox in its doorhanger.

https://reviewboard.mozilla.org/r/177696/#review183090

Looks good. Thanks.
Attachment #8905885 - Flags: review?(lchang) → review+
Comment on attachment 8905885 [details]
Bug 1395028 - Part 1: Add credit card sync checkbox in its doorhanger.

https://reviewboard.mozilla.org/r/177696/#review183860

::: browser/extensions/formautofill/FormAutofillDoorhanger.jsm:122
(Diff revision 2)
> +        get checked() {
> +          return Services.prefs.getBoolPref("services.sync.engine.creditcards");
> +        },
> +        get label() {
> +          // If sync account is not set, return null label to hide checkbox
> +          return Services.prefs.prefHasUserValue("services.sync.username") ?
> +            GetStringFromName("creditCardSyncCheckbox") : null;
> +        },
> +        callback(event) {
> +          let {secondaryButton, menubutton} = event.rangeParent.parentNode.parentNode;
> +          let checked = event.target.checked;
> +          Services.prefs.setBoolPref("services.sync.engine.creditcards", checked);

I would like Mark to review this patch, specifically the parts dealing with the two prefs to ensure that this is the best way to do this and that it properly handles a user without sync setup or with sync disconnected.
Attachment #8905885 - Flags: review?(MattN+bmo) → review?(markh)
Comment on attachment 8905885 [details]
Bug 1395028 - Part 1: Add credit card sync checkbox in its doorhanger.

https://reviewboard.mozilla.org/r/177696/#review184010

::: browser/extensions/formautofill/locales/en-US/formautofill.properties:18
(Diff revision 2)
>  changeAutofillOptions = Change Form Autofill Options
>  autofillOptionsLinkOSX = Form Autofill Preferences
>  autofillSecurityOptionsLinkOSX = Form Autofill & Security Preferences
>  changeAutofillOptionsOSX = Change Form Autofill Preferences
>  addressesSyncCheckbox = Share addresses with synced devices
> +creditCardSyncCheckbox = Share credit card with synced devices

The grammar here seems wrong - it should probably be "Share credit cards with synced devices" or similar.
Attachment #8905885 - Flags: review?(markh) → review+
Comment on attachment 8907515 [details]
Bug 1395028 - Part 2: Add cc doorhanger sync checkbox mochitest.

https://reviewboard.mozilla.org/r/179220/#review184780

This patch reminds me that the sync checkbox seems not necessary if sync is already enabled. Could you confirm this with UX? Thanks.

::: browser/extensions/formautofill/test/browser/browser_creditCard_doorhanger.js:100
(Diff revision 1)
> +      cb.click();
> +      is(SpecialPowers.getBoolPref(SYNC_CREDITCARDS_PREF), true,
> +         "creditCards sync should be enabled after checked");
> +      is(secondaryButton.disabled, true, "Not saving button should be disabled");
> +      is(menuButton.disabled, true, "Never saving menu button should be disabled");

Let's click it again to verify the reverting case.
Attachment #8907515 - Flags: review?(lchang)
Comment on attachment 8907515 [details]
Bug 1395028 - Part 2: Add cc doorhanger sync checkbox mochitest.

https://reviewboard.mozilla.org/r/179220/#review185838

Looks good. Thanks.
Attachment #8907515 - Flags: review?(lchang) → review+
Please land it after all dependencies land. Thanks.
Comment on attachment 8922215 [details]
Bug 1395028 - Part 3: Hide the sync checkbox if credit card sync is unavailable.

https://reviewboard.mozilla.org/r/193232/#review198490

Thanks.
Attachment #8922215 - Flags: review?(lchang) → review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a6350651948
Part 1: Add credit card sync checkbox in its doorhanger. r=lchang,markh
https://hg.mozilla.org/integration/autoland/rev/5ccb5a1de8f6
Part 2: Add cc doorhanger sync checkbox mochitest. r=lchang
https://hg.mozilla.org/integration/autoland/rev/1fc7705bb390
Part 3: Hide the sync checkbox if credit card sync is unavailable. r=lchang
Flags: qe-verify+
QA Contact: gasofie
Verified as fixed on 58.0b5 with Windows 10x64, Ubuntu 14.4 and MacOS 10.12.6
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.