Last Comment Bug 634419 - Port |Bug 602682 - Sync UI: Implement easy setup|
: Port |Bug 602682 - Sync UI: Implement easy setup|
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Sync UI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1b3
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on: 526445 601019 602682 618229 618300 618435 618704 620547 621466 621757 632952
Blocks: 605841 639339 642521 664569
  Show dependency treegraph
 
Reported: 2011-02-15 14:50 PST by Jens Hatlak (:InvisibleSmiley)
Modified: 2011-06-15 15:28 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
b3+


Attachments
Part 1: Add a Device Wizard (20.59 KB, patch)
2011-02-15 14:51 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 2: Existing Account Wizard (33.83 KB, patch)
2011-02-15 14:56 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 1: Add a Device Wizard v1a (21.98 KB, patch)
2011-03-06 00:43 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 1: Add a Device Wizard v1b (21.93 KB, patch)
2011-03-06 09:37 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 2: Existing Account Wizard v1a (33.82 KB, patch)
2011-03-06 09:38 PST, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 2: Existing Account Wizard v1b (36.01 KB, patch)
2011-03-13 08:10 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 1: Add a Device Wizard v1c (21.37 KB, patch)
2011-03-21 17:22 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review
Part 2: Existing Account Wizard v1c (34.34 KB, patch)
2011-03-21 17:25 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 2: Existing Account Wizard v1d (35.74 KB, patch)
2011-03-25 16:18 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 2: Existing Account Wizard v1e (34.94 KB, patch)
2011-03-27 14:35 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
Part 1: Add a Device Wizard v1d [Checkin: comment 28] (21.44 KB, patch)
2011-03-29 10:26 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
Details | Diff | Review
Part 2: Existing Account Wizard v1f [Checkin: comment 29] (33.62 KB, patch)
2011-03-29 10:29 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2011-02-15 14:50:37 PST
This will contain string changes, thus should block b3.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2011-02-15 14:51:57 PST
Created attachment 512603 [details] [diff] [review]
Part 1: Add a Device Wizard

632952
Comment 2 Jens Hatlak (:InvisibleSmiley) 2011-02-15 14:54:31 PST
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) 2011-02-15 14:56:13 PST
Created attachment 512606 [details] [diff] [review]
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) 2011-02-17 13:04:12 PST
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) 2011-03-06 00:43:40 PST
Created attachment 517235 [details] [diff] [review]
Part 1: Add a Device Wizard v1a

Updated for changes made in bug 636839.
Comment 6 Jens Hatlak (:InvisibleSmiley) 2011-03-06 09:37:34 PST
Created attachment 517277 [details] [diff] [review]
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) 2011-03-06 09:38:40 PST
Created attachment 517278 [details] [diff] [review]
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) 2011-03-13 08:10:52 PDT
Created attachment 519008 [details] [diff] [review]
Part 2: Existing Account Wizard v1b

Incorporated changes from bug 621757.
Comment 9 neil@parkwaycc.co.uk 2011-03-20 17:29:14 PDT
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) 2011-03-21 17:22:56 PDT
Created attachment 520809 [details] [diff] [review]
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) 2011-03-21 17:25:10 PDT
Created attachment 520810 [details] [diff] [review]
Part 2: Existing Account Wizard v1c
Comment 12 neil@parkwaycc.co.uk 2011-03-22 10:54:33 PDT
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) 2011-03-22 15:24:59 PDT
(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 neil@parkwaycc.co.uk 2011-03-24 09:37:06 PDT
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 neil@parkwaycc.co.uk 2011-03-24 14:45:08 PDT
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 neil@parkwaycc.co.uk 2011-03-24 15:09:37 PDT
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 neil@parkwaycc.co.uk 2011-03-24 15:12:29 PDT
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) 2011-03-25 16:18:29 PDT
Created attachment 522003 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-03-25 17:29:20 PDT
(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) 2011-03-26 15:17:36 PDT
(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) 2011-03-26 17:23:54 PDT
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) 2011-03-27 14:35:45 PDT
Created attachment 522248 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-03-28 02:02:23 PDT
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 neil@parkwaycc.co.uk 2011-03-28 02:20:17 PDT
(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 neil@parkwaycc.co.uk 2011-03-29 04:10:53 PDT
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) 2011-03-29 10:26:31 PDT
Created attachment 522726 [details] [diff] [review]
Part 1: Add a Device Wizard v1d [Checkin: comment 28]

Comments 12 and 15 (size="4") addressed
Comment 27 Jens Hatlak (:InvisibleSmiley) 2011-03-29 10:29:08 PDT
Created attachment 522727 [details] [diff] [review]
Part 2: Existing Account Wizard v1f [Checkin: comment 29]

Comments 23-25 addressed, with r=Neil from comment 24
Comment 28 Jens Hatlak (:InvisibleSmiley) 2011-03-29 12:49:54 PDT
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) 2011-03-29 12:50:31 PDT
Comment on attachment 522727 [details] [diff] [review]
Part 2: Existing Account Wizard v1f [Checkin: comment 29]

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

Note You need to log in before you can comment on or make changes to this bug.