Closed Bug 675823 Opened 13 years ago Closed 13 years ago

Rework Sync setup UI: "J-PAKE First"

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: rnewman, Assigned: philikon)

References

Details

(Whiteboard: [verified in services])

Attachments

(5 files, 4 obsolete files)

Implement reworked UI flow according to updated mockups.

    Desirable: land with "Pair device" on Home Tab

    Maintain some method to asynchronously set up two devices. Single point of entry; get-out clause: "I'm not near my computer".

    Attempting to pair/join account via create account flow. Help users who get lost.

    prompt for email before we ask them if they have an account or not (need to update mockups)

    Delay mobile device sync until desktop has synced. J-PAKE communication channel?
Blocks: 675826
Assignee: nobody → ally
Depends on: 684074
Depends on: 677863
Assignee: ally → philipp
Blocks: 689311
Depends on: 689428
When we pair with a device while creating a new account, we want to complete the pairing after the first successful sync. By that time, the wizard will have long gone. Because we shouldn't leak DOM windows, but also need the JPAKEClient object and a controller for it to survive, the SendCredentialsController exists. It ensures the JPAKEClient stays alive and Sync credentials are sent after the first sync completes successfully.

Missing tests (bad philikon!), everything else should be there.
Attachment #563582 - Flags: feedback?(rnewman)
This implements "J-PAKE first" on desktop. It doesn't change the rest of the wizard to go with the new flow, and it's missing appropriate UX for mobile. BUT: it works! More parts to follow.
Attachment #563590 - Flags: feedback?(rnewman)
Depends on: 690616
Comment on attachment 563582 [details] [diff] [review]
Part 1 (WIP 1): Implement SendCredentialsController

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

Really nice.

::: services/sync/modules/policies.js
@@ +745,5 @@
> + * This object is designed to take over as the JPAKEClient controller,
> + * presumably replacing one that is UI-based which would either cause
> + * DOM objects to leak or the JPAKEClient to be GC'ed when the DOM
> + * context disappears. This object stays alive for the duration of the
> + * transfer by being strong-ref'ed as an nsIObserver.

<3 <3
Attachment #563582 - Flags: feedback?(rnewman) → feedback+
Comment on attachment 563590 [details] [diff] [review]
Part 2 (WIP 1): Implement "J-PAKE first" on desktop

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

Still a bunch of TODOs and such, but certainly seems like the right track, and no obvious holes.

