634419 - Port |Bug 602682 - Sync UI: Implement easy setup|

Status: RESOLVED FIXED

(SeaMonkey :: Sync UI, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

This will contain string changes, thus should block b3.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 1: Add a Device Wizard

632952

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 512603 [details] [diff] [review]
Part 1: Add a Device Wizard

Second try...

Easy Setup - Add a Device Wizard
This is on top of bug 632952 and ports:
  - Bug 602682 - Sync UI: Implement easy setup (Add a Device Wizard part)
  - Bug 526445 - Sync pref pane: Change "Stop Using This Account" to "Deactivate This Device" label

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 2: Existing Account Wizard

This is also on top of bug 632952 and ports:
  - Bug 602682 - Sync UI: Implement easy setup (Existing Account Wizard part)
  - Bug 618229 - don't enforce a minimum passphrase length for entering existing data
  - Bug 618435 - Missing word in device connected screen
  - Bug 618704 - "Add a device" string is hard-coded in Firefox preferences, Sync panel
  - Bug 621466 - Sync UI: "Add a Device" dialog enables "Next" button after going to Sync Option

Note regarding "to &syncBrand.shortName.label; Options": cf. bug 602682 comment 60: 'we assume windows and say "options"'. SM has only Preferences, like Linux Minefield. Should we keep Options for compatibility (think Firefox Mobile devices) or use something like Preferences/Options?

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 512603 [details] [diff] [review]
Part 1: Add a Device Wizard

Actually, since I started by copying syncAddDevice.* from browser/, this patch already contains the fixes for the following two bugs, too: 

Bug 618300 - Easy Setup UI: "Device Connected" modal dialog has no OK/Close button
Bug 620547 - Typing in the entire PAKE code, then clicking "I don't have this device with me", then coming back leaves the next button disabled

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 1: Add a Device Wizard v1a

Updated for changes made in bug 636839.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 1: Add a Device Wizard v1b

Pre-review cleanup (removed empty space/line at the end of textbox tags)

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 2: Existing Account Wizard v1a

Incorporated changes from Bug 601019 - Sync UI: Always show "I have lost my Sync Key" and "Reset Password"

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 2: Existing Account Wizard v1b

Incorporated changes from bug 621757.

Comment 9

[[e-mail]]

Comment on attachment 517277 [details] [diff] [review]
Part 1: Add a Device Wizard v1b

>+              <button label="&addDevice.label;"
This button looks lonely. Doesn't this belong on the menulist?

>+                      oncommand="gSyncPane.openAddDevice(); return false;"
[I see it now in other places but I wonder why they bother to return false]

>+    this.pin1.setAttribute("maxlength", PIN_PART_LENGTH);
>+    this.pin2.setAttribute("maxlength", PIN_PART_LENGTH);
>+    this.pin3.setAttribute("maxlength", PIN_PART_LENGTH);
Why isn't this done in the XUL?

>+    this.nextFocusEl = {pin1: this.pin2,
>+                        pin2: this.pin3,
>+                        pin3: this.wizard.getButton("next")};
Nit: could do with spaces around {}s.

>+    this.wizard.canAdvance = (this.pin1.value.length == PIN_PART_LENGTH
>+                              && this.pin2.value.length == PIN_PART_LENGTH
>+                              && this.pin3.value.length == PIN_PART_LENGTH);
Nit: &&s belong at the end of the (previous) line.

>+// onWizardAdvance() and onPageShow() are run before init() so we'll set
>+// these up as lazy getters.
>+["wizard", "pin1", "pin2", "pin3"].forEach(function (id) {
>+  XPCOMUtils.defineLazyGetter(gSyncAddDevice, id, function() {
>+    return document.getElementById(id);
[Could have used document.documentElement instead of this.wizard]

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 1: Add a Device Wizard v1c

(In reply to comment #9)
> >+              <button label="&addDevice.label;"
> This button looks lonely. Doesn't this belong on the menulist?

I first thought this should be kept outside the menulist in accordance with Firefox, but on second thought it really is as much connected to the account as all the other menuitems, and IMO it also looks better, so I moved it there now.

> >+    this.pin1.setAttribute("maxlength", PIN_PART_LENGTH);
> >+    this.pin2.setAttribute("maxlength", PIN_PART_LENGTH);
> >+    this.pin3.setAttribute("maxlength", PIN_PART_LENGTH);
> Why isn't this done in the XUL?

Probably so that the constant is used in all places, but true, not really an argument. Changed.

> >+    this.wizard.canAdvance = (this.pin1.value.length == PIN_PART_LENGTH
> >+                              && this.pin2.value.length == PIN_PART_LENGTH
> >+                              && this.pin3.value.length == PIN_PART_LENGTH);
> Nit: &&s belong at the end of the (previous) line.

[Technically doesn't even need the parentheses, but probably better to keep it in this multi-line case.]

> >+// onWizardAdvance() and onPageShow() are run before init() so we'll set
> >+// these up as lazy getters.
> >+["wizard", "pin1", "pin2", "pin3"].forEach(function (id) {
> >+  XPCOMUtils.defineLazyGetter(gSyncAddDevice, id, function() {
> >+    return document.getElementById(id);
> [Could have used document.documentElement instead of this.wizard]

I'd like to keep such things in sync with FF so that I don't have to keep it in mind when porting future changes.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 2: Existing Account Wizard v1c

Comment 12

[[e-mail]]

Comment on attachment 520809 [details] [diff] [review]
Part 1: Add a Device Wizard v1c

>+    if (textbox && textbox.value.length == PIN_PART_LENGTH)
>+      this.nextFocusEl[textbox.id].focus();
>+
>+    this.wizard.canAdvance = (this.pin1.value.length == PIN_PART_LENGTH &&
>+                              this.pin2.value.length == PIN_PART_LENGTH &&
>+                              this.pin3.value.length == PIN_PART_LENGTH);
This needs to be the other way around. Otherwise the next button won't be enabled in time for you to focus it! r=me with that fixed.

[I don't know what the ideal thing to do is if the third textbox is completed before the first two.]

Is there some CSS missing? Those text boxes look really wide for 4 letters ;-)

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #12)
> This needs to be the other way around. Otherwise the next button won't be
> enabled in time for you to focus it! r=me with that fixed.

Good catch! Change applied locally, and thanks.

> [I don't know what the ideal thing to do is if the third textbox is completed
> before the first two.]

No need to make things complicated...

> Is there some CSS missing? Those text boxes look really wide for 4 letters ;-)

I was about to say "try WWWW" but then realized that I had always applied part 2 when testing, which contains the CSS you're missing (.pin). But you'll see that now, won't you? :-)

Comment 14

[[e-mail]]

Comment on attachment 520810 [details] [diff] [review]
Part 2: Existing Account Wizard v1c

>+      <textbox id="easySetupPIN1"
>+               class="pin"
>+               value=""
>+               disabled="true"/>
>+      <textbox id="easySetupPIN2"
>+               class="pin"
>+               value=""
>+               disabled="true"/>
>+      <textbox id="easySetupPIN3"
>+               class="pin"
>+               value=""
>+               disabled="true"/>
[Should these be readonly rather than disabled?]

>+<!ENTITY existingSyncKey.description  "You can get a copy of your Sync Key by going to &syncBrand.shortName.label; Options on your other device, and selecting &#x0022;My Sync Key&#x0022; under &#x0022;Manage Account&#x0022;.">
Shouldn't this be Preferences throughout?

>+.pin {
>+  font-size: 18pt;
>+  width: 4em;
[Note to self: compare with size="4"]

>+#add-device-throbber > image,
>+#login-throbber > image {
>   list-style-image: url("chrome://global/skin/icons/loading_16.png");
Don't need the > image, the style inherits.

Comment 15

[[e-mail]]

Comment on attachment 520810 [details] [diff] [review]
Part 2: Existing Account Wizard v1c

>   get _usingMainServers() {

>+        this.wizard.canRewind = true;
Weird. When I first tried this, the "back" button wasn't available on one of the screens. But I forgot which, and when I went to check back, it worked!

>+  onExistingServerCommand: function () {
>+    let control = document.getElementById("existingServer");
>+    if (control.selectedIndex == 0) {
>+      control.removeAttribute("editable");
>+      Weave.Svc.Prefs.reset("serverURL");
>+    } else {
>+      control.setAttribute("editable", "true");
>+      // Force a style flush to ensure that the binding is attached.
>+      control.clientTop;
>+      control.value = "";
>+      control.inputField.focus();
OK, so some issues:
1. Need to reset the focus when switching back to the default server as well.
2. Should avoid explicitly focusing the input field. Just focus the control.
3. If you press down arrow, this switches the menulist to editable, but the custom menuitem still thinks it's selected. Can't work out what causes this.

>+    let control = document.getElementById("server");
>     if (!this._usingMainServers) {
Might as well check the selectedIndex here too. Above nits also apply here.

>-          <label control="serverType"
>+          <label control="server"
>                  value="&server.label;"/>
Why no access key :-( (Same goes for other version)

>               <menuitem label="&serverType.main.label;"
>                         value="main"/>
>+              <menuitem label="&serverType.custom2.label;"
>                         value="custom"/>
These value attributes are unused. (Same goes for other version)

>+<!ENTITY setup.haveAccount.label "I already have a &syncBrand.fullName.label; account.">
>+<!ENTITY button.connect.label "Connect">
Bah, these are inconsistent :s

>+.pin {
>+  font-size: 18pt;
>+  width: 4em;
I'd prefer size="4" on the fields, given that we already set maxwidth.

Comment 16

[[e-mail]]

Comment on attachment 520810 [details] [diff] [review]
Part 2: Existing Account Wizard v1c

>+      control.setAttribute("editable", "true");
>+      // Force a style flush to ensure that the binding is attached.
>+      control.clientTop;
>+      control.value = "";
It seems somehow to be related to setting the value and changing the editability. (Speaking of which, we have a property for that.) But the only way I could work around it was to say control.menuBoxObject.activeChild = null;

Comment 17

[[e-mail]]

Comment on attachment 520810 [details] [diff] [review]
Part 2: Existing Account Wizard v1c

>+                    oncommand="gSyncSetup.onExistingServerCommand();"
Assuming it works, I'd prefer onselect=

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 2: Existing Account Wizard v1d

(In reply to comment #14)
> >+      <textbox id="easySetupPIN3"
> >+               class="pin"
> >+               value=""
> >+               disabled="true"/>
> [Should these be readonly rather than disabled?]

Hmm, readonly has the advantage of making them selectable, which helps with setting up other browsers on the same machine (think Firefox or something running in a VM). Now we need some CSS to make it look disabled (color; the border looks different, too, but I don't care too much about that). A possible downside would be that it may be confusing users because then you can click inside the textboxes and get a cursor which suggests you can type into it. It's not that bad with the changed text color, though, so it's acceptable to me.

> >+<!ENTITY existingSyncKey.description  "You can get a copy of your Sync Key by going to &syncBrand.shortName.label; Options on your other device, and selecting &#x0022;My Sync Key&#x0022; under &#x0022;Manage Account&#x0022;.">
> Shouldn't this be Preferences throughout?

See comment 3. Basically there are currently four Sync users: Firefox, Firefox Mobile, Firefox Home (iPhone) and SeaMonkey. All but SeaMonkey and non-Windows Firefox use Options. The strings in question refer to other devices so I thought we'd be consistent with Firefox here, which would also better match existing documentation (the guiding links go to Firefox's SUMO).

Anyway, I'm open for suggestions. As I said in comment 3, we might use something like "Preferences/Options".

> +.pin {
> >+  font-size: 18pt;
> >+  width: 4em;
[Note to self: compare with size="4"]

FTR: size="4" is not wide enough for WWWW (whereas width:4em is), but I think that's a very unlikely edge case.

(In reply to comment #15)

> > +<!ENTITY setup.haveAccount.label "I already have a &syncBrand.fullName.label; account.">
> > +<!ENTITY button.connect.label "Connect">
> Bah, these are inconsistent :s

In which way? Locally? Looking at the whole file you'll see that the labels of all syncSetup buttons have a "button." prefix.


I chose to create a new internal method for the whole control updating stuff (after some reordering I realized almost everything was the same anyway), but had to introduce an extra param for focussing since onServerCommand (now onServerSelect) was also called from onPageShow and we don't want to move the focus in that case. Also the style flush turned out to be necessary in the case of switching back to the default server, too.

I could also reproduce the "down arrow" selection issue but found no way to work around it (not sure where your suggestion would need to be placed). I have to admit that I didn't really search long here, though, since it really seems like a very minor issue to me.

Comment 19

[[e-mail]]

(In reply to comment #18)
> (In reply to comment #14)
> > >+      <textbox id="easySetupPIN3"
> > >+               class="pin"
> > >+               value=""
> > >+               disabled="true"/>
> > [Should these be readonly rather than disabled?]
> Hmm, readonly has the advantage of making them selectable, which helps with
> setting up other browsers on the same machine (think Firefox or something
> running in a VM). Now we need some CSS to make it look disabled
readonly fields should already have a different appearance; at least, the background colour changes for me. If it doesn't, then go back to disabled.

> See comment 3. Basically there are currently four Sync users: Firefox, Firefox
> Mobile, Firefox Home (iPhone) and SeaMonkey. All but SeaMonkey and non-Windows
> Firefox use Options. The strings in question refer to other devices so I
> thought we'd be consistent with Firefox here, which would also better match
> existing documentation (the guiding links go to Firefox's SUMO).
Well, seeing as some of the strings you've previously provided use both...

> FTR: size="4" is not wide enough for WWWW (whereas width:4em is), but I think
> that's a very unlikely edge case.
Again, size="4" WWWWFM ;-)

> > > +<!ENTITY setup.haveAccount.label "I already have a &syncBrand.fullName.label; account.">
> > > +<!ENTITY button.connect.label "Connect">
> > Bah, these are inconsistent :s
> In which way? Locally? Looking at the whole file you'll see that the labels of
> all syncSetup buttons have a "button." prefix.
Right, but "I already have an account" is a button...

> had to introduce an extra param for focussing since onServerCommand (now
> onServerSelect) was also called from onPageShow and we don't want to move the
> focus in that case.
I'd say that's a good reason not to use onselect. Sorry to trouble you.

> I could also reproduce the "down arrow" selection issue but found no way to
> work around it (not sure where your suggestion would need to be placed).
I think it was before the setAttribute("editable") call.

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #19)
> > Hmm, readonly has the advantage of making them selectable, which helps with
> > setting up other browsers on the same machine (think Firefox or something
> > running in a VM). Now we need some CSS to make it look disabled
> readonly fields should already have a different appearance; at least, the
> background colour changes for me. If it doesn't, then go back to disabled.

global/skin/textbox.css sets:
textbox[readonly="true"] -> background-color:-moz-Dialog; color:-moz-DialogText;
textbox[disabled="true"] -> background-color:-moz-Dialog; color:Graytext;
  ^ and for Mac: borders -> transparent ThreeDShadow -moz-Dialog;

So the background color depends on the OS/settings but is the same for readonly and disabled. But I wasn't talking about the bg color but rather the text color. Leaving it to the default in the readonly case looks just wrong, especially with an active caret, so I added the value from the disabled case to the .pin class.

BTW: The text-align:center set on the .pin class propagates to the context menu (which of course you only get in the readonly case).

So if we stick to disabled we lose the ability to copy and if we switch to readonly we need to add CSS for the text color and probably solve the context menu issue. Your call.

BTW2: Now that I tested on Linux I saw that at least under Kubuntu the contrast between the PIN background and foreground colors is bad (light grey vs. dark grey), whereas it's OK on Windows (white vs. dark grey). It's the same with Firefox, but that doesn't have to keep us from improving it. One possible solution would be to explicitly set the background color, too (to white).

> > See comment 3. Basically there are currently four Sync users: Firefox, Firefox
> > Mobile, Firefox Home (iPhone) and SeaMonkey. All but SeaMonkey and non-Windows
> > Firefox use Options. The strings in question refer to other devices so I
> > thought we'd be consistent with Firefox here, which would also better match
> > existing documentation (the guiding links go to Firefox's SUMO).
> Well, seeing as some of the strings you've previously provided use both...

Oh that's just from the Part 1 patch (must have qrefresh'd the wrong patch) so it's really a decision to be made in this bug. Just let me know what you think.

> > FTR: size="4" is not wide enough for WWWW (whereas width:4em is), but I think
> > that's a very unlikely edge case.
> Again, size="4" WWWWFM ;-)

Comparing Win7 (exceeds) with Kubuntu (WFM), it seems to be a minor OS/font difference issue.

> > > > +<!ENTITY setup.haveAccount.label "I already have a &syncBrand.fullName.label; account.">
> > > > +<!ENTITY button.connect.label "Connect">
> > > Bah, these are inconsistent :s
> > In which way? Locally? Looking at the whole file you'll see that the labels of
> > all syncSetup buttons have a "button." prefix.
> Right, but "I already have an account" is a button...

Either I don't get what you want or you're confused. The current state on trunk is that "I Have a SeaMonkey Sync Account" is a button. That is being replaced by a label + button combination through this bug, and there the button is "Connect".

> > had to introduce an extra param for focussing since onServerCommand (now
> > onServerSelect) was also called from onPageShow and we don't want to move the
> > focus in that case.
> I'd say that's a good reason not to use onselect. Sorry to trouble you.

Changed all on*Select occurrences back to on*Command locally.

> > I could also reproduce the "down arrow" selection issue but found no way to
> > work around it (not sure where your suggestion would need to be placed).
> I think it was before the setAttribute("editable") call.

Seems to work, added locally with the following comment:
// Prevent double selection upon using down key

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

Feedback from Neil over IRC:
1) disabled, readonly has too many issues
2) I think we need to mention Preferences
3) so, size="4" is ok?
4) I must be confused, let me try the patch again

My reply:
1) changed back to disabled locally
2) removed regression from Part 1
3) yes
4) I'm curious.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 2: Existing Account Wizard v1e

This should be it, assuming the Connect button confusion is resolved when you give this another try.

Comment 23

[[e-mail]]

Comment on attachment 522248 [details] [diff] [review]
Part 2: Existing Account Wizard v1e

>diff --git a/suite/common/sync/syncAddDevice.xul b/suite/common/sync/syncAddDevice.xul
This is going to be confusing. Are you going to land both patches in a single commit (or move these changes before you commit)?

>+  _updateControl: function(controlId, focus) {
[Because we switched back to command we always want to focus.]

>+      // Prevent double selection upon using down key
[Nit: p or .]


>+          <label control="server"
>                  value="&server.label;"/>
>-          <menulist id="serverType" oncommand="gSyncSetup.onServerChange();">
>+          <menulist id="server"
>+                    oncommand="gSyncSetup.onServerCommand();"
>+                    oninput="gSyncSetup.onServerInput();">
>             <menupopup>
>               <menuitem label="&serverType.main.label;"
>-                        value="main"/>
>-              <menuitem label="&serverType.custom.label;"
>-                        value="custom"/>
>+                        accesskey="&serverType.main.accesskey;"/>
>+              <menuitem label="&serverType.custom2.label;"
>+                        accesskey="&serverType.custom2.accesskey;"/>
Actually it was the menulist itself that was lacking an access key, although I have no objection to putting access keys on the menuitems.

Comment 24

[[e-mail]]

(In reply to comment #23)
> I have no objection to putting access keys on the menuitems.
Sorry, I forgot this doesn't looks as good on a menulist. Please remove the access keys from the menulist menuitems, and consider adding one to the menulist itself. With that, and the rest of the conditional menulist focusing reverted, r=me, as everything else seems to be OK now.

Comment 25

[[e-mail]]

Comment on attachment 522248 [details] [diff] [review]
Part 2: Existing Account Wizard v1e

>+        this.onServerCommand(true);
Please remove this line too, philikon was unable to justify it.

Comment 26

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 1: Add a Device Wizard v1d [Checkin: comment 28]

Comments 12 and 15 (size="4") addressed

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Part 2: Existing Account Wizard v1f [Checkin: comment 29]

Comments 23-25 addressed, with r=Neil from comment 24

Comment 28

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 522726 [details] [diff] [review]
Part 1: Add a Device Wizard v1d [Checkin: comment 28]

http://hg.mozilla.org/comm-central/rev/5f36003d805e

Comment 29

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 522727 [details] [diff] [review]
Part 2: Existing Account Wizard v1f [Checkin: comment 29]

http://hg.mozilla.org/comm-central/rev/ad530f75937a