509044 - text beside icons in default mail toolbar

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: toolbar w./ text beside the icons

To save vertical space in the default mail header I think we should investigate using buttons with the text beside them instead of under them.  

With bug 474523 removing many of the toolbar buttons from the default toolbar there will be enough horizontal space to support this scheme.

So far in my test, see screenshot, I'm able to save 17px of space compared to the current layout and I think it's just as readable as the old layout.

To implement this I'd suggest that we add an additional toolbar layout item in the toolbar customize palette show option and default to this layout.

e.g.
[ Icons beside text | v ]
| Icons above text  |
| Icons             |
| Text              |
'-------------------'

The additional item will allow people to continue to customize the toolbar (or revert) as they see fit and will let our default toolbar layout be optimized for saving vertical space.

Comment 1

[David Ascher (:davida)]

I wonder if philor would know what'd be involved.

Comment 2

[rsx11m]

SeaMonkey has implemented that as part of bug 428216 (and related), as a right-click context menu to the toolbars. Maybe it gives some ideas/code.

Comment 3

[Andreas Nilsson (:andreasn)]

This will also make the address book window looks nicer, because it will get rid of some of the odd padding going on on the top of bottom of the search box.

Comment 4

[Phil Ringnalda (:philor)]

Yeah, what's involved in doing it SM-style is just adding an attribute, probably labelalign="end" to match SM, that you can use in CSS to set -moz-box-orient: horizontal, plus some toolbar contextmenu code to change it. What's involved in having the UI to change it in the customize toolbars dialog is some ugly overlaying of the toolkit JS and XUL, since we just recently unforked and stopped using our own customize code.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Ok, assigning this to Blake.  Lets do this SM-style so we can get it done quickly and easily.  I'm considering marking this blocking but I don't want Standard8 coming down on me; maybe I'll wait until after the conf call tomorrow :)

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

this change has the opportunity to save a lot more vertical space than other tweaks so lets make it happen.

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

It would be great to get this to land by b4 but I'm going to aim it for rc1 as it hasn't had the target milestone set and I don't think we'll have time for this.  The change is just porting an existing function so hopefully that will mean less pain for us when we grab it.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

At the very least, we're going to have new options in the right-click menu.  Hopefully the localizers can copy them from SeaMonkey, so it won't be too much work.

Later,
Blake.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to borrow the SeaMonkey behaviour.

Ah philor, I was going to ask asuth or mkmelin to review this, but then I noticed that you commented in the bug, from a position of authority, so you get the r? instead.  ;)

Thanks,
Blake.

Comment 10

[Phil Ringnalda (:philor)]

Not because I wanted to! davida *made* me!

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 402113 [details] [diff] [review]
A patch to borrow the SeaMonkey behaviour.

Just change the default mode to be text-beside icons and it's ui-r+ from me.

Comment 12

[Philip Chee]

+        toolbar.removeAttribute("ignoremodepref");
+        toolbar.setAttribute("ignoremodepref", "true");
+    document.persist(toolbar.id, "ignoremodepref");
You don't need these. It's there to support some SeaMonkey global prefs that don't exist in Thunderbird.

The constructor in our extended toolbox binding isn't necessary but you might consider that for 3.next.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #11)
> Just change the default mode to be text-beside icons and it's ui-r+ from me.