::: browser/base/content/syncSetup.js
@@ +103,5 @@
>      let obs = [
>        ["weave:service:changepph:finish", "onResetPassphrase"],
>        ["weave:service:login:start",  "onLoginStart"],
>        ["weave:service:login:error",  "onLoginEnd"],
> +      ["weave:service:login:finish", "onLoginEnd"],

Nit: trailing comma. Yeah, I know it doesn't cause a problem in practice...

@@ +132,2 @@
>  
> +    this.wizardType = "";

Weren't we changing the default to null or a non-empty string in the last bug I gave feedback on?

@@ +586,5 @@
>          return false;
> +      case EXISTING_ACCOUNT_LOGIN_PAGE:
> +        // If we've paired at the beginning, we went straight to the manual
> +        // login page, so we if we go back, go back to the page that let's
> +        // us choose whether we already have an account or not.

// If we were already pairing on entry, we went straight to the manual
// login page. If subsequently we go back, return to the page that lets
// us choose whether we already have an account.

@@ +1119,1 @@
>  // onWizardAdvance() and onPageShow() are run before init(), so we'll set

Delete 'set'.
Attachment #563590 - Flags: feedback?(rnewman) → feedback+
Attachment #563623 - Flags: feedback?(rnewman) → feedback+
Now with tests and, thanks to that, a bunch of silly mistakes removed.
Attachment #563582 - Attachment is obsolete: true
Attachment #564005 - Flags: review?(rnewman)
Cleaned up and addressed nits.
Attachment #563590 - Attachment is obsolete: true
Attachment #564012 - Flags: review?(rnewman)
Comment on attachment 564005 [details] [diff] [review]
Part 1 (v1): Implement SendCredentialsController

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

::: services/sync/modules/policies.js
@@ +768,5 @@
> +  // this device's sync configuration, in case that happens while we
> +  // haven't finished the first sync yet.
> +  Services.obs.addObserver(this, "weave:service:sync:finish",     false);
> +  Services.obs.addObserver(this, "weave:service:sync:error",      false);
> +  Services.obs.addObserver(this, "weave:service:start-over",      false);

Nit: needless additional space.

@@ +776,5 @@
> +  unload: function unload() {
> +    this._log.trace("Unloading.");
> +    Services.obs.removeObserver(this, "weave:service:sync:finish");
> +    Services.obs.removeObserver(this, "weave:service:sync:error");
> +    Services.obs.removeObserver(this, "weave:service:start-over");

Let's make this safe with a try..catch. unload() could potentially be called multiple times.
Attachment #564005 - Flags: review?(rnewman) → review+
Comment on attachment 564012 [details] [diff] [review]
Part 2 (v1): Implement "J-PAKE first" on desktop

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

Looks good. I haven't reviewed visually yet; will do that now then set r+.

::: browser/base/content/syncSetup.js
@@ +228,5 @@
> +      Services.obs.removeObserver("weave:service:sync:error", send);
> +      let credentials = {account:   Weave.Service.account,
> +                         password:  Weave.Service.password,
> +                         synckey:   Weave.Service.passphrase,
> +                         serverURL: Weave.Service.serverURL};

If you're feeling really, really motivated, it might be nice to have this "credential bundle" be spat out by Weave.Service (and, indeed, consumed by it):

  let credentials = Weave.Service.credentials;

We do this in quite a few places...

@@ +400,5 @@
>  
>    onPageShow: function() {
>      switch (this.wizard.pageIndex) {
> +      case PAIR_PAGE:
> +        /////this.wizard.getButton("next").hidden = false;

Ahem.

@@ +653,5 @@
>    },
>  
> +  startPairing: function startPairing() {
> +    this.pairDeviceErrorRow.hidden = true;
> +    const JPAKE_ERROR_USERABORT = Weave.JPAKE_ERROR_USERABORT;

Little comment explaining why, plz.

@@ +696,5 @@
> +    if (!this._jpakeclient) {
> +      // The channel was aborted while we were setting up the account
> +      // locally. XXX TODO should we do anything here, e.g. tell
> +      // the user on the last wizard page that it's ok, they just
> +      // have to pair again?

File a bug to follow up?

::: browser/base/content/syncSetup.xul
@@ +79,5 @@
> +    <description>
> +      &addDevice.dialog.description.label;
> +      <label class="text-link"
> +             value="&addDevice.showMeHow.label;"
> +             href="https://services.mozilla.com/sync/help/add-device"/>

Not a big deal, but this URL is shared with syncAddDevice.xul. It'd be nice if it were an entity in a shared DTD.
Screenshots plz. :D

/me is apparently waaay too lazy.
Avoid captcha flickering, adjust some strings, and apply CSS changes to all three platforms.
Attachment #564012 - Attachment is obsolete: true
Attachment #564012 - Flags: review?(rnewman)
Attachment #564023 - Flags: review?(rnewman)
Attachment #563623 - Attachment is obsolete: true
Attachment #564024 - Flags: review?(rnewman)
This removes the Recovery Key page and collapses the other "new account" pages into just one, as per mockups.
Attachment #564025 - Flags: review?(rnewman)
Attachment #564026 - Flags: review?(rnewman)
(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> Created attachment 564023 [details] [diff] [review] [diff] [details] [review]
> Part 2 (v2): Implement "J-PAKE first" on desktop
> 
> Avoid captcha flickering, adjust some strings, and apply CSS changes to all
> three platforms.

This comment was meant to go with Part 4. Part 2 v2 is actually the same as v1. But it wasn't reviewed yet, so I just left it like this.
(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> (In reply to Philipp von Weitershausen [:philikon] from comment #11)
> > Created attachment 564023 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Part 2 (v2): Implement "J-PAKE first" on desktop
> > 
> > Avoid captcha flickering, adjust some strings, and apply CSS changes to all
> > three platforms.
> 
> This comment was meant to go with Part 4. Part 2 v2 is actually the same as
> v1. But it wasn't reviewed yet, so I just left it like this.

Uh, ignore me. I see comment 9 now. E_TOO_MANY_BOOGS_AND_PATCHES.
Attachment #564024 - Flags: review?(rnewman) → review+
Blocks: 653547
Comment on attachment 564025 [details] [diff] [review]
Part 4 (v1): More concise setup flow

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

This looks pretty good to me.

::: browser/base/content/syncSetup.js
@@ -376,5 @@
> -    let valid, str;
> -    if (password.value == document.getElementById("weavePassphrase").value) {
> -      // xxxmpc - hack, sigh
> -      valid = false;
> -      errorString = Weave.Utils.getErrorString("change.password.pwSameAsSyncKey");

You can kill the string, too.

@@ +1074,5 @@
>      if ((stateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW) == 0)
>        return;
>  
> +    // If we didn't find a captcha, assume it's not needed and don't show it.
> +    let responseStatus = request.QueryInterface(Ci.nsIHttpChannel).responseStatus;

Might be a redundant QI, but I don't immediately see where request comes from, so rock on.

::: browser/locales/en-US/chrome/browser/syncSetup.dtd
@@ +38,5 @@
>  <!ENTITY setup.ppLink.label         "Privacy Policy">
>  <!ENTITY setup.tosAgree3.label      "">
>  <!ENTITY setup.tosAgree2.accesskey  "">
>  
> +<!-- My Recover Key dialog -->

Recovery.
Attachment #564025 - Flags: review?(rnewman) → review+
Attachment #564026 - Flags: review?(rnewman) → review+
Comment on attachment 564023 [details] [diff] [review]
Part 2 (v2): Implement "J-PAKE first" on desktop

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

r+ given nits, and we'll take a look at the prettiness in one go.
Attachment #564023 - Flags: review?(rnewman) → review+
STRs for QA:

There are two large UX portions to this bug:

(1) "J-PAKE First" for users who start out on mobile and don't have a Firefox Sync connected desktop yet.

* Start out on a mobile device
* Click Set up Sync, read the instructions about going to "Pair a Device" on desktop
* Find "Pair a Device" on the Sync prefpane
* Pair the two devices. The mobile device will display a waiting message once the pairing is in progress.
* Once the pairing is established, the desktop allows the user to create a new account or sign into an existing one.
* Once the desktop has completed its first sync, the mobile device receives the credentials and does its first sync.

(Note: "Add a Device" was renamed to "Pair a Device" in an attempt to make it clearer that you need two devices for the process.)

(2) Simplified account creation flow as per faaborg's mockups in bug 675813:

* There is only one wizard page in the account creation flow now
* The Recovery Key page has been removed
* The captcha has been moved to the first page
verified in pairing galaxy tab to desktop with s-c builds with both a new and an existing account.
Whiteboard: [fixed in services] → [verified in services]
Blocks: sync2sm
Depends on: 710543
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.

Attachment

General

Created:
Updated:
Size: