Should add the Firefox Sync tour into the onBoarding overlay

Hello Eoger, I have some questions about using syncing login page; if you know the detail, may you give me some advise or forward it to someone who knows it better? We're going to have an iframe in both about:home and about:newtab page, allowing user to create or sign-in sync account there; once user logged in from the iframe, the browser should also be logged in too (e.g. in browser preference page it should displays user is logged in). I tried to do the login through https://accounts.firefox.com/signup?service=sync&context=fx_desktop_v3 rather than about:accounts in the iframe, given that we're going to move newtab page to content process and not able to embed an about: URI from a content page. But some problems arise: 1. in about:home, the page doesn't allow me to embed it in an iframe, given the page denies X-Frame-Options. However in about:newtab it works (And I don't know why the two pages act different). If this is not the correct way to do the login, what's the correct way for it? 2. I need to listen to successful login event on about:home or about:newtab after login is successful, so for now I decide to observe fxaccounts:onverified from chrome process and send it by async message. But I wonder if there's a way to do it directly from a content page?

This is probably a question for the FxA folks, redirecting to rfkelly.

> We're going to have an iframe in both about:home and about:newtab page, > allowing user to create or sign-in sync account there; once user logged in from the iframe, > the browser should also be logged in too +Alex and +Ryan to consider this holistically from an FxA product perspective. > in about:home, the page doesn't allow me to embed it in an iframe, > given the page denies X-Frame-Options. However in about:newtab it works (And I don't know why the two pages act different). I also don't know why this should be different between the two; :rexboy do you have a WIP branch where I could take this for a spin to debug further? > I need to listen to successful login event on about:home or about:newtab after login is successful If you're in web content embedding an iframe, you should be able to listen for postMessage events from the iframe in the same way that the current first-run page does: https://github.com/mozilla/fxa-content-server/blob/master/docs/relier-communication-protocols/firstrun.md

I also want to check the security properties of this setup. What other sort of code gets run in about:home and about:newtab? Do they pull in any third-party javascript? It may be worth looping in some security folks to ensure we're not opening up any exciting new attack surfaces here.

Also, are there mockups where we can review the proposed experience from a UX perspective?

(In reply to Ryan Kelly [:rfkelly] from comment #3) > +Alex and +Ryan to consider this holistically from an FxA product > perspective. > The spec and prototype for the new onboarding tour for your reference: https://mozilla.invisionapp.com/share/2HB9YE939#/screens/230316106 > > in about:home, the page doesn't allow me to embed it in an iframe, > > given the page denies X-Frame-Options. However in about:newtab it works (And I don't know why the two pages act different). > > I also don't know why this should be different between the two; :rexboy do > you have a WIP branch where I could take this for a spin to debug further? > See the sample patch if you need: https://gist.github.com/rexboy7/26d7aaddd4c5a30c6a644c70b0667ee0 After applying it just click on the fox icon at left-top side of about:home and about:newtab, and you'll see the iframe embedded there. I thought the blocking was controlled from server side, but after your answer I suspect that it runs okay only on about:newtab just because about:newtab is under chrome process while about:home isn't. Actually the X-Frame-Options of the login page received from about:newtab is still DENY but it got the page loaded normally. Looks like it just ignores the X-Frame-Options specified from server side. And we found that the login page did whitelisted X-Frame-Options to this page: https://www.mozilla.org/en-US/firefox/51.0/firstrun/?f=99 in the HTML header. but if we are going to whitelist an local URI (namely about:home), I'm not sure if it's possible? > > I need to listen to successful login event on about:home or about:newtab after login is successful > > If you're in web content embedding an iframe, you should be able to listen > for postMessage events from the iframe in the same way that the current > first-run page does: > > > https://github.com/mozilla/fxa-content-server/blob/master/docs/relier- > communication-protocols/firstrun.md This part sounds good to me. thanks for the information!

More description for the "onboarding tour spec" I pasted above: The spec is a proposal for first-time use experience on version 56. We want to attach a dialog on both about:home and about:newtab that contains an sync login form to promote the syncing feature after they install/update their browser. If technically embedding an iframe is not possible we did discussed a fall-back plan, which is open about:accounts on another tab through UI-Tour's library. But we still want to know whether embedding an iframe, which is more ideal, is possible in these two page.

(In reply to Ryan Kelly [:rfkelly] from comment #4) > I also want to check the security properties of this setup. What other sort > of code gets run in about:home and about:newtab? Do they pull in any > third-party javascript? It may be worth looping in some security folks to > ensure we're not opening up any exciting new attack surfaces here. All the codes under about:home and about:newtab are local codes came from inside Firefox, so I don't think they load any 3rd party libraries for now. In the future version about:home will go to Activity Stream and as far as I know the 3rd party library they used is React. Seems Paul is in PTO :-/ I can try to find some other security folks' opinion.

Update: after some internal discussion we'll just going to go for the B plan so clearing ni?, reason follows.

(In reply to KM Lee [:rexboy] from comment #9) > Update: after some internal discussion we'll just going to go for the B plan > so clearing ni?, reason follows. s/internal/offline/. We are in the same company there isn't any "internal" discussion :P. The plain and simple reason is that it's unlikely we can defeat our own security model. I also don't think about:home or about:newtab has an valid origin that X-Frame-Options can whitelist (`location.origin` returns null, assuming this is the valid way to test). Let's just do this as what's being referred as "Plan B".

Thanks a lot for the description Tim. Update for lockdown string of the Sync tour from bug 1354707 comment 14: > > [6: Sync] > > Navigation link: Firefox Sync > > Headline: Sync brings it all together. > > Body: Access your bookmarks and passwords on any device. You can even send a > > tab from your laptop to your phone! Better yet, you can choose what you sync > > and what you don't.

Hello Kelly: The attachment is the screenshot for plan B: after user fill up an email in the form, we are going to open about:accounts in a new tab for them to sign in through UITour. Per UX's opinion, we need to pass the email user entered into the sign-in form of about:accounts. As far as I know, although I can let UITour generate a URL with additional parameter like about:accounts?action=signup&account=foo@bar.com, but the form embedded in about:accounts is still an iframe pointing to https://accounts.firefox.com/signup site. So my question is, do we have an obvious way to set the user name of the form from outside the sign-in iframe? If there isn't, do you have any suggestion way to do it?

I just knew we can use email=email@email.com in URL as argument. To trigger it programmatically from UITour I made a patch to make it possible. Not sure if there are other things to be noticed. Matt and Kelly may you have some feedback for me. Thanks! This patch just changed the library part which means it needs some test code for testing it. I'll attach a patch with UI codes if someone need to test it.

This is a testable patch on about:home. Just apply it, compile, then open about:home and click on the "Opening Account Page With Specified Email" button and it should work.

> I just knew we can use email=email@email.com in URL as argument. Yep, I can confirm that something like this should work: about:accounts?action=signup&email=test@example.com And if it doesn't, that's a bug for our team to fix.

Comment on attachment 8877206 [details] [diff] [review] Patch for UITour This looks fine to me, although I won't pretend to have much detailed expertise in the client code here.

Thanks for confirming with the usage Kelly. I'm making a full patch for the UItour, I'll just update it later.

Comment on attachment 8877206 [details] [diff] [review] Patch for UITour bug 1357026 may be more suitable for discussing UITour's patch. Full patch has been sent there.

visual spec sync tour page https://bug1354707.bmoattachments.org/attachment.cgi?id=8877008 overall overlay https://mozilla.invisionapp.com/share/2HB9YE939#/screens/230316106

This patch is based on bug 1357023 and bug 1357026, you may need to apply them before trying (if they're not landed yet) :Mossop and :Flod may you take a look on this patch?

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review154446 Besides the Firefox Sync/Account branding issue, there's a much worse localizability problem: you're imposing the English sentence structure to all languages. Basically code (and more important design) assumes that you can translate this in a natural way. Create a Firefox Account to continue to Firefox Sync While most languages won't be able to do it. What if I need to do this? To continue to Firefox Sync create a Firefox Account Emphasis (i.e. bigger font) would end up on the wrong sentence. Or what if I need something like to place "create a Firefox Account" in the middle of a sentence? A more localizable design would be to avoid splitting the sentence and make the title self-contained, as the markup assumes. <h3>Firefox Account</h3> <p><strong>Create a Firefox Account</strong> to continue to Firefox Sync</p> ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:27 (Diff revision 2) > onboarding.tour-private-browsing.description=Browse the internet without saving your searches or the sites you visited. When your session ends, the cookies disappear from %S like they were never there. > onboarding.tour-private-browsing.button=Show Private Browsing in Menu > + > +# LOCALIZATION NOTE(onboarding.tour-sync): %S is > +# brandShortName. > +onboarding.tour-sync=%S Sync Firefox Sync is one of the few cases where you want Firefox hard-coded. Even on Nightly it should still say Firefox Sync. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:32 (Diff revision 2) > +onboarding.tour-sync=%S Sync > +onboarding.tour-sync.title=Sync brings it all together. > +onboarding.tour-sync.description=Access your bookmarks and passwords on any device. You can even send a tab from your laptop to your phone! Better yet, you can choose what you sync and what you don’t. > + > +# LOCALIZATION NOTE(onboarding.tour-sync.form.title): %S is brandShortName. > +onboarding.tour-sync.form.title=Create a %S Account Same here: Firefox Account ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:36 (Diff revision 2) > +# LOCALIZATION NOTE(onboarding.tour-sync.form.title): %S is brandShortName. > +onboarding.tour-sync.form.title=Create a %S Account > + > +# LOCALIZATION NOTE(onboarding.tour-sync.form.description): %S is > +# brandShortName. > +onboarding.tour-sync.form.description=to continue to %S Sync Firefox Sync.

ni? Verdi per l10n suggestions by :Flod in comment 23. Can we change current string to a more self-contained title and sentence to meet l10n requirements?

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review155244 I'll look this over more once the l10n issues are resolved. ::: browser/extensions/onboarding/content/onboarding-tour-agent.js:21 (Diff revision 2) > Mozilla.UITour.openSearchPanel(() => {}); > break; > + case "onboarding-tour-sync-button": > + let emailInput = document.getElementById("onboarding-tour-sync-email-input"); > + Mozilla.UITour.showFirefoxAccounts(null, emailInput.value); > + preventDefault(); Do you mean event.preventDefault() ? Why is that required here but not in other cases?

(In reply to KM Lee [:rexboy] from comment #24) > ni? Verdi per l10n suggestions by :Flod in comment 23. Can we change current > string to a more self-contained title and sentence to meet l10n > requirements? The thing I'm trying to have happen is to make our small form with just an email field, match the full form that we'll be taking people to. So I'd like the string here to match what will be displayed on the form. To that end I think we should try to do what the FxA team does here: Use "Create a Firefox account to continue to Firefox Sync" where possible. Where this doesn't make sense for the language, we can use these two separate stings: "Create a Firefox Account" "Continue to Firefox Sync"

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review154446 Based on Comment 26, I think the conclusion for us is use self-contained sentences for description if combining the two to make the sentences is not possible. I updated some instructions on the localization note. May you take a look?

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review155244 > Do you mean event.preventDefault() ? Why is that required here but not in other cases? This was to prevent the form action before.. but it should be no necessary given we're using the UITour and no "opening in other tab" case. I've removed it.

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review156060 It's OK with the suggested localization comments. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:51 (Diff revision 4) > + > +onboarding.tour-sync=Firefox Sync > +onboarding.tour-sync.title=Sync brings it all together. > +onboarding.tour-sync.description=Access your bookmarks and passwords on any device. You can even send a tab from your laptop to your phone! Better yet, you can choose what you sync and what you don’t. > + > +onboarding.tour-sync.form.title=Create a Firefox Account Please add a localization note here too # LOCALIZATION NOTE(onboarding.tour-sync.form.title): This string is displayed # as a title and followed by onboarding.tour-sync.form.description. # Your translation should be consistent with the form displayed in # about:accounts when signing up to Firefox Account. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:52 (Diff revision 4) > +onboarding.tour-sync=Firefox Sync > +onboarding.tour-sync.title=Sync brings it all together. > +onboarding.tour-sync.description=Access your bookmarks and passwords on any device. You can even send a tab from your laptop to your phone! Better yet, you can choose what you sync and what you don’t. > + > +onboarding.tour-sync.form.title=Create a Firefox Account > +# LOCALIZATION NOTE(onboarding.tour-sync.form.description): The description Please change this to # LOCALIZATION NOTE(onboarding.tour-sync.form.description): The description # continues after onboarding.tour-sync.form.title to create a complete sentence. # If it's not possible for your locale, you can translate this string as # "Continue to Firefox Sync" instead. # Your translation should be consistent with the form displayed in # about:accounts when signing up to Firefox Account.

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. Since Bug 1357641 is fixed and going to land, please rebase and update as below so that the tour notification for Sync would appear: - browser/extensions/onboarding/content/onboarding.js: add one `getNotificationStrings(bundle)` method to return strings for notification - browser/extensions/onboarding/locales/en-US/onboarding.properties: Add strings of Sync tour notification, Please find the strings in [1] - browser/extensions/onboarding/test/browser/head.js: Add Sync tour id into the `TOUR_IDs` array. Please add the id according to the tour order. (`TOUR_IDs` now is not in the Central, but will appear after Bug 1357641 is landed) [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1357668#c4 Thank you

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review156094 ::: browser/extensions/onboarding/content/onboarding.js:345 (Diff revision 4) > let li = this._window.document.createElement("li"); > - li.textContent = this._bundle.GetStringFromName(tour.tourNameId); > + // We always put brand short name as the first argument for it's the > + // only and frequently used arguments in our l10n case. Rewrite it if > + // other arguments appears. > + li.textContent = this._bundle.formatStringFromName( > + tour.tourNameId, [BRAND_SHORT_NAME], 1); we don't need pass the browser name since now `Firefox Sync` is a fixed string

(In reply to KM Lee [:rexboy] from comment #33) > Created attachment 8879856 [details] > screenshot for notification after rebasing Thank you for this. Just found that forgot to mention onboarding.css requires update as well.

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. Ok, so we got rebased and have notifications now. Fischer: may you check again if it works for you. flod: because the rebase we got two additional strings you may want to take a look again: onboarding.notification.onboarding-tour-sync.title onboarding.notification.onboarding-tour-sync.message Mossop: The polyfill is just the same patch for bug 1357641, so in case you need to try it you can pick one of them to apply. Thank you guys!

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review156182

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. https://reviewboard.mozilla.org/r/149784/#review156360

Comment on attachment 8878433 [details] Bug 1357023 - Add onboarding tour for Firefox Sync. Thanks you looks good to me

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1f298a00b83&selectedJob=109062578 looks fine on try server

(In reply to KM Lee [:rexboy] from comment #43) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c1f298a00b83&selectedJob=109062578 > looks fine on try server Just a note that this bug should be checked-in after the bug 1357641.

The Polyfill patch is blocking Autoland from pushing this. Commit message w/o bug number, lack of review, etc.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 11817274916f -d e88dabd01b07: rebasing 403622:11817274916f "Bug 1357023 - Add onboarding tour for Firefox Sync. r=flod,mossop" (tip) merging browser/extensions/onboarding/content/onboarding-tour-agent.js merging browser/extensions/onboarding/content/onboarding.css merging browser/extensions/onboarding/content/onboarding.js merging browser/extensions/onboarding/locales/en-US/onboarding.properties merging browser/extensions/onboarding/test/browser/head.js warning: conflicts while merging browser/extensions/onboarding/content/onboarding-tour-agent.js! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/extensions/onboarding/content/onboarding.css! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/extensions/onboarding/locales/en-US/onboarding.properties! (edit, then use 'hg resolve --mark') warning: conflicts while merging browser/extensions/onboarding/test/browser/head.js! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

cannot land see comment 46

Seems that bug 1357641 has been backed out :-/. The patch is rebased on it so let's wait for it to be landed again. I'll send checkin-needed after that. Thanks for help.

Rebased with latest patch of bug 1357641. It should be okay landing onto autoland now. Looks okay on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5889ebb4b4ab

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5bad40321f1f Add onboarding tour for Firefox Sync. r=flod,mossop

This issue has been verified on Win 10 x64.