Closed Bug 1557635 Opened 4 months ago Closed 3 months ago

Update the Sync onboarding screen's image and title (to "ACCOUNT")

Categories

(Firefox for Android :: First Run, defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 69
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 68+ verified
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 - wontfix
firefox69 --- verified

People

(Reporter: cpeterson, Assigned: diana.iacos)

References

Details

(Whiteboard: [bcs:p1] [trailhead:fennec] [fennec68.0.2])

Attachments

(2 files)

Bug 1555044 landed the new onboarding strings:

  • updatednewfirstrun_sync_subtext ("Sign in to your account to get the most out of &brandShortName;")
  • updatednewfirstrun_signin_button ("Sign in to &brandShortName;")

We need to use these new strings in Fennec's onboarding code (like we did for the new onboarding strings for EU Browser Choice in Fennec 67 bug 1545805).

Assignee: nobody → diana.iacos
Status: NEW → ASSIGNED
Attached image Sync - Fennec.png

Updated imagery for the updated Sync screen.
Can be added beside the existing ones in /mobile/android/app/src/main/res/drawable-nodpi but we'll need to also have a fallback strategy to the old images depending on the localization status of the sync strings.

Summary: Use the new "Sign in to" strings in Fennec onboarding → Update the Sync onboarding screen

In addition to changing the third card's image, we also need to change the third card's title from SYNC to ACCOUNT, as shown in this design mockup:

https://docs.google.com/presentation/d/1fv1EdAxmtHt6jypDgp8UQt0tN9K_WQW2sRdlfLss9GI/edit?ts=5cf04c70#slide=id.g58125d67e0_5_84

Summary: Update the Sync onboarding screen → Update the Sync onboarding screen's image and title (to "ACCOUNT")

With this changes we must now support 3 Onboarding versions.

Latest Onboarding UX will use a new title for the "Sync screen", new subtext,
new image and new text for the signin button.
This will be presented only if all this new Strings are localized.

Refactored the existing OnboardingStringUtil to serve as a central point of
getting the right Strings to be used and querying the Onboarding UX version the
app should offer.

Applied a lossless compression for the new sync image which resulted in a 26.5%
size reduction.

Removed the lint suppression initially necessary for when first added the
updated Sync Strings which were not used at the moment.

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc09b9a55c48
Update the Sync Onboarding screen; r=VladBaicu

Keywords: checkin-needed

Backed out changeset cc09b9a55c48 (bug 1557635) for lint failure at android_strings.dtd on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/5b1724bafc9387044cc794e9f69c31f151b26fd0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=251903522&revision=cc09b9a55c4825e055b32c2c2e812e555283cb56

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251903522&repo=autoland&lineNumber=88

Log snippet:
[task 2019-06-14T15:40:51.450Z] updating to branch default
[task 2019-06-14T15:40:51.761Z] 1029 files updated, 0 files merged, 0 files removed, 0 files unresolved
[task 2019-06-14T15:40:51.891Z] 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
[task 2019-06-14T15:40:56.533Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/base/locales/en-US/android_strings.dtd:24:1 | Duplicate string with ID: firstrun_account_title (l10n)
[task 2019-06-14T15:40:56.533Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/base/locales/en-US/android_strings.dtd:45:1 | Duplicate string with ID: firstrun_account_title (l10n)
[taskcluster 2019-06-14 15:40:56.916Z] === Task Finished ===
[taskcluster 2019-06-14 15:40:57.022Z] Artifact "public/code-review/mozlint.json" not found at "/builds/worker/mozlint.json"
[taskcluster 2019-06-14 15:40:57.494Z] Unsuccessful task run with exit code: 1 completed in 102.31 seconds

Flags: needinfo?(petru.lingurar)

Thanks Raul! Now all should be good.

Flags: needinfo?(petru.lingurar)
Keywords: checkin-needed

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fc4b64bb62b
Update the Sync Onboarding screen; r=VladBaicu

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Flags: qe-verify+
Keywords: checkin-needed

[Tracking Requested - why for this release]:

Petru, can QA test these new onboarding strings and image before we uplift? Since the Fennec Nightly channel in the Google Play Store is actually version 68 Beta, QA can probably install a Fennec 69 build from Treeherder.

It's probably too late to uplift these UI changes to Fennec 68 Beta, but we'll want to uplift them to the Fennec ESR channel for the ESR 68.1 release (2019-09-03).

Flags: needinfo?(petru.lingurar)
OS: Unspecified → Android

Verified that the Sync screen is now updated on the Nightly 69.0a1 (2019-06-20).
Devices:

  • Nokia 6 (Android 7.1.1);
  • Xiaomi Mi 8 Lite (Android 8.1);
  • OnePlus 5T (Android 9);
  • Google Pixel (Android Q).
Flags: qe-verify+
Flags: needinfo?(petru.lingurar)

Comment on attachment 9072137 [details]
Bug 1557635 - Update the Sync Onboarding screen; r?VladBaicu

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Updated the Onboarding UX.
    Probably too late to uplift in Beta so it should be part of ESR.
  • User impact if declined: Users would see an older version of our Onboarding
  • Fix Landed on Version: 69
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch is only touching the Onboarding. Verified by QA.
  • String or UUID changes made by this patch:
Attachment #9072137 - Flags: approval-mozilla-esr68?

[Tracking Requested - why for this release]:

We will want to uplift this Fennec fix to the ESR 68 branch for the Fennec 68.1 release.

Comment on attachment 9072137 [details]
Bug 1557635 - Update the Sync Onboarding screen; r?VladBaicu

Updates to the Fennec onboarding UX. Approved for Fennec 68.1b1.

Attachment #9072137 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Comment on attachment 9072137 [details]
Bug 1557635 - Update the Sync Onboarding screen; r?VladBaicu

Actually, holding off on this for now until I can confirm that the l10n bumper is ready to go on ESR68.

Attachment #9072137 - Flags: approval-mozilla-esr68+ → approval-mozilla-esr68?

Comment on attachment 9072137 [details]
Bug 1557635 - Update the Sync Onboarding screen; r?VladBaicu

Take two, approved for 68.1b2! This will also be in tomorrow's Nightly builds if QA wants to get a jump start on verifying.

Attachment #9072137 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Hi, verified as fixed on Fennec 68.1b2 build 1 with devices:

  • Sony Xperia Z5 (Android 7.0)
  • Google Pixel 3a XL (Android 9)
  • Samsung Galaxy S9 (Android 8.0.0)
  • Sony Xperia Z3 (Android 5.1.1)
Status: RESOLVED → VERIFIED
Hardware: Unspecified → ARM

Thanks for verifying this on 68.1b2. Per discussion with cpeterson, approved for Fennec 68.0.2 as well. We'll want this to be verified again on those builds once ready, likely later this week.

FIREFOX_ESR_68_0_X_RELBRANCH:
https://hg.mozilla.org/releases/mozilla-esr68/rev/873471c5d85082ac97907ed830622f2721c89104

Petru, Ryan merged your onboarding patch to the ESR 68.0.x relbranch, but there is some build bustage because some code from top site bug 1555950 is not in the 68.0.x relbranch.

Is there code from top site bug 1555950 that we need to merge the 68.0.x relbranch? Or can you just manually rebase your patch on the relbranch and push a new patch up to Phabricator for Ryan to merge?

https://hg.mozilla.org/releases/mozilla-esr68/shortlog/FIREFOX_ESR_68_0_X_RELBRANCH

Flags: needinfo?(petru.lingurar)

Here's the error message:
Exception: Could not find drawable in '/builds/worker/workspace/build/src/mobile/android/app/src/main/res' for 'aliexpress'

So I think it isn't actually bustage from this bug per se, but bustage from the corresponding l10n-changesets.json bump that was landed on the relbranch at the same time. In particular, it appears that the second patch in bug 1555950 is adding the missing assets being complained about?

Is it enough to just land that patch (and the third one to make the linter happy)? It appears to be according to Try, though I'm not sure what the consequences of that would be.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78134786e34b3ddfeb8f530509f9f3044f09915e

Things are also green on Try if I graft all 3 patches from bug 1555950 to the relbranch, so maybe that'd be an option otherwise.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a2402e946976ec8a32400fa738a383cc17fb14d

This ticket - it's patch should not depend in any on the localized top sites.

What I understand though is that whatever build is failing with that error would be a multi-locale one containing the configs from bug 1564319.
Those new configs would enable localized top sites depending on user's locale, top sites which would try to use local drawables when shown in the awesomescreen.
So if the updated l1on packages (with the configs from bug 1564319) are part of release definitely the first and second patches from bug 1555950 should also be.
(Patch 3 of bug 1555950 was necessary if these patches were to be landed before the l1on changes in bug 1564319 and so a lint error would be thrown because no code would actually use them. I think same would happen though with single-locale builds so patch 3 is also needed).

With this green try https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a2402e946976ec8a32400fa738a383cc17fb14d and the recent green PI test I'd say we're confident to land bug 1555950 but this would mean Localized Top Sites would be available right away in this release version, though they were tentatively planned for the 68.1 release - bug 1555950 comment 33
Otherwise we would need to revert the changes from bug 1564319.

NI-ing Chris to weigh in on enabling Localized Top Sites now or later.

Flags: needinfo?(petru.lingurar)
Flags: needinfo?(cpeterson)
QA Whiteboard: [qa-triaged]

(In reply to Petru-Mugurel Lingurar[:petru] from comment #21)

NI-ing Chris to weigh in on enabling Localized Top Sites now or later.

I don't see any problem, from a product perspective, with enabling the Localized Top Sites now (in ESR 68.0.x dot release) if the patches all apply cleanly.

Flags: needinfo?(cpeterson)
Whiteboard: [bcs:p1] [trailhead:fennec] → [bcs:p1] [trailhead:fennec] [fennec68.0.2]

Hi, verified as fixed on Firefox 68.1b7 using the following devices:
• Google Pixel 3a (Android 9)
• Galaxy S9 (Android 8.0.0)
• Samsung Galaxy S7 (Android 7)

Flags: qe-verify+

Hi, verified as fixed on Firefox RC 68.1 and Firefox RC 68.0.2 with

  • Google Pixel 3 XL (Android 9)
  • Samsung Galaxy Note 9 (Android 8.1.0)
  • Sony Xperia Z5 (Android 7.0)

Will remove the [qa-triaged].

QA Whiteboard: [qa-triaged]
You need to log in before you can comment on or make changes to this bug.