Closed
Bug 675823
Opened 13 years ago
Closed 13 years ago
Rework Sync setup UI: "J-PAKE First"
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: rnewman, Assigned: philikon)
References
Details
(Whiteboard: [verified in services])
Attachments
(5 files, 4 obsolete files)
9.40 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
20.96 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
4.31 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
19.06 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
8.14 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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?
Updated•13 years ago
|
Assignee: nobody → ally
Assignee | ||
Updated•13 years ago
|
Assignee: ally → philipp
Assignee | ||
Comment 1•13 years ago
|
||
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)
Assignee | ||
Comment 2•13 years ago
|
||
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 | ||
Comment 3•13 years ago
|
||
Attachment #563623 -
Flags: feedback?(rnewman)
Reporter | ||
Comment 4•13 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•13 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•13 years ago
|
Attachment #563623 -
Flags: feedback?(rnewman) → feedback+
Assignee | ||
Comment 6•13 years ago
|
||
Now with tests and, thanks to that, a bunch of silly mistakes removed.
Attachment #563582 -
Attachment is obsolete: true
Attachment #564005 -
Flags: review?(rnewman)
Assignee | ||
Comment 7•13 years ago
|
||
Cleaned up and addressed nits.
Attachment #563590 -
Attachment is obsolete: true
Attachment #564012 -
Flags: review?(rnewman)
Reporter | ||
Comment 8•13 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•13 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•13 years ago
|
||
Screenshots plz. :D /me is apparently waaay too lazy.
Assignee | ||
Comment 11•13 years ago
|
||
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)
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #563623 -
Attachment is obsolete: true
Attachment #564024 -
Flags: review?(rnewman)
Assignee | ||
Comment 13•13 years ago
|
||
This removes the Recovery Key page and collapses the other "new account" pages into just one, as per mockups.
Attachment #564025 -
Flags: review?(rnewman)
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #564026 -
Flags: review?(rnewman)
Assignee | ||
Comment 15•13 years ago
|
||
(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.
Assignee | ||
Comment 16•13 years ago
|
||
(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•13 years ago
|
Attachment #564024 -
Flags: review?(rnewman) → review+
Reporter | ||
Comment 17•13 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•13 years ago
|
Attachment #564026 -
Flags: review?(rnewman) → review+
Reporter | ||
Comment 18•13 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+
Assignee | ||
Comment 19•13 years ago
|
||
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]
Assignee | ||
Comment 20•13 years ago
|
||
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
Comment 21•13 years ago
|
||
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]
Assignee | ||
Comment 22•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Updated•6 years ago
|
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.
Description
•