Closed
Bug 1395028
Opened 7 years ago
Closed 7 years ago
[Form Autofill] Add credit card sync checkbox in its doorhanger
Categories
(Toolkit :: Form Manager, defect, P3)
Toolkit
Form Manager
Tracking
()
VERIFIED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | verified |
People
(Reporter: lchang, Assigned: steveck)
References
(Blocks 1 open bug)
Details
(Whiteboard: [form autofill:M4])
Attachments
(3 files)
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
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → schung
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
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+
Updated•7 years ago
|
status-firefox57:
--- → affected
Comment 4•7 years ago
|
||
mozreview-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.
Updated•7 years ago
|
Attachment #8905885 -
Flags: review?(MattN+bmo) → review?(markh)
Comment 5•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 13•7 years ago
|
||
Please land it after all dependencies land. Thanks.
Reporter | ||
Updated•7 years ago
|
status-firefox57:
affected → ---
status-firefox58:
--- → affected
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 23•7 years ago
|
||
mozreview-review |
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+
Comment 24•7 years ago
|
||
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
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5a6350651948 https://hg.mozilla.org/mozilla-central/rev/5ccb5a1de8f6 https://hg.mozilla.org/mozilla-central/rev/1fc7705bb390
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Flags: qe-verify+
QA Contact: gasofie
Comment 26•7 years ago
|
||
Verified as fixed on 58.0b5 with Windows 10x64, Ubuntu 14.4 and MacOS 10.12.6
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•