526445 - Rearrange Sync prefs panel

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Maaren]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4
Build Identifier: Weave 0.8

Just expand everything
-> panel isn't big enough
(see image)

just don't work with arrows, and set the other items there by default?
(eventually grayed out like in the privacy pref. panel)

Reproducible: Always

Comment 1

[Maaren]

Attachment: The preference panel of Weave 0.8 is to small

Comment 2

[Mike Connor [:mconnor]]

Yeah, either the pane needs to expand, or they can't both be open at once. Taking.

Comment 3

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/0ca68f1cb53d
fix this properly for Mac (actually animate/resize appropriately)

Still need to make this work well for Windows/Linux.

Comment 5

[Mike Connor [:mconnor]]

Going to punt this, it's okay for beta (manage account is rarely used, but when it is, it's fully accessible) for Windows/Linux.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: screenshot on Ubuntu, with "Unknown error" bar pushing stuff even lower

Note that this bug is exacerbated by the bright-red "Unknown Error" bar described in bug 528855.  See attached screenshot, taken from Weave 1.0b1 on a mozilla-central nightly.

(So far this bug hasn't a problem on Ubuntu -- everything fits, even when expanded -- but the error bar pushes us just beyond the available space.)

Any solution to this bug needs to take that (usually-hidden) error bar into consideration. (assuming that it's still a part of Weave when this gets fixed)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> Any solution to this bug needs to take that (usually-hidden) error bar into
> consideration.

s/Any solution/A complete solution/

Comment 8

[Philip Chee]

In the Browser Sync -> Use my custom settings, instead of a VBOX put the five checkboxes in a two column grid.

Browser Sync? Doesn't Weave support Thunderbird?

Comment 9

[Mike Connor [:mconnor]]

This is a lot better now, just a bit of minor cosmetic stuff now.  Punting to 1.1.

(In reply to comment #8)
> In the Browser Sync -> Use my custom settings, instead of a VBOX put the five
> checkboxes in a two column grid.
> 
> Browser Sync? Doesn't Weave support Thunderbird?

Not really, removing support for now in Bug 533580

Comment 11

[Leszek Zyczkowski]

I tested 1.3b5, and still this bug occurs. Now aren't displayed also Terms of Service, and Privacy Policy.

Comment 12

[Mike Connor [:mconnor]]

This is fixed on trunk on all platforms.

Comment 13

[Darren Kalck [:D-Kalck]]

I'm using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100929 Firefox/4.0b7pre Built from http://hg.mozilla.org/mozilla-central/rev/942b5b6e74e4
and it's still not fixed.

Comment 14

[Philipp von Weitershausen [:philikon]]

Correct, when you open the Manage Account drop down, the stuff below still gets pushed off the screen on Windows. We have plans to redesign this prefpane, though, don't we? Perhaps we should morph this bug into that.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> Perhaps we should morph this bug into that.

(Or, if that ends up as a separate bug, then this one should depend on that one.)

Comment 17

[Dietrich Ayala (:dietrich)]

No update in a couple of months. Is this still a problem? Sync people, can you comment as to why this should block (or not)?

Comment 18

[Benjamin Smedberg]

I saw this yesterday on Windows.

Comment 19

[Alex Limi (:limi) — Firefox UX Team]

I assume what needs to be fixed here is that the pref pane needs to expand further to accommodate the error message? 

If you want a new UI for this, add back "uiwanted", but I think the bug is in the preference pane size calculation at the moment? Tested this using the current nightly on OS X, and it works for me — but maybe it's platform-specific? (I was unable to trigger the Sync error message, though)

Comment 20

[Philipp von Weitershausen [:philikon]]

(In reply to comment #19)
> I assume what needs to be fixed here is that the pref pane needs to expand
> further to accommodate the error message? 

No. We want to redesign the Sync prefpane. It's a clusterfuck right now with that Manage Account expander. mconnor, faaborg and I sat down at the All Hands and discussed the redesign and I believe faaborg had some mockups in the works already.

> If you want a new UI for this, add back "uiwanted", but I think the bug is in
> the preference pane size calculation at the moment?

It's *a* bug, but not this bug.

> Tested this using the
> current nightly on OS X, and it works for me — but maybe it's
> platform-specific? (I was unable to trigger the Sync error message, though)

It is platform specific. I think we can't dynamically resize dialogs on Windows.

Comment 21

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #20)
> No. We want to redesign the Sync prefpane. It's a clusterfuck right now with
> that Manage Account expander. mconnor, faaborg and I sat down at the All Hands
> and discussed the redesign and I believe faaborg had some mockups in the works
> already.

Aha. I didn't know about that, sorry. Just trying to resolve the uiwanted bugs for the current blockers. :)

Comment 22

[Michael Lefevre]

(In reply to comment #20)
> > If you want a new UI for this, add back "uiwanted", but I think the bug is in
> > the preference pane size calculation at the moment?
> 
> It's *a* bug, but not this bug.

Hrm... well, it *was* this bug, until it got morphed into what it is now. Should someone file a new bug for the stuff getting pushed out of the dialog? In my view, that bug is more likely to remain a blocker than redesigning the UI (although if that happens, then great).

Comment 23

[Michael Lefevre]

Sorry, please ignore that last comment - I've just noticed that bug 563349 is already filed and depending on this.

Comment 24

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

Attachment: New prefpane layout

This layout ensures that the UI won't overflow (pushing the terms of service and privacy policy off scree), even if we add more account options or data types.

It also more logically groups actions that are taken on the account, versus actions that are taken on the machine (at the moment it is easy to believe that a data type is for the specific machine, and not all machines associated with the account).

Comment 25

[Philipp von Weitershausen [:philikon]]

(In reply to comment #24)
> Created attachment 500428 [details]
> New prefpane layout

Lovely, thank you! What's the difference between "Stop Using This Account" in the drop down menu and the "Deactivate this device" link down at the bottom?

Comment 26

[Philipp von Weitershausen [:philikon]]

I should also note that the Deactivate this device link would require a new string and that the glyphs in the drop down menu don't exist yet either. (We don't have any of faaborg's nice mockup glyphs yet in the wizards -- I guess it's all part of bug 591122.)

Comment 27

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

Sorry, forgot to add a note about that.  I added "stop using this account" for the time being, since we have the string.  But later, I think we should remove that and switch to "deactivate this device" since the action is being carried out on the device, and not the whole account.  Also it has a nice symmetry to the control for adding a device.  At the moment "stop using this account" seems to imply that it will deactivate all of your devices, and possibly destroy all of your data, when in reality it just cuts off access for the current device, and removes the local data (right?).

Comment 28

[Philipp von Weitershausen [:philikon]]

(In reply to comment #27)
> Sorry, forgot to add a note about that.  I added "stop using this account" for
> the time being, since we have the string.  But later, I think we should remove
> that and switch to "deactivate this device" since the action is being carried
> out on the device, and not the whole account.  Also it has a nice symmetry to
> the control for adding a device.

I agree! Apart from the string freeze, is there anything that's keeping us from doing the change now? I'd say let's change this while we're at it... tentatively adding [strings].

> At the moment "stop using this account" seems
> to imply that it will deactivate all of your devices, and possibly destroy all
> of your data, when in reality it just cuts off access for the current device,
> and removes the local data (right?).

Right (if by removing local data you mean removing Sync settings -- it doesn't remove anything you've synced).

Comment 29

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

>Apart from the string freeze, is there anything that's keeping us from
>doing the change now?

Nope, just the string freeze

>Right (if by removing local data you mean removing Sync settings -- it doesn't
>remove anything you've synced).

Right, much later if we implement multiple user accounts, we'll at that point probably want to actually clear off the device (the assumption is that you are giving the computer away).  You can of course then undo the operation  by simply logging in again.

Comment 30

[Philip Chee]

Addons.mozilla.org just mass mailed addon authors that it's safe to update their extensions to 4.0.*
<http://blog.mozilla.com/addons/2010/12/29/add-ons-can-now-be-compatible-with-4-0/>
Presumably this includes extensions that add their own entry to the list of services synced?

Comment 31

[Philipp von Weitershausen [:philikon]]

(In reply to comment #30)
> Addons.mozilla.org just mass mailed addon authors that it's safe to update
> their extensions to 4.0.*
> <http://blog.mozilla.com/addons/2010/12/29/add-ons-can-now-be-compatible-with-4-0/>
> Presumably this includes extensions that add their own entry to the list of
> services synced?

Currently we have no plans making that list extensible. That doesn't mean we can't do it, but it's a separate issue. Please file a new bug if you want to discuss this, this bug is purely about rearranging the existing UI.

Comment 33

[eyal gruss]

Attachment: tabs option is cropped

Comment 34

[eyal gruss]

Attachment: both tabs and history options are cropped

Comment 36

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 37

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Rearranges the Sync prefs panel according to faaborg's mockup. It does not add any of the additional glyphs the mockup contains. It also does not contain the string change. I will do this in a separate patch so that we can always decide whether we want to take it or not (I think we should.)

This also fixes bug 616488 since I had to move that code around anyway.

Comment 38

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3: String change (v1)

Move the "Stop Using This Account" menu item to a "Deactivate This Device" label, as suggested by the mockups.

Comment 39

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1.1): Rearrange prefs panel

Move the CSS for the height of the engines list into theme code, that way it can be different on various platforms. Make it a tad bigger on Windows.

Comment 40

[Philipp von Weitershausen [:philikon]]

One thing that the v1.1 doesn't do yet is remove the Connect button. I've thought long and hard about this, but I can't find a case where we'd still need it in the UI, especially with bug 616488 fixed in this patch as well.

Comment 41

[Philipp von Weitershausen [:philikon]]

Attachment: Win7 screenshot of v1.1

Comment 42

[Philipp von Weitershausen [:philikon]]

Attachment: Mac screenshot of v1.1

Comment 43

[Philipp von Weitershausen [:philikon]]

Attachment: screenshot of Wrong Sync Key error (v1.1 patch)

Comment 44

[Philipp von Weitershausen [:philikon]]

Attachment: screenshot of Incorrect Password error (v1.1 patch)

Comment 45

[Philipp von Weitershausen [:philikon]]

Comparing the screenshots with faaborg's mockups, I see that we could use a bit more padding for the groupboxes and maybe a little less around the checkboxes. I'd like to get faaborg's feedback first, though.

Alex?

Comment 46

[:Atoll]

Comment on attachment 501438 [details]
Win7 screenshot of v1.1

faaborg: There's an instance of inconsistent wording in this dialog.  It should be either "Device Name", or "Deactivate This Computer", but not the current mix of "Computer" and "Device" both referencing the same thing.  "Add a Computer" doesn't make any sense at all, so I suggest "Device Name" as the fix.

Comment 47

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 501441 [details]
Mac screenshot of v1.1

Actually, this isn't a select pulldown, so it shouldn't look like one. It's an "inline menu", so should look more like the Bookmarks button in our UI — an equivalent in OS X is the little "Gear" menu in the Finder.

The select pulldown implies that you're making one of the options active, which isn't what is happening here. :)

Comment 48

[Philipp von Weitershausen [:philikon]]

(In reply to comment #47)
> Actually, this isn't a select pulldown, so it shouldn't look like one.

I know! It *is* a <button type="menu">, so I think I'm using the right widget and it's just wrongly styled by default. So yell at the XUL guys plz :p

> It's an "inline menu", so should look more like the Bookmarks button in our
> UI — an equivalent in OS X is the little "Gear" menu in the Finder.

These don't have text, though.

> The select pulldown implies that you're making one of the options active, which
> isn't what is happening here. :)

Yup, agreed. Will see if we can steal a style from the toolbar buttons which seem to be appropriately styled. This still feels like an upstream XUL bug that we maybe should address. Will investigate and possibly file follow-up.

Comment 49

[Philipp von Weitershausen [:philikon]]

Ok, I dabbled with the Mac style last night, but I couldn't get it to do what you wanted. At this point we should defer this a follow up. I filed bug 623722 on the XUL/widget guys.

Comment 50

[Stefan [:stefanh]]

As I said in bug 623722 the currently styled button looks like a "Command Pop-Down Menu" Note that it differs from a menulist.

From the Apple HIH;
"A command pop-down menu is similar to a pull-down menu, but it appears within
a window rather than in the menu bar. A command pop-down menu presents a list
of commands that affect the containing window’s contents.

The colors window (in for example textedit) has an example (the palette view).

So, I'm not sure the current styling is wrong. It might not be what you want, though.

Comment 51

[Stefan [:stefanh]]

Fwiw, here's a link to an image of the colors window:
http://developer.apple.com/library/mac/documentation/UserExperience/Conceptual/AppleHIGuidelines/art/ct_openpopdownmenu.jpg

Comment 52

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

But even in that screen shot there is another visually identical control that behaves differently (selecting the image that is displayed, currently set to spectrum).  While this perhaps isn't technically a problem, a toolbar style drop down seems like it would disambiguate the type of control a bit more. (oh how I wish these HIGs were wikis... :)

Comment 53

[Stefan [:stefanh]]

(In reply to comment #52)
> But even in that screen shot there is another visually identical control that
> behaves differently (selecting the image that is displayed, currently set to
> spectrum).

Hey, it has 2 arrows ;-)

Comment 54

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Get rid of Connect/Disconnect button

Comment 55

[Philipp von Weitershausen [:philikon]]

Limi just said in bug 623722 comment 9:

> I wouldn't make it block anything, and I'm fine with shipping the aqua-style
> down arrow inline menu in FF4 — but it does look weird and feel weird. :)

So pending reviews from mconnor and ui feedback from faaborg, I think this is good to go.

Comment 56

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1.2): Rearrange prefs panel

Update Part 1 slightly to provide 10px padding on Windows for the groupboxes, as per faaborg's offline feedback. This is ready for landing now.

Comment 57

[Tony Chung [:tchung]]

needs litmus tests when this is fixed

Comment 58

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1.3): Rearrange prefs panel

Discovered a small bug: we would switch to the Update/Reset page on any login error, even when it was due to the network or server. This now makes sure we only go to that page on wrong password or sync key.

Comment 59

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1.1): Get rid of Connect/Disconnect button

Rebase on top of Part 1 (v1.3). All observers now go through updateWeavePrefs, so simplified the observer registering logic as well.

Comment 60

[Philipp von Weitershausen [:philikon]]

Got r+ from mconnor via IRC for the updated bits, landed:

Part 1: https://hg.mozilla.org/mozilla-central/rev/b1a940a22511
Part 2: https://hg.mozilla.org/mozilla-central/rev/2ec3f1dcd2c4
Part 3: https://hg.mozilla.org/mozilla-central/rev/c0e05d518f57

Comment 61

[Francesco Lodolo [:flod]]

Not sure if this is the right place to ask, but this bug is the only reference I have from Mercuarial's history of /browser/preferences/sync.dtd. 

Why exactly did you choose the word "deactivate"? If the original action is "Add a device", the logical opposite should be "Remove this device". An alternative, knowing what the label does, could be "Disconnect this device". 

I'm asking because I have to translate this string and I'd like to know the reasons for this choice.

Comment 62

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

Disconnect might imply that it is a momentary action (especially since we previously had a button for connecting and disconnecting).  So we needed a stronger word.  Remove would work just as well, but deactivate seemed even stronger, and got across that the device would definitely no longer have access to the account.

Comment 63

[Leszek Zyczkowski]

Attachment: Sync panel - new design

Comment 64

[Leszek Zyczkowski]

Maybe is too late, but I  have new mockup for Sync window. See attached image. What is a plus:
Clear window
Less clicks for get function
Accesskeys - very important for many users
Buttons width going to translated strings automatically
Clear XUL code
Minus:
Have to add accesskeys for translation.

I have ready to use sync.xul file.

Comment 65

[Tracy Walker [:tracy]]

verified with current nightly build

Comment 66

[[e-mail]]

Comment on attachment 503042 [details] [diff] [review]
Part 1 (v1.3): Rearrange prefs panel

>+              <richlistbox id="syncEnginesList"
>+                           orient="vertical"
>+                           onselect="if (this.selectedCount) this.clearSelection();">
What's the point of a richlistbox if you're not going to use it as a list?
If you want a scrollable region, just set an overflow style...

Comment 68

[Tracy Walker [:tracy]]

yes, covered and recovered in-litmus