Closed Bug 509044 Opened 11 years ago Closed 10 years ago

text beside icons in default mail toolbar

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: clarkbw, Assigned: bwinton)

References

Details

(Whiteboard: [has l10n impact])

Attachments

(4 files, 6 obsolete files)

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.
I wonder if philor would know what'd be involved.
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.
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.
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.
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 :)
Assignee: nobody → bwinton
Flags: wanted-thunderbird3+
this change has the opportunity to save a lot more vertical space than other tweaks so lets make it happen.
Flags: blocking-thunderbird3+
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.
Target Milestone: --- → Thunderbird 3.0rc1
Whiteboard: [no l10n impact]
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.
Status: NEW → ASSIGNED
Whiteboard: [no l10n impact] → [has l10n impact]
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.
Attachment #402113 - Flags: ui-review?(clarkbw)
Attachment #402113 - Flags: review?(philringnalda)
Whiteboard: [has l10n impact] → [has l10n impact][needs review philor][needs ui-review clarkbw]
Not because I wanted to! davida *made* me!
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.
Attachment #402113 - Flags: ui-review?(clarkbw) → ui-review+
+        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.
(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.
Whiteboard: [has l10n impact][needs review philor][needs ui-review clarkbw] → [has l10n impact][needs review 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.
(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.
Attachment #402113 - Attachment is obsolete: true
Attachment #402343 - Flags: review?(philringnalda)
Attachment #402113 - Flags: review?(philringnalda)
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.
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.
(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.
Attachment #402343 - Attachment is obsolete: true
Attachment #402343 - Flags: review?(philringnalda)
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.
Attachment #403032 - Flags: ui-review?(clarkbw)
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.
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.
Attachment #403078 - Flags: ui-review?(clarkbw)
Attachment #403078 - Flags: review?(philringnalda)
Comment on attachment 403078 [details] [diff] [review]
A patch for the previous screenshot.

There'll be a new patch coming sometime tomorrow.
Attachment #403078 - Flags: review?(philringnalda)
There's an annoying lack of ids in customizeToolbar.xul, so i agree you'd have to hack it adding the options through javascript :(
Just FTR - something like this, and hooking into the menupopup instead (if it only had an id).
Er, ignore the previous.
Attachment #403112 - Attachment is obsolete: true
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…)
Attachment #403078 - Attachment is obsolete: true
Attachment #403116 - Flags: ui-review?(clarkbw)
Attachment #403116 - Flags: review?(philringnalda)
Attachment #403078 - Flags: ui-review?(clarkbw)
Attachment #403032 - Flags: ui-review?(clarkbw)
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 :)
Attachment #403116 - Flags: review?(philringnalda) → review+
Whiteboard: [has l10n impact][needs review philor] → [has l10n impact][needs ui-r 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.
Attachment #403116 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attachment #403113 - Attachment is obsolete: true
Attachment #403295 - Flags: ui-review?(clarkbw)
Attachment #403295 - Flags: review?(philringnalda)
Comment on attachment 403295 [details] [diff] [review]
A fixed-up version of mkmelin's patch.

it's like christmas in september!
Attachment #403295 - Flags: ui-review?(clarkbw) → ui-review+
Whiteboard: [has l10n impact][needs ui-r clarkbw] → [has l10n impact][needs review 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
(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.
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.
(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.
Attachment #403295 - Attachment is obsolete: true
Attachment #403364 - Flags: review?(philringnalda)
Attachment #403295 - Flags: review?(philringnalda)
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.
Attachment #403364 - Flags: review?(philringnalda) → review+
Whew, http://hg.mozilla.org/comm-central/rev/087a5b97e85f - nice work!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has l10n impact][needs review philor] → [has l10n impact]
Depends on: 519413
Blocks: 519429
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?
Yup.  I'm working on it in bug 519429.
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?
(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.
You need to log in before you can comment on or make changes to this bug.