(In reply to comment #12)
> +        toolbar.removeAttribute("ignoremodepref");
> +        toolbar.setAttribute("ignoremodepref", "true");
> +    document.persist(toolbar.id, "ignoremodepref");
> You don't need these. It's there to support some SeaMonkey global prefs that
> don't exist in Thunderbird.

I've fixed both of these, and have a new patch, but would like to wait to see what changes philor suggests before I post it.

Thanks,
Blake.

Comment 14

[Phil Ringnalda (:philor)]

None that I can see (after all, I know how brutally it's already been reviewed for SeaMonkey) - let's have the new patch.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: The new patch, with clarkbw's and Philip Chee's nits fixed.

(In reply to comment #14)
> None that I can see (after all, I know how brutally it's already been reviewed
> for SeaMonkey) - let's have the new patch.

Huh, I'm shocked.  Well, here's the new patch.

Thanks,
Blake.

Comment 16

[Phil Ringnalda (:philor)]

Yeah, my screwup, I should have applied it to see that SM's UI wouldn't work for us.

clarkbw: new ui-wanted. SM can get away with having per-toolbar context menu access to text-beside as the only UI, because they aren't using it as default, and they don't have any access to customization other than from the context menu. For them, you only have text-beside if you discovered that you can right-click a toolbar and set it there. For us, there'll be three classes of users: people with no idea they can customize, who'll just have to live with it; people who right-click to customize, who'll have a chance of discovering that even though nothing in the customization dialog changes it, that new Settings For This Toolbar submenu while right-clicking to get to the customization dialog does change it, and people who think that customization is something you do from the View menu, who will have no idea that right-clicking would give them new and different UI.

Three choices I can see:

* don't turn it on by default, and work on getting it into the toolkit dialog for 3.next as "Icons and Text Below/Icons and Text Beside"

* have it not be set per-toolbar, so the context menu changes every toolbar not just the one you clicked, which would let the View menu have

View
Toolbars > Mail Toolbar
           My Custom Bar
           ------------
           Settings    >  Icons and Text
           Customize...   Icons
                          Text
                          ---------------
                          Show text beside
                          Use small
                          Use defaults

(which makes it pretty weird that we duplicate the icons&text/icons/text/small thing in the menu/context menu and also in the dialog, without the excuse of the dialog being global and the context menu being per-toolbar, but is maybe better than)

* keep it per-toolbar, and have a copy of the context menu for every toolbar in the View menu, like

View
Toolbars > Mail Toolbar
           My Custom bar
           ------------
           Settings    >  Mail Toolbar   > Icons and Text
                          My Custom Bar  > Icons
                                           Text
                                           ---------------
                                           Show text beside
                                           Etc.

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

Oh good!  I didn't think I'd have the chance to muck with this UI.  :)

In terms of per-toolbar options I would let the simpler code make the decision there.  Longer term I think your 1st option, at least getting this option into toolkit, is a necessary step for the 3.1 time frame.  Since that toolkit option would likely be across all toolbars it's probably best to stick with not per-toolbar setting now.

I'd really like to have this as the default option as it evens out our new "toolbar on a diet" in terms of used horizontal space and gets back that vertical space we're using with the tabs row.

So if I assume we can make this a global text beside icons option I'd suggest an addition to the view menu similar to what you are proposing.  I don't think we need all the options (use small, use defaults, etc) but really just enough options to alter what we've made newly available.

View
Toolbars > Mail Toolbar
           Custom Toolbarz Yo!
           -------------------
           Settings           >  Icons and Text
                                 Icons beside Text
                                 Icons Only
                                 Text Only
           Customize...

I'm a bit unsure about "Icons and Text" if we should change it to be "Icons above Text" in order to stand out more from our new option.

Likely we could use a similar Settings menu popup for the right click menu.

A minimal set like that has some positives in that we don't duplicate more than we need to.  The major visible change we'd be defaulting to becomes more visible in the menu for people to reset without actually customizing.

The negatives would be that some people will see some of the settings there and want all the settings pulled in.  I would dupe these bugs against the toolkit but that implements text beside icons in the Show menu.  Once we have a new toolkit toolbar palette we'd likely pull the Settings menu all together and go back to just having Customize... as our path for change.

Comment 18

[Phil Ringnalda (:philor)]

(In reply to comment #17)
> Oh good!  I didn't think I'd have the chance to muck with this UI.  :)

That's the thing about toolbar customization: no matter how hard you try to scrape it off your shoe, days or weeks or months later you start to sniff, and look around, and then you realize you've still got customization on your shoe.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Attachment: What if we remove all the duplicate config?

We could just have the "Show text beside icon" menu item there (disabled if we're not on "icons and text" mode), and also throw it in the View » Toolbars menu.

How does that look to you (and Andreas), for an interim solution?

Thanks,
Blake.

Comment 20

[Magnus Melin [:mkmelin]]

Was there a reason we just don't overlay and add "text beside icon" to the Show dropdown in the toolbar customization? Personally i don't think it need to be more visible than that, if that's what was the worry here.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch for the previous screenshot.

Okay, let's see how we all like this version.

Magnus: The main reason I've gone this route was that it seemed like an easier road to go down, especially this close to the string freeze.  At first we thought that we could just copy the SeaMonkey implementation, and slip it in.  Even now that we know we want to use our own UI, it's still only 69 lines of changes.

Finally, I'm not entirely sure how I would slide in the changes to the mode list.  I wouldn't want to override the whole customizeToolbar.xul.  I suppose I could use some javascript to add in the new mode, and then somehow override initDialog function to initialize the modelist correctly based on the mode and the labelalign, and the restoreDefaultSet function to restore it correctly, and maybe the updateToolbarMode function, too…  Well, it just seemed trickier than doing it the way I did in the patch.

(If it's actually easier to overlay, please send me a hacked-together WIP patch, and I'll be more than happy to clean it up and drive it in before the string freeze.)

Thanks,
Blake.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 403078 [details] [diff] [review]
A patch for the previous screenshot.

There'll be a new patch coming sometime tomorrow.

Comment 23

[Magnus Melin [:mkmelin]]

There's an annoying lack of ids in customizeToolbar.xul, so i agree you'd have to hack it adding the options through javascript :(

Comment 24

[Magnus Melin [:mkmelin]]

Attachment: non-working-hacked together overlay

Just FTR - something like this, and hooking into the menupopup instead (if it only had an id).

Comment 25

[Magnus Melin [:mkmelin]]

Attachment: non-working-hacked together overlay

Er, ignore the previous.

Comment 26

[Blake Winton (:bwinton) (:☕️)]

Attachment: A more cleaned up version of my previous patch.

It's my daughter's birthday today, so I'm probably not going to start trying to get a patch based on mkmelin's until tomorrow.  In the mean time, this one seems to do what we want.  (And we could add another patch based on mkmelin's after this…)

Comment 27

[Phil Ringnalda (:philor)]

Comment on attachment 403116 [details] [diff] [review]
A more cleaned up version of my previous patch.

I'm not entirely sure this is the UI we want, but r=me for this being the way to do it if it is :)

Comment 28

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 403116 [details] [diff] [review]
A more cleaned up version of my previous patch.

This bug makes me feel like saying something inspiring like: "You go to release with the UI you have.  It's not the UI you might want or wish to have at a later time."  Wow that feels dirty just to read much less say out loud, thanks Rumsfeld!.

Lets hope we can get the UI we actually want so perhaps we can hold off on the checkin until just before its too late.

Comment 29

[Blake Winton (:bwinton) (:☕️)]

Attachment: A fixed-up version of mkmelin's patch.

Okay, so, let's see if we can drive this version of the patch in by tomorrow, so that we don't have to go to 403116, a.k.a. the safety patch.  ;)

Thanks,
Blake.

Comment 30

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 403295 [details] [diff] [review]
A fixed-up version of mkmelin's patch.

it's like christmas in september!

Comment 31

[Phil Ringnalda (:philor)]

Notes mostly to myself for now, while I'm away from a build env:

* customizeToolbarOverlay.xul needs a license block (and I bet we've landed other things already which also do, by not meeting the "oh, come on, nobody licenses CSS or strings" rule)

* you shouldn't ever need to use "*+" in a jar.mn, because * always replaces - some of the times that we've used "*+" in the past may have been intending to say "even if you drop my preprocessing, I still want the 'replace even if the source timestamp is older' behavior, because I'm replacing an existing file" but that's both not the case here, where you're not preprocessing *or* replacing, and not likely to really tell some random future person that anyway

* don't forget that those dump()s need to go

Comment 32

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #31)
> Notes mostly to myself for now, while I'm away from a build env:

I'll fix them anyways, while I'm here.  :)

> * customizeToolbarOverlay.xul needs a license block (and I bet we've landed
> other things already which also do, by not meeting the "oh, come on, nobody
> licenses CSS or strings" rule)

Fixed.  (I also added one for the dtd, but it feels kind of silly.)

> * you shouldn't ever need to use "*+" in a jar.mn

Replaced with *.

> * don't forget that those dump()s need to go

Fixed.

While I was there, I changed "customizeToolbar.dtd" to "customizeToolbarOverlay.dtd" to match "customizeToolbarOverlay.xul" and "baseMenuOverlay.dtd".

I should have another free block of time in about four hours, if you find other stuff by then.

Thanks,
Blake.

Comment 33

[Phil Ringnalda (:philor)]

I won't be home before then - if I were you, I'd attach the new version, to make me feel guilty about any other nits, so I'd fix them myself and push it.

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with some of philor's nits fixed.

(In reply to comment #33)
> I won't be home before then - if I were you, I'd attach the new version, to
> make me feel guilty about any other nits, so I'd fix them myself and push it.

Well, never let it be said that I didn't take reviewer's suggestions.  :)

(I'll be on for a while tonight, if you find other less nit-y stuff.)

Thanks,
Blake.

Comment 35

[Phil Ringnalda (:philor)]

Comment on attachment 403364 [details] [diff] [review]
The previous patch, with some of philor's nits fixed.

I managed to deal with the rest of my nits, which were just filing the serial numbers off those copy-pasted license blocks, and switching the <string> to a <data> since data hides itself for us.

Comment 36

[Phil Ringnalda (:philor)]

Whew, http://hg.mozilla.org/comm-central/rev/087a5b97e85f - nice work!

Comment 37

[Thomas Stache]

The new nightly is out, and toolbar customization is busted.

http://hg.mozilla.org/comm-central/rev/087a5b97e85f#l2.59
	getElementById("labelAlignToolbar").setAttribute("checked", end);
fails, and since it is followed by an almost identical line with
	getElementById("labelAlign").setAttribute("checked", end);
I fear the line is bogus, and shouldn't be there at all?

Comment 38

[Blake Winton (:bwinton) (:☕️)]

Yup.  I'm working on it in bug 519429.

Comment 39

[Joachim Herb]

If changing to Show: "Icons beside Text" and then pressing "Restore Default Set" the drop down menu of "Show:" becomes blank. Is this a new bug?

Comment 40

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #39)
> If changing to Show: "Icons beside Text" and then pressing "Restore Default
> Set" the drop down menu of "Show:" becomes blank. Is this a new bug?

I think it's more related to bug 519607, but since that's also been resolved fixed, you should probably create it as a new bug.

Note: When I did the steps to reproduce on my latest build, I didn't get a blank "Show:" dropdown, but I instead got it showing "Icons and Text", but with the icons beside the text, which still isn't really right.