Closed Bug 957425 Opened 6 years ago Closed 6 years ago

implement entry-point into setting up fxa-based sync into about:accounts

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: markh, Assigned: zaach)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file, 4 obsolete files)

For fxa-based sync, we desire a new page which serves as an entry-point into the process of setting up sync.  The content of this page will be, roughly:

"""
[pretty image]
Sign in to backup and sync your tabs, bookmarks and more wherever you use Firefox!

[Get started] (<- a button that will open about:accounts)

<small font>Using sync on an old Fx version [Click Here] (<- a button that will let you pair to an existing sync account)
"""

This page will be shown whenever the existing "setup sync" page is currently shown.  In the standard "options" dialog, this page will be shown in the dialog, but in all other cases it will be shown in a tab.  In all cases, clicking the [Get Started] button will open about:accounts in a tab (and close the options dialog if open)

CCing jgruen as (IIUC) he owns the exact styling and wording.
oh - "about:sync-setup" is just a strawman - I guess it could also be a chrome:// URL or anything else that makes sense - but the current understanding is that it will be all local content without any hosted parts.
Whiteboard: [qa+]
This patch is a very rough skeleton for about:sync-setup.  It needs love from a designer (and presumably an image or 2).  It is very much a WIP, but if someone wants to take this and add styling etc, please go for it!

Note also that the new about page probably can't use URI_SAFE_FOR_UNTRUSTED_CONTENT as it opens the preferences dialog, which (according to AboutRedirector.cpp) means this patch will need security review when it's ready.
https://cloudup.com/cwMNEeXPdO8 (from jgruen's https://www.dropbox.com/s/522cjy4pxpeaoht/sync_flow_1_8_step_thru.pdf) looks like a fairly high-fidelity mockup. Assuming we're happy with that look, styling should be relatively straightforward.

John, can you provide the source image on this bug?

