This contains string changes, so should block b3.

Attachment 500519 [details] shows why the pane was changed for Firefox: Their former "Manage Account" button was expanding a list of links that moved the following content further below. We chose to not go that way and use buttons instead (also with the benefit of having accesskeys). But I think this decision needs to be revisited now: If you compare the attached screen shot for our UI with attachment 500428 [details] from FF's UI, you'll see: * I copied the use of a drop-down menu because I think this is superior to both using an expanding button and using buttons * I added the accesskeys we already had to the new menuitems * I kept the "Stop Using This Account" item even though it's gone in current FF trunk (it does the same as their "Deactivate this device" which I left out intentionally) * There are no icons for any of the menuitems, neither in FF's nor our proposed UI, only in attachment 500428 [details] * I moved the drop-down after the account name (which I kept for easy copy+paste) to save space (and at the same time use the space that is there) * I kept our main groupbox caption because I think it's better to make it clear that this "Sync" pane actually refers to "SeaMonkey Sync" (and we have the account name below instead of in the caption) * I copied the moving of the individual sync items to the new richlistbox because it's better for extensibility (and IMO also looks better) * I copied the removing of the second caption, because "Browser Sync" was not really helpful anyway IMO What you cannot see (from the screen shot): * I kept the buttons for the error case (where FF is still using text links) * There is no Add a Device in our UI yet because that's up for bug 634419 (which I'll need to adapt to the new situation created here, but that's not for now) Neil, if you'd let Ian do both r and ui-r, that would be OK as well.

Comment on attachment 515186 [details] [diff] [review] patch [Checkin: comment 13] >+ <label value="&accountName.label;" control="accountName"/> >+ <textbox id="accountName" readonly="true"/> >+ <button type="menu" >+ label="&manageAccount.label;" >+ accesskey="&manageAccount.accesskey;"> The textbox needs to have flex="1" in case the labels are so long that they would otherwise overflow. >+ <richlistbox id="syncEnginesList" Why is this a richlistbox? I guess a <listbox rows="5"> with checkbox type listcells might work, if you really want it to be able to scroll. > <!-- Manage Account --> > <!ENTITY manageAccount.label "Manage Account"> >+<!ENTITY manageAccount.accesskey "A"> > <!ENTITY viewQuota.label "View Quota"> > <!ENTITY viewQuota.accesskey "V"> > <!ENTITY changePassword.label "Change Password"> > <!ENTITY changePassword.accesskey "C"> > <!ENTITY mySyncKey.label "My Sync Key"> > <!ENTITY mySyncKey.accesskey "M"> > <!ENTITY resetSync.label "Reset Sync"> > <!ENTITY resetSync.accesskey "e"> > <!ENTITY stopUsingAccount.label "Stop Using This Account"> >-<!ENTITY stopUsingAccount.accesskey "A"> >+<!ENTITY stopUsingAccount.accesskey "o"> This is a bad choice of letter, since it's a lower case vowel. "S" might be OK, but there might be confusion with "Reset Sync" "A" might be OK, but you would want to change "Manage Account" to "M". [Popup accesskeys only conflict within the popup, so it wouldn't be strictly necessary to change it, but it might be just confusing.] While you're here, I think the other access keys are all wrong too, my favourites would be "Q", "P", "K" and either "R" or "S" (not sure which). >+.syncGroupBox { >+ padding: 10px; Why?

