Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Rework Sync setup UI: "J-PAKE First"

RESOLVED FIXED in mozilla10

Status

Cloud Services
Firefox Sync: UI
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rnewman, Assigned: philikon)

Tracking

(Blocks: 1 bug)

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [verified in services])

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Blocks: 675826
Assignee: nobody → ally
(Assignee)

Updated

6 years ago
Depends on: 684074
(Assignee)

Updated

6 years ago
Depends on: 677863
(Assignee)

Updated

6 years ago
Assignee: ally → philipp
(Assignee)

Updated

6 years ago
Blocks: 689311
(Assignee)

Updated

6 years ago
Depends on: 689428
Created attachment 563582 [details] [diff] [review]
Part 1 (WIP 1): Implement SendCredentialsController

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)
Created attachment 563590 [details] [diff] [review]
Part 2 (WIP 1): Implement "J-PAKE first" on desktop

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)
(Assignee)

Updated

6 years ago
Depends on: 690616
Created attachment 563623 [details] [diff] [review]
Part 3 (WIP 1): Show message while pairing on mobile
Attachment #563623 - Flags: feedback?(rnewman)
(Reporter)

Comment 4

6 years ago
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+
(Reporter)

Comment 5

6 years ago
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+
(Reporter)

Updated

6 years ago
Attachment #563623 - Flags: feedback?(rnewman) → feedback+
Created attachment 564005 [details] [diff] [review]
Part 1 (v1): Implement SendCredentialsController

Now with tests and, thanks to that, a bunch of silly mistakes removed.
Attachment #563582 - Attachment is obsolete: true
Attachment #564005 - Flags: review?(rnewman)
Created attachment 564012 [details] [diff] [review]
Part 2 (v1): Implement "J-PAKE first" on desktop

Cleaned up and addressed nits.
Attachment #563590 - Attachment is obsolete: true
Attachment #564012 - Flags: review?(rnewman)
(Reporter)

Comment 8

6 years ago
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+
(Reporter)

Comment 9

6 years ago
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.
(Reporter)

Comment 10

6 years ago
Screenshots plz. :D

/me is apparently waaay too lazy.
Created attachment 564023 [details] [diff] [review]
Part 2 (v2): Implement "J-PAKE first" on desktop

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)
Created attachment 564024 [details] [diff] [review]
Part 3 (v1): Show message while pairing on mobile
Attachment #563623 - Attachment is obsolete: true
Attachment #564024 - Flags: review?(rnewman)
Created attachment 564025 [details] [diff] [review]
Part 4 (v1): More concise setup flow

This removes the Recovery Key page and collapses the other "new account" pages into just one, as per mockups.
Attachment #564025 - Flags: review?(rnewman)
Created attachment 564026 [details] [diff] [review]
Part 5 (v1): Update strings
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.
(Reporter)

Updated

6 years ago
Attachment #564024 - Flags: review?(rnewman) → review+
(Reporter)

Updated

6 years ago
Blocks: 653547
(Reporter)

Comment 17

6 years ago
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+
(Reporter)

Updated

6 years ago
Attachment #564026 - Flags: review?(rnewman) → review+
(Reporter)

Comment 18

6 years ago
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+
Pushed with nits addressed:

https://hg.mozilla.org/services/services-central/rev/8689feaa86f4
https://hg.mozilla.org/services/services-central/rev/e9d67d9dc0fd
https://hg.mozilla.org/services/services-central/rev/927aa154f29a
https://hg.mozilla.org/services/services-central/rev/060619fad02c
https://hg.mozilla.org/services/services-central/rev/974498957b83
Status: NEW → ASSIGNED
Whiteboard: [fixed in services]
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]
https://hg.mozilla.org/mozilla-central/rev/8689feaa86f4
https://hg.mozilla.org/mozilla-central/rev/e9d67d9dc0fd
https://hg.mozilla.org/mozilla-central/rev/927aa154f29a
https://hg.mozilla.org/mozilla-central/rev/060619fad02c
https://hg.mozilla.org/mozilla-central/rev/974498957b83
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Blocks: 687316
(Assignee)

Updated

6 years ago
Depends on: 710543
(Reporter)

Updated

5 years ago
Duplicate of this bug: 677863
You need to log in before you can comment on or make changes to this bug.