(That AboutRedirector comment is ignorable, that horse has already left the barn.)
Flags: needinfo?(jgruen)
Hey guys, I'll be meeting with brand identity tomorrow to discuss finalizing the visual design. I'd also recommend pinging Nick Champman as he's writing the front end code for FxA and ideally the sync page could use the same CSS
Flags: needinfo?(jgruen)
Still somewhat of a WIP - obviously needs styling, and I just this minute learned what the usual practice is for avoiding markup in the DTD (so please assume I'll fix that) - but early-feedback is good feedback...
Attachment #8356989 - Attachment is obsolete: true
Attachment #8357631 - Flags: feedback?(ttaubert)
Looks like the styling we can steal lives in https://github.com/mozilla/fxa-content-server.

That does raise a potential concern, though - if the goal is to match the styling of the remote account signup flow, we run the risk of things getting out of sync if the remote styling is changed (website can be updated much more easily than Firefox). 

This probably isn't a huge deal either way, but one was to address it would be to also host this page remotely, so that they can actually just share the same styling. It also avoids discrepancies between localization of server vs. Firefox content.
Also note: per https://mail.mozilla.org/pipermail/sync-dev/2014-January/000687.html and https://wiki.mozilla.org/User_Services/Sync/Datatype_Selection_Fx29, the text on this page should be clear about what data will be synced (so more complete than the text in comment 0).
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #6)
> This probably isn't a huge deal either way, but one was to address it would
> be to also host this page remotely, so that they can actually just share the
> same styling. It also avoids discrepancies between localization of server
> vs. Firefox content.

ckarlof points out that this content needs to live in the preferences pane as well, so this could be awkward. We can live with styling diverging slightly.
Giving this to mhammond for now, but might tag Tim to help with the styling of this (and integration with bug 957426).
Assignee: nobody → mhammond
Comment on attachment 8357631 [details] [diff] [review]
0001-Bug-957425-add-about-sync-setup-for-entry-point-into.patch

Review of attachment 8357631 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/sync/aboutSyncSetup.css
@@ +1,2 @@
> +#oldsync {
> +    font-size: 50%;

Nit: indentation seems off.

::: browser/base/content/sync/aboutSyncSetup.js
@@ +36,5 @@
> +}
> +
> +// Button onclick handlers
> +function handleOldSync() {
> +  // if this is in a tab, do we close it?

If this is in a tab, shouldn't we just re-use it?

::: browser/base/content/sync/aboutSyncSetup.xhtml
@@ +7,5 @@
> +  %htmlDTD;
> +  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
> +  %brandDTD;
> +  <!ENTITY % aboutAccountsDTD SYSTEM "chrome://browser/locale/aboutSyncSetup.dtd">
> +  %aboutAccountsDTD;

Nit: aboutSyncSetupDTD;

@@ +20,5 @@
> +     href="chrome://browser/content/sync/aboutSyncSetup.css"
> +     type="text/css" />
> +  </head>
> +  <body>
> +    <p>TODO: a pretty image and styling!</p>

Can't wait. But honestly, I think we should get those core workflow items landed rather sooner than later. We can always uplift style changes.

@@ +27,5 @@
> +    <p id="oldsync">&aboutSyncSetup.useOldSync;</p>
> +    <script type="text/javascript;version=1.8"
> +      src="chrome://browser/content/sync/aboutSyncSetup.js" />
> +    <script type="application/javascript;version=1.8"
> +      src="chrome://browser/content/utilityOverlay.js"/>

Do we really need that file if we have getChromeWindow()? What is that used for?

::: browser/locales/en-US/chrome/browser/aboutSyncSetup.dtd
@@ +2,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY aboutSyncSetup.pageTitle "Welcome to Sync">
> +<!ENTITY aboutSyncSetup.description "Sign in to backup and sync your tabs, bookmarks, and more whereever you use &brandShortName;!">

Nit: wherever. But whatever, that text is going to change anyway.

@@ +3,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY aboutSyncSetup.pageTitle "Welcome to Sync">
> +<!ENTITY aboutSyncSetup.description "Sign in to backup and sync your tabs, bookmarks, and more whereever you use &brandShortName;!">
> +<!ENTITY aboutSyncSetup.getStarted "Get Started">

Nit: instead of ".getStarted" maybe ".startButton.label"?

@@ +4,5 @@
> +
> +<!ENTITY aboutSyncSetup.pageTitle "Welcome to Sync">
> +<!ENTITY aboutSyncSetup.description "Sign in to backup and sync your tabs, bookmarks, and more whereever you use &brandShortName;!">
> +<!ENTITY aboutSyncSetup.getStarted "Get Started">
> +<!ENTITY aboutSyncSetup.useOldSync "Using Sync on an older version of &brandShortName;? <a href='#' onclick='handleOldSync();'>Click Here</a>">

Yeah, as you said. That markup shouldn't be here :) Shouldn't be a big problem to separate the label and the link, no?
Attachment #8357631 - Flags: feedback?(ttaubert) → feedback+
Comment on attachment 8357631 [details] [diff] [review]
0001-Bug-957425-add-about-sync-setup-for-entry-point-into.patch

Review of attachment 8357631 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/sync/aboutSyncSetup.js
@@ +37,5 @@
> +
> +// Button onclick handlers
> +function handleOldSync() {
> +  // if this is in a tab, do we close it?
> +  let url = Services.urlFormatter.formatURLPref("app.support.baseURL") + "sync-migration";

I was just told that the final URL will be https://services.mozilla.com/legacysync
(In reply to Tim Taubert [:ttaubert] from comment #10)
> Nit: indentation seems off.

Done.

> ::: browser/base/content/sync/aboutSyncSetup.js
> @@ +36,5 @@
> > +}
> > +
> > +// Button onclick handlers
> > +function handleOldSync() {
> > +  // if this is in a tab, do we close it?
> 
> If this is in a tab, shouldn't we just re-use it?

Yeah, we do.  The comment was stale, so I removed it.

> Nit: aboutSyncSetupDTD;

Fixed.

> Can't wait. But honestly, I think we should get those core workflow items
> landed rather sooner than later. We can always uplift style changes.

Yep :)

> > +    <script type="application/javascript;version=1.8"
> > +      src="chrome://browser/content/utilityOverlay.js"/>
> 
> Do we really need that file if we have getChromeWindow()? What is that used
> for?

Nope - removed.

> Nit: wherever. But whatever, that text is going to change anyway.

I updated the text to the most recent in the mockups, and the end of the sentence was removed, so problem solved :)

> Nit: instead of ".getStarted" maybe ".startButton.label"?

Done.

> Yeah, as you said. That markup shouldn't be here :) Shouldn't be a big
> problem to separate the label and the link, no?

The recent mockups made this easier too - the link now takes up the entire text, making life easier.

(In reply to Tim Taubert [:ttaubert] from comment #11)
> I was just told that the final URL will be
> https://services.mozilla.com/legacysync

OK, that's there too.
Attachment #8357631 - Attachment is obsolete: true
Attachment #8364076 - Flags: review?(ttaubert)
This is all being done by the new fancy about:accounts
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
OK. Good.
Status: RESOLVED → VERIFIED
Attached patch aboutaccounts-sync.patch (obsolete) — Splinter Review
Here's an updated patch that includes the setup page in about:accounts, along with all the styling. It also handles the ?signin=true query parameter.
Attachment #8364076 - Attachment is obsolete: true
Attachment #8364076 - Flags: review?(ttaubert)
Attachment #8364828 - Flags: review?(mhammond)
oops - closed too early - this but has just changed focus!
Assignee: mhammond → zack.carter
Status: VERIFIED → REOPENED
Resolution: WONTFIX → ---
Summary: implement about:sync-setup for entry-point into setting up fxa-based sync → implement entry-point into setting up fxa-based sync into about:accounts
Comment on attachment 8364828 [details] [diff] [review]
aboutaccounts-sync.patch

Review of attachment 8364828 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with the comments addressed.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +138,5 @@
> +               .QueryInterface(Ci.nsIInterfaceRequestor)
> +               .getInterface(Ci.nsIDOMWindow);
> +}
> +
> +function openLink(url) {

This will no longer be used from the prefs pane IIUC, so this probably needs some tweaks.  It looks like the only caller for this is for the "legacy sync" SUMO page, in which case I expect you just want to replace the current tab with that new URL - so getChromeWindow() could be removed and this just becomes 'window.location = url'.

If it turns out you want a new tab, then you probably want:

getChromeWindow().switchToTabHavingURI(url, true);

@@ +198,5 @@
> +function hide(id) {
> +  document.getElementById(id).style.display = 'none';
> +}
> +
> +init();

I'm surprised we don't need to do this on a load or DOMContentLoaded event?  Maybe it's because the script tags aren't in <head>?  I think a DOMContentLoaded handler would be safer:

document.addEventListener("DOMContentLoaded", function onload() {
  document.removeEventListener("DOMContentLoaded", onload, true);
  init();
}, true);
Attachment #8364828 - Flags: review?(mhammond) → review+
Comment on attachment 8364828 [details] [diff] [review]
aboutaccounts-sync.patch

Review of attachment 8364828 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/aboutaccounts/aboutaccounts.xhtml
@@ +69,5 @@
> +              <a class="button" href="#" onclick="getStarted()">&aboutAccountsSetup.startButton.label;</a>
> +            </div>
> +
> +            <div class="links">
> +              <a id="oldsync" href="#" onclick="handleOldSync();">&aboutAccounts.useOldSync.label;</a>

also, this entity should be &aboutAccountsSetup.useOldSync.label; - note the missing "Setup"
This patch has all my review comments fixed - I was going to land it, but the trees are closed :(
Attachment #8364828 - Attachment is obsolete: true
Attachment #8364860 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d81113054e13
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.