957426 - New "Sync options" page for FxA-based Sync

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

If the browser is configured to use fxax-based sync, a different fxa-specific "sync options" page will need to be shown instead of the existing one.

Current mockups by UX show a different design than the current one, but that's not finalized yet - but at a minimum, the "manage account" area will change, "pair device", "unlink device" etc links will be removed, the TOS and privacy policy are likely to link to different places, etc.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-957426-WIP-sync-preferences-prefer-to-use-about-.patch

This attachment is ontop of the one in bug 957425 - it is *very* much a WIP.  

To see what it does:
* Apply the patch from bug 957425 and this attachment.
* Go to Options -> Sync - note that about:sync-setup (from bug 957425) is shown instead of the normal sync options.  Close it.
* open about:sync-setup in a tab.
* Click on the "click here" link to indicate you want to use an "old" sync account.
* Note the Sync Options dialog opens - but now it is showing the "old" sync preference pane.
* Note the new link "Use Firefox Accounts to configure Sync" - click it.
* Note the preference pane changes back to showing about:sync-setup
* Click again on the link to indicate you want to use the "old" sync (but this time you are clicking it in the dialog, not the tab)
* Note how the panel again changes to the "old" sync setup.
* Repeat ad-nauseum.

To repeat - very much a WIP and needs much cleanup and some clarity on a number of things, but it *looks* like progress ;)

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

This patch is getting closer but still needs more work.  But it's close enough to get feedback on the general approach.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

This patch should be almost ready to go, sans some styling tweaks and the ever-changing UX requirements.  It correctly handles the case when sync is already configured for the legacy provider, and also when configured for fxa.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

Updated to match the current UX flows, and also makes some attempt to guess what the UX flow should be if the server rejected the credentials.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8362791 [details] [diff] [review]
0003-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

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

This looks good, given constraints :D

::: browser/components/preferences/sync.js
@@ +8,5 @@
>  const PAGE_NO_ACCOUNT = 0;
>  const PAGE_HAS_ACCOUNT = 1;
>  const PAGE_NEEDS_UPDATE = 2;
>  
> +const FXA_PAGE_DETERMINING = 3;

Determining what? :) Perhaps PAGE_PLEASE_WAIT or PAGE_INTERSTITIAL?

@@ +118,5 @@
> +        if (data) {
> +          // We are logged in locally, but maybe we are in a state where the
> +          // server rejected our credentials (eg, password changed on the server)
> +          let fxaLoginStatus = document.getElementById("fxaLoginStatus");
> +          let enginesDisabled;

enginesListDisabled. Engines themselves can be disabled...

@@ +233,5 @@
>      }
>    },
>  
> +  manageFirefoxAccount: function() {
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");

The docs don't say if this will open a new window if one doesn't exist. Please check that behavior -- open prefs and then close all browser windows.

@@ +243,5 @@
> +  verifyFirefoxAccount: function() {
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");
> +    win.switchToTabHavingURI("about:accounts", true);
> +    // seeing as we are doing this in a tab we close the prefs dialog.
> +    window.close();

Maybe move these four lines into a "jumpToContent" or "switchToAboutAccounts" method? Or just call manageFirefoxAccount from this one...

::: browser/components/preferences/sync.xul
@@ +186,5 @@
> +        </vbox>
> +
> +        <vbox id="noFxaAccount">
> +          <!-- *sob* - markh can't work out how to make the iframe take up
> +               all the space!

File a bug?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0004-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

Thanks for the feedback.

(In reply to Richard Newman [:rnewman] from comment #5)
> Comment on attachment 8362791 [details] [diff] [review]
> Determining what? :) Perhaps PAGE_PLEASE_WAIT or PAGE_INTERSTITIAL?

PAGE_PLEASE_WAIT it is!

> enginesListDisabled. Engines themselves can be disabled...
Done

> The docs don't say if this will open a new window if one doesn't exist.
> Please check that behavior -- open prefs and then close all browser windows.

It doesn't open a new window, but on Windows at least, I can't close the browser window which opened prefs as prefs is modal.  But yeah, this will fail in that case.  Maybe a followup would be OK to fix this (or if OSX can hit this, I'll research a little more) - but as per below, there's now only 1 place which does this.

> Maybe move these four lines into a "jumpToContent" or
> "switchToAboutAccounts" method? Or just call manageFirefoxAccount from this
> one...

Done - openContentInBrowser() jumped out of my head, but I'm up for anything else.  Also note that "manage" now reads a pref to determine what to open, and there's a good chance "verify" might change later too, but that's ok for now.

> > +          <!-- *sob* - markh can't work out how to make the iframe take up
> > +               all the space!
> 
> File a bug?

Fixing my other flex related issues seems to have made this go away too, so I've nuked the comment.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Mark Hammond [:markh] from comment #6)

> > The docs don't say if this will open a new window if one doesn't exist.
> > Please check that behavior -- open prefs and then close all browser windows.
> 
> It doesn't open a new window, but on Windows at least, I can't close the
> browser window which opened prefs as prefs is modal.  But yeah, this will
> fail in that case.  Maybe a followup would be OK to fix this (or if OSX can
> hit this, I'll research a little more) - but as per below, there's now only
> 1 place which does this.

Yeah, OS X can hit this. Applications and windows are separate concepts: you can have Firefox open with no windows, and prefs is a separate window. Linux might be the same?

I would be surprised if this hasn't already been solved…

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8364309 [details] [diff] [review]
0004-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

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

r+ assuming UX is happy with the strings, and you tackle the OS X window issue. (If it's super hard, push it to a follow-up.)

::: browser/components/preferences/sync.js
@@ +113,5 @@
> +      this.page = PAGE_PLEASE_WAIT;
> +      Components.utils.import("resource://gre/modules/FxAccounts.jsm");
> +      fxAccounts.getSignedInUser().then(data => {
> +        this.page = data ? FXA_PAGE_LOGGED_IN : FXA_PAGE_LOGGED_OUT;
> +        if (data) {

if (!data) {
  this.page = FXA_PAGE_LOGGED_OUT;
  return;
}

// We are logged in locally...

@@ +118,5 @@
> +          // We are logged in locally, but maybe we are in a state where the
> +          // server rejected our credentials (eg, password changed on the server)
> +          let fxaLoginStatus = document.getElementById("fxaLoginStatus");
> +          let enginesListDisabled;
> +          // not verfied implies login error state, so check that first.

s/not verfied/Not verified

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
@@ +46,5 @@
>  <!ENTITY prefs.tosLink.label        "Terms of Service">
>  <!ENTITY prefs.ppLink.label         "Privacy Policy">
> +
> +<!-- Firefox Accounts stuff -->
> +<!ENTITY determiningStatus.label    "Determining your Firefox Account status...">

"Determining your &syncBrand.fxa.label; status…"

Note ellipsis.

@@ +48,5 @@
> +
> +<!-- Firefox Accounts stuff -->
> +<!ENTITY determiningStatus.label    "Determining your Firefox Account status...">
> +<!ENTITY signedInUnverified.beforename.label "">
> +<!ENTITY signedInUnverified.aftername.label "is not verified">

Is this concatenation the appropriate way to do this? I thought we typically use substitution.

Closing period?

@@ +51,5 @@
> +<!ENTITY signedInUnverified.beforename.label "">
> +<!ENTITY signedInUnverified.aftername.label "is not verified">
> +
> +<!ENTITY signedInLoginFailure.beforename.label "">
> +<!ENTITY signedInLoginFailure.aftername.label "failed to login to the server">

Closing period?

@@ +53,5 @@
> +
> +<!ENTITY signedInLoginFailure.beforename.label "">
> +<!ENTITY signedInLoginFailure.aftername.label "failed to login to the server">
> +
> +<!ENTITY notSignedIn.label           "You are not signed in">

Closing period.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0005-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

Thanks!

(In reply to Richard Newman [:rnewman] from comment #8)
> r+ assuming UX is happy with the strings, and you tackle the OS X window
> issue. (If it's super hard, push it to a follow-up.)

I handled this by using gSyncUtils._openLink() if getMostRecentWindow returns null.  The comment explains why I don't always use that.

> if (!data) {
>   this.page = FXA_PAGE_LOGGED_OUT;
>   return;
> }

done.

> s/not verfied/Not verified

Done.

> "Determining your &syncBrand.fxa.label; status…"
> 
> Note ellipsis.

Done.

> 
> Is this concatenation the appropriate way to do this? I thought we typically
> use substitution.

This was the advice given to me on #fx-team - I wasn't aware entities could do string substitutions, but only string bundles could (and the other alternative was markup directly in the .dtd, which actually works but is apparently frowned-upon :).  Either way, I've left that alone.

> Closing period?

I took the liberty of channeling UX here and added the periods - they weren't in the mockups I was running from, but it does look weird without them.

This patch also has a new implementation of the function verifyFirefoxAccount(), which uses string bundles.  Note also that some strings will appear to be missing from this patch - they are now in bug 957425 (which shares those strings).  For this reason, I'm asking for another quick review.

Thanks!

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

This patch adds a "Privacy Notice" string and uses it in the sync options pane.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8365439 [details] [diff] [review]
0002-Bug-957426-Have-sync-preferences-magically-do-the-ri.patch

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

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties
@@ +138,5 @@
> +
> +###Preferences::Sync::Firefox Accounts
> +firefoxAccountsVerificationSentTitle=Verification Email sent
> +# LOCALIZATION NOTE: %S = user's email address.
> +firefoxAccountsVerificationSentHeading=An email verification link waits at %S

s/waits/awaits

@@ +139,5 @@
> +###Preferences::Sync::Firefox Accounts
> +firefoxAccountsVerificationSentTitle=Verification Email sent
> +# LOCALIZATION NOTE: %S = user's email address.
> +firefoxAccountsVerificationSentHeading=An email verification link waits at %S
> +firefoxAccountVerificationSentDescription=Please check your email, and click the verification link to begin synching

s/synching/syncing. And I'd kill the comma -- you need to do both tasks to begin syncing.

How 'bout this:

   Click the verification link in your email to begin syncing.

::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
@@ +43,5 @@
>  <!ENTITY unlinkDevice.label           "Unlink This Device">
>  
>  <!-- Footer stuff -->
>  <!ENTITY prefs.tosLink.label        "Terms of Service">
>  <!ENTITY prefs.ppLink.label         "Privacy Policy">

Is this still used?

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Thanks!

(In reply to Richard Newman [:rnewman] from comment #11)
> > +###Preferences::Sync::Firefox Accounts
> > +firefoxAccountsVerificationSentTitle=Verification Email sent
> > +# LOCALIZATION NOTE: %S = user's email address.
> > +firefoxAccountsVerificationSentHeading=An email verification link waits at %S
> 
> s/waits/awaits
> 
> @@ +139,5 @@
> > +###Preferences::Sync::Firefox Accounts
> > +firefoxAccountsVerificationSentTitle=Verification Email sent
> > +# LOCALIZATION NOTE: %S = user's email address.
> > +firefoxAccountsVerificationSentHeading=An email verification link waits at %S
> > +firefoxAccountVerificationSentDescription=Please check your email, and click the verification link to begin synching
> 
> s/synching/syncing. And I'd kill the comma -- you need to do both tasks to
> begin syncing.
> 
> How 'bout this:
> 
>    Click the verification link in your email to begin syncing.

jgruen: I took this wording directly from the mockups - can you please decide if the suggested string changes above are OK?  (I'd probably have just done them if I was going to land this today - but I'm not, so there's time for you weigh in :)

> ::: browser/locales/en-US/chrome/browser/preferences/sync.dtd
> @@ +43,5 @@
> >  <!ENTITY unlinkDevice.label           "Unlink This Device">
> >  
> >  <!-- Footer stuff -->
> >  <!ENTITY prefs.tosLink.label        "Terms of Service">
> >  <!ENTITY prefs.ppLink.label         "Privacy Policy">
> 
> Is this still used?

It is still used by the "old" sync preferences (ie, for users who start with a configured legacy account.)  Only the Fxa-based panels say "Privacy Notice" which I assume was the intent - please correct me if I'm wrong.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/5c74030c7c0e

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c74030c7c0e

Comment 15

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 29.0b8
Build Id: 20140414143035
OS: Win 7 x64, Mac Os x 10.9.2, Ubuntu 13.04 x 64

The Sync Options page is updated to be in line with Firefox Accounts.

However I found an annoying behavior, clicking on the Manage link on the Sync Options(Preferences), on Mac Os X and Linux, will close the Options(Preferences) window. This makes sense on windows due to the fact that Options(Preferences) window is modal.
Options(Preferences) is not modal on Mac and Linux so there is no reason to close it.
Another argument to not close the Options(Preferences)window is that if you go to Options(Preferences), close the browser page then click on the Manage link a browser window is opened with https://accounts.firefox.com/settings but in this case the Options(Preferences) window is not closed.

Comment 16

[Mark Hammond [:markh] [:mhammond]]

(In reply to Catalin Varga [QA][:VarCat] from comment #15)
> However I found an annoying behavior, clicking on the Manage link on the
> Sync Options(Preferences), on Mac Os X and Linux, will close the
> Options(Preferences) window. This makes sense on windows due to the fact
> that Options(Preferences) window is modal.
> Options(Preferences) is not modal on Mac and Linux so there is no reason to
> close it.
> Another argument to not close the Options(Preferences)window is that if you
> go to Options(Preferences), close the browser page then click on the Manage
> link a browser window is opened with https://accounts.firefox.com/settings
> but in this case the Options(Preferences) window is not closed.

Feel free to open a bug on that!