689311 - "Pair a Device" and "Set up Sync" links in the Sync prefpane

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Attachment: "mockup"

In bug 675821 we're adding "Pair a Device" and "Set up Sync" links to the home tab. This is blocked on the existence of a home tab in the first place, so if we get "J-PAKE First" (bug 675823) landed before that, we could easily expose it in the pref pane for now.

See attachment for "mockup".

Comment 1

[Philipp von Weitershausen [:philikon]]

Turns out it's useful to have this link to launch the wizard while I work on it, so I'm taking this.

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: v1

This anticipates the work in bug 675823 and makes the setup wizard aware of different wizard types (we already have standard and reset sync, so this essentially adds a third kind)

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: screenshot

Here's a screenshot of Windows. Ultimately we will add those links to the Home tab as well, but for now those provide a good entry point to the new "J-PAKE First" wizard.

Comment 4

[Philipp von Weitershausen [:philikon]]

Note that I think the wording needs to improve here still because it isn't super obvious what each of the two links do. We probably want to say something like

  Set up Sync on this computer

  Pair with another device

to differentiate the two flows better.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 562918 [details] [diff] [review]
v1

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

(Ugh, syncSetup.js cuddles half of its "else" clauses. I won't suggest cleanup for the lines that pop up in the context!)

I've got a few questions on parts of this, so just clearing the r? until you respond. Forgive me if my remarks seem a little cautious, but I'm erring on that side until I know this code a little better.

::: browser/components/preferences/sync.js
@@ +147,5 @@
> +   * Invoke the Sync setup wizard.
> +   * 
> +   * @param wizardType
> +   *        Indicates type of wizard to launch. One of these strings:
> +   *          ""      -- regular set up wizard

O-O

How about "setup", or "default"?

The implication is that you're relying on "" being falsy, but I actually don't see that elsewhere in the patch, so this smells of voodoo.

@@ +151,5 @@
> +   *          ""      -- regular set up wizard
> +   *          "pair"  -- pair a device first
> +   *          "reset" -- reset sync
> +   */
> +  openSetup: function (wizardType) {

As far as I can tell, this function was an exact clone of browser/base/content/browser-syncui.js:gSyncUI.openSetup.

I dislike the duplication, but I'm more worried by the new divergence.

Any thoughts on addressing that dupe?

::: browser/components/preferences/sync.xul
@@ +78,5 @@
>              &weaveDesc.label;
>            </description>
> +          <separator/>
> +          <label class="text-link"
> +                 onclick="event.stopPropagation(); gSyncPane.openSetup();"

Could you put a value in as the argument to openSetup, just to be explicit? I'd rather have it be obvious what each of these two branches do, not just the 'pair' link.

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #5)
> (Ugh, syncSetup.js cuddles half of its "else" clauses. I won't suggest
> cleanup for the lines that pop up in the context!)

Thank you. It would be a clusterfuck. I'm trying to ship something here, style is the least of my worries. :)

> > +   * @param wizardType
> > +   *        Indicates type of wizard to launch. One of these strings:
> > +   *          ""      -- regular set up wizard
> 
> O-O
> 
> How about "setup", or "default"?
> 
> The implication is that you're relying on "" being falsy, but I actually
> don't see that elsewhere in the patch, so this smells of voodoo.

I'm relying on that in syncSetup.js, in gSyncSetup.init():

    this.wizardType = "";
    if (window.arguments && window.arguments[0]) {
      this.wizardType = window.arguments[0];
    }

Can't we just say something not-truthy will lead to the regular wizard?

> @@ +151,5 @@
> > +   *          ""      -- regular set up wizard
> > +   *          "pair"  -- pair a device first
> > +   *          "reset" -- reset sync
> > +   */
> > +  openSetup: function (wizardType) {
> 
> As far as I can tell, this function was an exact clone of
> browser/base/content/browser-syncui.js:gSyncUI.openSetup.
> 
> I dislike the duplication, but I'm more worried by the new divergence.
> 
> Any thoughts on addressing that dupe?

I know it exists. And yes, it sucks. Bug 583366 exists for it (mentioned in the XXXzpao comment!). If we find it really, really bothers us, then we can fix it. But until then, I'm meh. Also, it's actually a [good-first-bug], so it's something that somebody else can always do at a later time, I think.

> ::: browser/components/preferences/sync.xul
> @@ +78,5 @@
> >              &weaveDesc.label;
> >            </description>
> > +          <separator/>
> > +          <label class="text-link"
> > +                 onclick="event.stopPropagation(); gSyncPane.openSetup();"
> 
> Could you put a value in as the argument to openSetup, just to be explicit?
> I'd rather have it be obvious what each of these two branches do, not just
> the 'pair' link.

Meh. Fine.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Philipp von Weitershausen [:philikon] from comment #6)

> Thank you. It would be a clusterfuck. I'm trying to ship something here,
> style is the least of my worries. :)

:D

Filed Bug 689821.

> Can't we just say something not-truthy will lead to the regular wizard?

wfm. I'd probably rather use null than ""!

> I know it exists. And yes, it sucks.

No need to fix now, but put a small warning comment (with a link to the bug) in the sync.js version?

> Meh. Fine.

Thank you!

I will be around tonight for a rubberstamp.

Comment 8

[Philipp von Weitershausen [:philikon]]

Note to self: we want to rename "Add a Device" to "Pair a Device with an Activation Code" to make the difference between "Set up Sync" and "Pair a Device" clearer.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> Note to self: we want to rename "Add a Device" to "Pair a Device with an
> Activation Code" to make the difference between "Set up Sync" and "Pair a
> Device" clearer.

I'm not sure that's the perfect phrasing; it can be read as 

  (Pair a device) (with an Activation Code) -- "pair another device with an activation code (which I will presumably be given after pressing this thing)

or

  (Pair a) (device with an Activation Code) -- "pair a device, given that I have an activation code already".

In fact, I'm not even sure which direction you intend this to mean!

Perhaps "Use an activation code to pair a device", which implies that you already have one, or some other more active language?

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Richard Newman [:rnewman] from comment #9)
> (In reply to Philipp von Weitershausen [:philikon] from comment #8)
> > Note to self: we want to rename "Add a Device" to "Pair a Device with an
> > Activation Code" to make the difference between "Set up Sync" and "Pair a
> > Device" clearer.
> 
> I'm not sure that's the perfect phrasing; it can be read as 
> 
>   (Pair a device) (with an Activation Code) -- "pair another device with an
> activation code (which I will presumably be given after pressing this thing)

No.

>   (Pair a) (device with an Activation Code) -- "pair a device, given that I
> have an activation code already".

Yes.

> Perhaps "Use an activation code to pair a device", which implies that you
> already have one, or some other more active language?

Too long. "Pair a Device" needs to stand out. Also: not my wording, Faaborg's.

English FTL. "It's your language, I'm just trying to use it."

Comment 11

[Richard Newman [:rnewman]]

(In reply to Philipp von Weitershausen [:philikon] from comment #10)

> Too long. "Pair a Device" needs to stand out. Also: not my wording,
> Faaborg's.

Doesn't matter whose wording, but CCing faaborg in case he has an opinion here!

If the goal is to avoid users hitting the wrong one and being confused, the wording needs to clearly communicate why you should click one and not the other. I don't think that "... Activation Code" does that on its own, and saying "No, you misunderstood" isn't any help to the user.

If it's OK for them to click the wrong one, then the next screen should offer a little help for users who do.

"Pair a device with an Activation Code" would make me think it's just badly worded geek-speak for "OK, here's where I start when I have another device I want to pair", and I'd be expecting to get an Activation Code when I click. I don't know what the average user would think, of course.

> English FTL. "It's your language, I'm just trying to use it."

For realz.

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Addressed review comments and improved styling a little bit. Didn't change the "Pair a Device" string at this point. We can always do that later.

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: screenshot v2

Comment 14

[Philipp von Weitershausen [:philikon]]

I've gone ahead and landed this on s-c:

https://hg.mozilla.org/services/services-central/rev/6d2a4652e87e

If there are any ui-review comments, we can address them before the hand-off or in a follow-up on m-c.

Comment 15

[Tracy Walker [:tracy]]

verified on s-c

Comment 16

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/6d2a4652e87e

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 564009 [details]
screenshot v2

not really sure where the ui-review field went, anyway ui-review+

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

>  (Pair a device) (with an Activation Code) -- "pair another device with an activation code >(which I will presumably be given after pressing this thing)

yeah, I came to the same conclusion, mentioning the activation code doesn't really help at all.  Kind of hoping that "pair" is enough to imply 2 things, and "Setup" is enough to imply where you are supposed to start if you've so far done nothing, but overall this doesn't match any other system so there isn't any consistency for users to rely on, and they very well may be confused.

It's possible that most first time sync users will come in to the UI through snippets and contextual messages (password manager, bookmarks), so under that approach there won't be as much confusion around having both links present (only setup is shown in those messages).