[Form Autofill] Add addresses sync checkbox in first time saving doorhanger

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: steveck, Assigned: steveck, NeedInfo)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 verified, firefox57 fixed)

Details

(Whiteboard: [form autofill:M4])

Attachments

(2 attachments)

We'll need to add this sync checkbox once the addresses sync is ready.
Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

Hi Mark, do you think we can determine whether the sync engine is available by checking "services.sync.username"(Services.prefs.prefHasUserValue("services.sync.username")), or is there any other pref that I need to check as well(maybe like "services.sync.engine.addresses.available"? But I think user will not see that doorhanger if services.sync.engine.addresses.available is false)
Attachment #8897805 - Flags: feedback?(markh)
Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

That looks fine to me (checking that pref isn't ideal, but it's done in lots of places already). There should be no need to check the .available pref.
Attachment #8897805 - Flags: feedback?(markh) → feedback+
Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

https://reviewboard.mozilla.org/r/169102/#review174756
Attachment #8897805 - Flags: review?(lchang) → review+
Comment on attachment 8898106 [details]
Bug 1390816 - Part 2: Add browser test for sync enabled case.

https://reviewboard.mozilla.org/r/169432/#review174772

It would be better to also test there's no checkbox in the doorhanger when sync isn't logged in.
Attachment #8898106 - Flags: review?(lchang) → review+
Comment on attachment 8898106 [details]
Bug 1390816 - Part 2: Add browser test for sync enabled case.

https://reviewboard.mozilla.org/r/169432/#review174772

I already added in the first test (verify if the checkbox is hidden).
Comment on attachment 8898106 [details]
Bug 1390816 - Part 2: Add browser test for sync enabled case.

https://reviewboard.mozilla.org/r/169432/#review174772

Oh I see! I misunderstood it. Sorry.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/839885df7077
Part 1: Add addresses sync checkbox in first time saving doorhanger. r=lchang
https://hg.mozilla.org/integration/autoland/rev/95b1178330d5
Part 2: Add browser test for sync enabled case. r=lchang
https://hg.mozilla.org/mozilla-central/rev/839885df7077
https://hg.mozilla.org/mozilla-central/rev/95b1178330d5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

Approval Request Comment
[Feature/Bug causing the regression]: Address autofill with sync
[User impact if declined]: No indication that address sync is an option from the FTU doorhanger
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Verification would be good
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: Uses existing checkbox doorhanger APIs
[String changes made/needed]: Yes but these strings are localized on pontoon and not part of the tree so they don't affect string freeze. We also aren't localizing at all for 56.
Attachment #8897805 - Flags: approval-mozilla-beta?
Hi flod,
Do you have any concerns about the string change part?
Flags: needinfo?(francesco.lodolo)
Hi Vance,
Can you help find a QA to verify this issue?
Flags: needinfo?(vchen)
(In reply to Gerry Chang [:gchang] from comment #13)
> Do you have any concerns about the string change part?

No. As Matt explained, at the moment Autofill is not exposed to localization at all, and it will be as an external repo in the future.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8897805 [details]
Bug 1390816 - Part 1: Add addresses sync checkbox in first time saving doorhanger.

Adding to the UI for the form autofill onboarding.
Let's uplift this for beta 7 and verify in beta once it lands.
Attachment #8897805 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/2c15a6ed11f7
Sorry about the unfortunate landing order of these patches. You can thank our l10n commit hook for that.

https://hg.mozilla.org/releases/mozilla-beta/rev/02852b5a5b3c
Flags: in-testsuite+
Verified that the check is correctly displayed in the doorhanger using Firefox 56 beta 12 across platforms (macOS 10.12.6, Ubuntu 16.04 32bit and Windows 64bit).
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.