Closed
Bug 509044
Opened 15 years ago
Closed 15 years ago
text beside icons in default mail toolbar
Categories
(Thunderbird :: Toolbars and Tabs, defect)
Thunderbird
Toolbars and Tabs
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)
28.79 KB,
image/png
|
Details | |
37.70 KB,
image/png
|
Details | |
10.58 KB,
patch
|
philor
:
review+
clarkbw
:
ui-review+
|
Details | Diff | Splinter Review |
21.55 KB,
patch
|
philor
:
review+
|
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.
Comment 1•15 years ago
|
||
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.
Comment 3•15 years ago
|
||
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•15 years ago
|
||
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.
Reporter | ||
Comment 5•15 years ago
|
||
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+
Reporter | ||
Comment 6•15 years ago
|
||
this change has the opportunity to save a lot more vertical space than other tweaks so lets make it happen.
Flags: blocking-thunderbird3+
Reporter | ||
Comment 7•15 years ago
|
||
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
Updated•15 years ago
|
Whiteboard: [no l10n impact]
Assignee | ||
Comment 8•15 years ago
|
||
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]
Assignee | ||
Comment 9•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has l10n impact] → [has l10n impact][needs review philor][needs ui-review clarkbw]
Comment 10•15 years ago
|
||
Not because I wanted to! davida *made* me!
Reporter | ||
Comment 11•15 years ago
|
||
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+
Comment 12•15 years ago
|
||
+ 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.
Assignee | ||
Comment 13•15 years ago
|
||
(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]
Comment 14•15 years ago
|
||
None that I can see (after all, I know how brutally it's already been reviewed for SeaMonkey) - let's have the new patch.
Assignee | ||
Comment 15•15 years ago
|
||
(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)
Comment 16•15 years ago
|
||
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.
Reporter | ||
Comment 17•15 years ago
|
||
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•15 years ago
|
||
(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.
Updated•15 years ago
|
Attachment #402343 -
Attachment is obsolete: true
Attachment #402343 -
Flags: review?(philringnalda)
Assignee | ||
Comment 19•15 years ago
|
||
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)
Comment 20•15 years ago
|
||
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.
Assignee | ||
Comment 21•15 years ago
|
||
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)
Assignee | ||
Comment 22•15 years ago
|
||
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)
Comment 23•15 years ago
|
||
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•15 years ago
|
||
Just FTR - something like this, and hooking into the menupopup instead (if it only had an id).
Comment 25•15 years ago
|
||
Er, ignore the previous.
Updated•15 years ago
|
Attachment #403112 -
Attachment is obsolete: true
Assignee | ||
Comment 26•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #403032 -
Flags: ui-review?(clarkbw)
Comment 27•15 years ago
|
||
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+
Updated•15 years ago
|
Whiteboard: [has l10n impact][needs review philor] → [has l10n impact][needs ui-r clarkbw]
Reporter | ||
Comment 28•15 years ago
|
||
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+
Assignee | ||
Comment 29•15 years ago
|
||
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)
Reporter | ||
Comment 30•15 years ago
|
||
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+
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has l10n impact][needs ui-r clarkbw] → [has l10n impact][needs review philor]
Comment 31•15 years ago
|
||
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
Assignee | ||
Comment 32•15 years ago
|
||
(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•15 years ago
|
||
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.
Assignee | ||
Comment 34•15 years ago
|
||
(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 35•15 years ago
|
||
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+
Comment 36•15 years ago
|
||
Whew, http://hg.mozilla.org/comm-central/rev/087a5b97e85f - nice work!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [has l10n impact][needs review philor] → [has l10n impact]
Comment 37•15 years ago
|
||
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?
Assignee | ||
Comment 38•15 years ago
|
||
Yup. I'm working on it in bug 519429.
Comment 39•15 years ago
|
||
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?
Assignee | ||
Comment 40•15 years ago
|
||
(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.
Description
•