(In reply to comment #1) > * I copied the moving of the individual sync items to the new richlistbox > because it's better for extensibility (and IMO also looks better) OK, I hadn't read this bit. I just don't like the hack used to get the richlistbox to work. How about a vbox with auto overflow?

(In reply to comment #2) > The textbox needs to have flex="1" in case the labels are so long that they > would otherwise overflow. Good point, changed locally. > >+ <richlistbox id="syncEnginesList" > Why is this a richlistbox? I guess a <listbox rows="5"> with checkbox type > listcells might work, if you really want it to be able to scroll. > > * I copied the moving of the individual sync items to the new richlistbox > > because it's better for extensibility (and IMO also looks better) > OK, I hadn't read this bit. I just don't like the hack used to get the > richlistbox to work. How about a vbox with auto overflow? When I said extensibility, I was thinking about an extension adding new Sync engines. Doing this differently than FF makes things more complicated for extension developers, for no apparent benefit. > This is a bad choice of letter (...) > While you're here, I think the other access keys are all wrong too I'll leave that whole "issue" to Ian who suggested the accesskeys we currently have. > >+.syncGroupBox { > >+ padding: 10px; > Why? Why not? Anyway, removed locally.

One thing I forgot to mention is that the Connect button has been removed (cf. bug 526445 comment 40). I agree it's for the better.

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #1) > > > * I copied the moving of the individual sync items to the new richlistbox > > > because it's better for extensibility (and IMO also looks better) > > OK, I hadn't read this bit. I just don't like the hack used to get the > > richlistbox to work. How about a vbox with auto overflow? > When I said extensibility, I was thinking about an extension adding new Sync > engines. Doing this differently than FF makes things more complicated for > extension developers, for no apparent benefit. Sure, but it's unlikely that extensions will require the container to be a richlistbox, as long as it has the same id.

Comment on attachment 515186 [details] [diff] [review] patch [Checkin: comment 13] >+#syncEnginesList { >+ height: 10em; >+} One last thing, can you make this a max-height: 12em; please. (I didn't really want any sort of height restriction at all and just have it scroll when it runs out of room but this was the best I could do.)

(In reply to comment #6) > Sure, but it's unlikely that extensions will require the container to be a > richlistbox, as long as it has the same id. FTR: As discussed on IRC, I'd like to keep it not so much for the richlistbox but for the use of richlistitems which extensions developers are likely to use if they add engines to Firefox Sync. (In reply to comment #7) > One last thing, can you make this a max-height: 12em; please. Changed locally.

> FTR: As discussed on IRC, I'd like to keep it not so much for the richlistbox > but for the use of richlistitems which extensions developers are likely to use > if they add engines to Firefox Sync. I think this is a bogus argument. Speaking as an extension developer who keeps a close eye on the extension development forums I think this is unlikely and since they are overlaying a different window/prefpane they would have to modify their overlays anyway.

Comment on attachment 515186 [details] [diff] [review] patch [Checkin: comment 13] >+++ b/suite/locales/en-US/chrome/common/pref/pref-sync.dtd > <!-- Login error feedback --> > <!ENTITY updatePass.label "Update"> > <!ENTITY updatePass.accesskey "U"> > <!ENTITY resetPass.label "Reset"> > <!ENTITY resetPass.accesskey "s"> This could be "R" > > <!-- Manage Account --> > <!ENTITY manageAccount.label "Manage Account"> >+<!ENTITY manageAccount.accesskey "A"> > <!ENTITY viewQuota.label "View Quota"> > <!ENTITY viewQuota.accesskey "V"> > <!ENTITY changePassword.label "Change Password"> > <!ENTITY changePassword.accesskey "C"> > <!ENTITY mySyncKey.label "My Sync Key"> > <!ENTITY mySyncKey.accesskey "M"> > <!ENTITY resetSync.label "Reset Sync"> > <!ENTITY resetSync.accesskey "e"> > <!ENTITY stopUsingAccount.label "Stop Using This Account"> >-<!ENTITY stopUsingAccount.accesskey "A"> >+<!ENTITY stopUsingAccount.accesskey "o"> And this could be "S" now, the problem with "o" is that it is next to a letter with a descender ("p"). According to https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies there is no problems using vowels generally. >+#syncEnginesList { >+ height: 10em; Yes, this definitely needs to be more than 10em to avoid scrolling in modern with just the standard list of Engines. I don't think it needs to be a richlist, as Philip says, extension authors will need a specific overlay for SeaMonkey anyway. r=me with those issues fixed/addressed.

(In reply to comment #10) > > <!ENTITY resetPass.label "Reset"> > > <!ENTITY resetPass.accesskey "s"> > This could be "R" Well, "r" is used for History, too. I guess that's OK now that it's inside the popup? > >+#syncEnginesList { > >+ height: 10em; > Yes, this definitely needs to be more than 10em to avoid scrolling in modern > with just the standard list of Engines. Neil's 12em is OK for Classic, but Modern needs 13em. If this approach is to be followed (see below why I'm not so sure about that), should we have both values equal? > I don't think it needs to be a richlist, as Philip says, extension authors will > need a specific overlay for SeaMonkey anyway. OK. Tried the vbox, but that looks totally different (plain), and adding a background color, border and margin/padding seems a bit too much tweaking to achieve visual equality. Then I found that the Scripts & Plugins pane is already using a listbox, without a rows value but with flex, so that's what I have locally now, with the height/max-height styles removed and no overflow (which is unnecessary in this case, the scrollbars appear if I force a smaller height). Would that be OK? Also I added a colon after "Sync My".

(In reply to comment #10) > According to https://developer.mozilla.org/en/XUL_Accesskey_FAQ_and_Policies > there is no problems using vowels generally. Huh, I wonder where I picked up that no-lowercase-vowel policy from then. (In reply to comment #11) > I guess that's OK now that it's inside the popup? Access keys in popups can only conflict with other elements in the popup. (Menuitems without access keys act as if they have an access key given by their first letter, if it's not already used as a menuitem access key.)

Comment on attachment 515186 [details] [diff] [review] patch [Checkin: comment 13] http://hg.mozilla.org/comm-central/rev/2387bfa59240 with richlistbox/richlistitem/checkbox replaced by listbox/listitem type=checkbox, new entity for My Sync (due to added colon) and without extra CSS rules