Closed Bug 509044 Opened 11 years ago Closed 10 years ago
text beside icons in default mail toolbar
28.79 KB, image/png
37.70 KB, image/png
10.58 KB, patch
|Details | Diff | Splinter Review|
21.55 KB, patch
|Details | Diff | Splinter Review|
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
this change has the opportunity to save a lot more vertical space than other tweaks so lets make it happen.
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
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.
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.
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.
(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.
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.
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 on attachment 403078 [details] [diff] [review] A patch for the previous screenshot. There'll be a new patch coming sometime tomorrow.
Just FTR - something like this, and hooking into the menupopup instead (if it only had an id).
Er, ignore the previous.
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 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.
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.
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]
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.