465138 - New message-reader UI elements in header pane need to be customizable

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[rsx11m]

This is a follow-up on bug 449691, which introduced buttons and drop-down menus to the header pane of the message reader, and bug 450724, introducing new UI elements to the address lists. There was substantial discussion specifically on the action buttons in the header pane, with several suggestions to make those configurable (also an item in Dan's "polish" list for TB3). I don't see this filed yet, therefore this spin-off bug.

Since customization is already possible for the main toolbar items, it would be consequent to also introduce those for the additional UI elements in the header pane. Users may consider them distracting, don't like the redundancy, find that they are taking up too much screen real estate, or want to remove them for other reasons (e.g., junk filtering done at the server, or don't want to accidentally hit the delete button).

The only mechanism provided thus far are respective "display: none" entries in userChrome.css, thus a more user-friendly mechanism should be provided. Whether this has to be the full palette of toolbar customizations or if simple on/off switches for buttons, menus, and address items would suffice appears to be the question to start with.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Agreed that this is a worthwhile goal.  I suspect this wouldn't block Thunderbird 3, despite the fact that it's arguably a regression.  I'm interested in others' thoughts here...

Comment 2

[Ronald Killmer]

The new Message-Reader header bar does need polishing both in it's look and  functionality. To date We still have the old layout of collapsed Col for Signed/Encrypted messages. My complaint with that Col is partially the retention of the upper spacer that causes the icons to be hidden with use of overflow control and a massive quantity of header lines.

My view is We can do a UI refresh using the concept employed with the Inline Abook popup.  Add icons for Signed and Encrypted that provide access to the related additional information, such as the Cert. This can resolve the inconvenient placement of the Tb 2 popup dialog. See Bug 456618

I have taken a look at the DOM structure with DOMi and have some disagreement with the DOM tree. To facilitate customizing the new buttons I believe a separate hbox is better since it can draw on the same bindings, etc. as the toolbars. Call it toolbar-3 perhaps. 

I disagree with the placement of the Drafts string and Edit button.  We loose too much Message View height when a draft is selected. The old location of that button in a collapsed Col was unacceptable with overflow control. So the question is, Is it feasible to add the button to the top line of the header bar. Either as a collapsed box between the From and the new buttons, or as a conditional display toggle with the set of new buttons.

Comment 3

[Magnus Melin [:mkmelin]]

Agreed we wouldn't block on this.

Comment 4

[rsx11m]

Based on comment #1 from Dan, I was expecting a wanted‑thunderbird3+, but it appears I'll have to renominate that explicitly then.

(In reply to comment #2)
> I have taken a look at the DOM structure with DOMi and have some disagreement
> with the DOM tree. To facilitate customizing the new buttons I believe a
> separate hbox is better since it can draw on the same bindings, etc. as the
> toolbars. Call it toolbar-3 perhaps. 

The right-click > Customize feature is already known by most users for the other toolbars, thus they would expect a similar functionality for the new buttons as well, I agree with that. If the full toolbar customization appears overdone, a right-click menu with checkable items (similar to the columns in the thread pane) would be another feasible option coming into my mind.

Comment 5

[Joe Sabash [:JoeS1]]

For RSS feeds the header pane shows useless buttons like "reply"
For newsgroups, where threads are normally signed and quoted, most of the "mail"
features are superfluous.(Don't need to know the sender, when looking at a sig
Each users wants/needs are different, so this UI should be customizable.

In viewing some saved compositions,(graphics) I might have lost as much as 20%
of my vertical screen real estate.

There is also the possibility of incorporating an f11 option, but in the meantime
please provide a way to select the tools we want.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #4)
> Based on comment #1 from Dan, I was expecting a wanted‑thunderbird3+, but it
> appears I'll have to renominate that explicitly then.

A while back, we (tb-drivers) started being stricter about our usages of wanted and blocking, which is to say that we're only marking stuff as wanted if we _really_ think there's a high chance we'll be able to find the developer-power to get this done and into the tree.  So by that qualification, I don't think this would really make the cut, even though we'd very much like to see this happen.

Comment 7

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

If someone has the time to work on this I can make time to help.  I think this is really important as well; though it was agreed (I think reasonably so) to not block the release if nobody has the time to work on this.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: most important piece: change the XUL widgetry to toolbar widgetry, v1

Whether or not we get to the point of shipping a full toolbar palette that users can manipulate directly, we very much want this to at least be a toolbar with toolbar buttons.  This patch does that work, but there are still some styling issues (on Mac, at least).  If someone else wants to pick this up, work through the CSS issues, and drive it into the tree, that would be fantastic.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

My last comment failed to mention why the above patch is the high-order bit here: without it, it's a pain for extension authors to add buttons as well.

Comment 10

[Alex Ihrig [:Thunderbird_Mail_DE]]

-      <xul:hbox align="start">
+      <xul:toolbar align="start">

AFAIK Add-ons need an ID for the toolbar to overlay it.

Comment 11

[Y-H]

How about something like this: (1) full view (default), (2) hide labels (currently: "hide details"), and (3) minimalist view (a one-line view, similar to what we get with TB2)? This would eliminate the need to build in a method for fully customizing the header pane, but satisfy users (like me) who prefer TB2's abbreviated pane.

Comment 12

[Alex Ihrig [:Thunderbird_Mail_DE]]

(In reply to comment #11)
> How about something like this: (1) full view (default), (2) hide labels
> (currently: "hide details"), and (3) minimalist view (a one-line view, similar
> to what we get with TB2)? This would eliminate the need to build in a method
> for fully customizing the header pane, but satisfy users (like me) who prefer
> TB2's abbreviated pane.

This wouldn't be sufficient for Add-ons to hook in.

Comment 13

[Alex Ihrig [:Thunderbird_Mail_DE]]

(In reply to comment #12)
> This wouldn't be sufficient for Add-ons to hook in.

From an Add-on developers point of view it's frustrating to get no help / no possibility to hook into the new header pane UI. Tb3 is promoted with it's extensibility, but we are still no able to adapt our UI elements.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #10)
> -      <xul:hbox align="start">
> +      <xul:toolbar align="start">
> 
> AFAIK Add-ons need an ID for the toolbar to overlay it.

XUL generated from XBL works a little bit differently: you don't get to overlay it directly; you can, however, create a binding that extends the original binding, I believe.  Bug 490042 may also be relevant here; I still need to work through that.

(In reply to comment #11)
> How about something like this: (1) full view (default), (2) hide labels
> (currently: "hide details"), and (3) minimalist view (a one-line view, similar
> to what we get with TB2)? This would eliminate the need to build in a method
> for fully customizing the header pane, but satisfy users (like me) who prefer
> TB2's abbreviated pane.

We do need to figure out what our in-app customization UI for the message header is for Tb3; if you could file another bug on that and assign it to me, that would be very helpful.  As Alexander says, that's separate from this bug, however.
 
(In reply to comment #13)
> 
> From an Add-on developers point of view it's frustrating to get no help / no
> possibility to hook into the new header pane UI. Tb3 is promoted with it's
> extensibility, but we are still no able to adapt our UI elements.

Apologies for not getting back to this bug sooner.  As per <http://weblogs.mozillazine.org/dmose/archives/2009/06/thunderbird_compact_header_mov.html>, the compact header has been removed specifically to make room for doing a better job with the one header that remains.  Happily, that means this gets to go on the blocking list.

Comment 15

[rsx11m]

Carrying over a suggestion from a related discussion which would fit here:

> (bug 474523 comment #44) To make a one-click solution available for the
> toolbar, the Start menu in Windows XP may actually provide a nice template. In
> its properties, you can select between the "new" or "classic" versions (the
> latter mimicking Win2k and before). To apply this to the case here, there could
> be three toolbars in total: (1) your current toolbar with all customizations;
> (2) the new toolbar at the same location with a different ID and the default
> settings proposed in your patch; (3) the toolbar in the header pane with action
> buttons. This would allow to show either (1) (thus hiding the action buttons in
> the process) to retain the classic view, or (2)+(3) for the new view with the
> buttons being located in the header pane. A boolean or radio item in the
> toolbar context menu could toggle between those two options.

Since the bug here would make the header-pane buttons a regular toolbar, switching it on and off should be feasible. Just look at View > Layout, offering three different pane arrangements, as another example.

There is also bug 505169 motivating a Text/Icon choice for the header-pane toolbar. Given that Text+Icon wouldn't make much sense here, the third option could be None in case a View > Toolbars solution isn't implemented.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

This version makes all the buttons except for the reply button configurable, and it also adds a context menu with a toggle for switching between text-only and icons-only.  clarkbw, I'd be curious to hear your thoughts on that...

Right now, I've only made the CSS changes and tested it on Linux.

For some reason the CSS doesn't work right in text mode on the buttons, even if I go one step further than this patch and force the various buttons to be "-moz-appearance: button;" instead of the default "-moz-appearance: toolbarbutton;".

Once that gets fixed, there's some cleanup to do before this is ready for review.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Appended the wrong version last time around; let's try again.

Comment 18

[Thomas D. (:thomas8)]

(In reply to comment #16)
Dan, it's great to hear this is making progress, even though I can only judge from the comments.
> a context menu with a toggle for switching between text-only and icons-only
In Bug 505169, comment #9 and bug 505169, comment #10 there was some consensus between rsx11m and me that it may not be necessary nor wise to exclude the "icon+text" option. Basically, depending on your screen size, space may not be an issue at all, and I can already see people (like me ;)) crying that they can't have icons AND text (which even Thunderbird 2 can do for toolbar buttons...)!

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Every menu item that exists has a cost in terms of visual complexity, and in terms of how much choice the user is forced to deal with.  Generally, we only want to accept those costs for cases where we think each choice is critically important to provide in the core.

My suspicion is that having both isn't enough of a win to merit the visual & interaction costs of taking the context menu from two items to three items.  That said, at the end of the day, it will be Bryan's call to make.

If that feature doesn't end up in core, it should very little work for someone to put together an extension to offer it.

Comment 20

[Peter Lairo]

(In reply to comment #19)
> Every menu item that exists has a cost in terms of visual complexity,

An omission also adds complexity in terms of confusion and frustration as to where the missing item might be.

Comment 21

[rsx11m]

Well, if a toolbar customization for text/icon/text+icon is readily available from another piece of code whereas a text/icon-only version would have to be implemented as a special case, I'd agree that this would increase complexity
of the code unnecessarily. I'd nevertheless like to see an option to hide the header-button toolbar completely, be it through the customization options or
a View > Toolbar menu item.

Comment 22

[Thomas D. (:thomas8)]

(In reply to comment #20)
> (In reply to comment #19)
> > Every menu item that exists has a cost in terms of visual complexity,
>
> An omission also adds complexity in terms of confusion and frustration as to
> where the missing item might be.

Spot-on, and I couldn't agree more. And not just for this bug...
Maybe the deeper problem behind this is the apparent lack of design guidelines, which in turn causes a need for a lot of separate decisions that are often based on individual feelings and inconsistent. Design guidelines could and would make a lot of things easier, both for users and developers. To name but a few:
- Focus takes keyboard input. Always.
- Mouseclick moves Focus. Always.
- All properties of an object should be accessible from any visual representation of that object (a FF issue; TB attachments, too...)
- ...
- User can customize keyboard shortcuts, by assigning them to commands (not vice versa).
- ...
- Toolbar buttons can be customized: icon/text/icon+text. All of them.
Having said which, I'm aware that design guidelines also involve decisions and feelings, and the minimalist approach also has something of a design guideline. But still...

Or maybe I just don't know where the UI design guidelines are. Please enlighten me!

Comment 23

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

Comment on attachment 399006 [details] [diff] [review]
patch, v2

This seems like it's going in the right direction.

Are we able to provide the standard toolbar customization palette?  I honestly don't know and am curious what it would look like.

Also with toolbarbuttons we get flat, large buttons that raise on hover instead of the buttons we have now.  Is that just CSS work that can overcome this?

(In reply to comment #20)
> (In reply to comment #19)
> > Every menu item that exists has a cost in terms of visual complexity,
> 
> An omission also adds complexity in terms of confusion and frustration as to
> where the missing item might be.

Neither of these states are a final end point.  We are always striking a balance between too much and too little.  We're looking for least confusion and frustration not the end of frustration and confusion as it doesn't exist.

Comment 24

[Peter Lairo]

Please include bug 516141 in any customization solution - preferably as default.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v3 (WIP)

After more arduous brawling with XUL and CSS, here's a still very-much WIP patch.  It's still Linux only, but now it looks more or less right, both in icon and text mode.

Things learned along the way:

<toolbarbutton> is highly optimized to look like the old-school OS-level toolbars at the top of the main window.  With significant use of !important, specificity hacking, and -moz-appearance, it can be bludgeoned into looking and acting like a regular button, however.  I think we're in reasonable shape here, though in an ideal world, one would probably end up refactoring <toolbarbutton> stuff somewhat to support this use case better.

<toolbar> and friends are not written to be used from XBL.  It doesn't look like it would be _too_ much work to fix that, and I've got it working in XBL at the moment, but it feels fragile.  There's some chance that I'll need to either do some work on toolkit infrastructure, or that I'll need to pull this out of XBL into XUL before it lands.  The latter would be at cross-purposes to this bug in some degree, I suspect, because it would make life hard for the various compact header extensions.

DOMi sometimes(?) draws it's boxes such that they don't include the margin, which can be very deceptive when debugging.

It looks like making the "customize toolbar" dialog work here is going to be enough work that we shouldn't let it drag this down with it: let's spin that off to another bug.

Next coding steps:

* throw together a trivial extension to make sure that it is, as hoped, easy to 
add items to the button box now
* clean up the use of toolbar from XBL, decide whether it's reasonable to leave things that way for landing
* port changes to other themes
* do basic re-sorting & commenting of the CSS in messageHeader.css so that it's less awful to work with in the future (might need to wait for a future bug)

Next design steps:

* make a decision on which of "icons / text / both" end up in the context menu. I've made my opinion clear here, but I don't feel terribly strongly about it.

* decide what to do about the icons.  This patch currently just re-uses existing ones, inspired by clarkbw's ovidiu-header repo.  Note that we currently don't have differing icons for reply/reply-all/reply-list.  My suspicion is that we'll want at least some new icons.

* this patch keeps the trash button as an icon, even in text mode.  Though inconsistent, it feels ok to me, but that may be because I'm used to it.

Comment 26

[Joachim Herb]

Just a suggestion: Take a look at All-in-One Sidebar: https://addons.mozilla.org/firefox/addon/1027 (It is licensed as MPL 1.1/GPL 2.0/LGPL 2.1)

It adds a toolbar to the sidebar. You can add all icons to this toolbar which you can also add to the main toolbar. An icon can even be visible at the same time in the main toolbar and in the sidebar toolbar.
Why not add a toolbar to the header pane into which you can drop icons from the main toolbar including the reply (all/list), forward, etc. buttons? Then you could even add icons like print, tag, etc.

Another suggestion: Create a toolbar button "other actions". Then the separate menu button wouldn't be needed anymore.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Sounds worthwhile, but I think it's beyond this scope of this specific bug, which is to get basic customization working in time for shipping Thunderbird 3.0.

Comment 28

[Günter]

Attachment: a tweak/proposal for the header discussion

@26 & @27
Yes, it seems to be an excellent idea having a sidebar for the message display.
I think it makes very much sense, the area for the message could be maximized with collapsing the header to just one line, all the required/configured functions/buttons would be positioned to the sidebar. 
Saving vertical space is just to display the icons, the tooltiptext would give enough explanation -- also to the inexperienced user.

Note: the jpg is based on compactHeader.xpi, see here:http://forums.mozillazine.org/viewtopic.php?f=29&t=1405155

Comment 30

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

(In reply to comment #25)
> Next design steps:
> 
> * make a decision on which of "icons / text / both" end up in the context menu.
> I've made my opinion clear here, but I don't feel terribly strongly about it.

I think it's worthwhile to have all three.  I think text is the mode we have right now.  I would suggest adding in the new mode for text+icons such that reply and forward both have icons but the rest remain the same.  And then the icons only mode as you already have it.

For strings I'd go with something like this:

(o) Show Icons and Text
( ) Show Icons Only
( ) Show Text Only

> * decide what to do about the icons.  This patch currently just re-uses
> existing ones, inspired by clarkbw's ovidiu-header repo.  Note that we
> currently don't have differing icons for reply/reply-all/reply-list.  My
> suspicion is that we'll want at least some new icons.

I'm adding Andreas for this.  We can reuse the existing reply (to sender) and forward icons from the thread view / message list area as the header is already doing.

We need some similar small sized reply all and reply list icons to use for those buttons.  That's two for each platform (win+lin,aero,and mac).

> * this patch keeps the trash button as an icon, even in text mode.  Though
> inconsistent, it feels ok to me, but that may be because I'm used to it.

I think this is ok for icons only and icons + text.  I think if we're going for all 3 modes then the text mode should really be all text.

Everything works really well so far and looks good.  Nice work on the CSS wrangling.

Comment 31

[Andreas Nilsson (:andreasn)]

the images from main-toolbar-small should work well for the reply-all and reply-list as they are 16x16 as well.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Andreas: on Mac, the images in main-toolbar-small are 32x32 also, unlike the trash image, which is 16x16.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

I was looking at the wrong part of the CSS file; it appears that they're actually 24x24 on Mac, unlike the trash image, which is 16x16.

Looks like they are 16x16 on Linux, however.

Comment 34

[Andreas Nilsson (:andreasn)]

indeed, those are 24x24, we should be allright on widows too, as the main-toolbar-small use 16x16 there. 16x16 for mac coming up!

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v4 (WIP)

Changes:

* Icons and Text, Icons Only, Text Only implemented
* Icons and Text is default
* Skin changes ported to Mac, but it doesn't have icons yet, so it looks kinda weird.  Re-sorted the Mac skin CSS for somewhat less insanity.
* Tooltip strings separate from those in the primary toolbar added, since some of those talk about "the selected messages".  First letter of these tooltips is lowercased for consistency with the buttons themselves.  clarkbw, does this sound reasonable?
* It's now possible to extend the toolbar by overlaying buttons onto <header-view-button-box> elements.  

The main things left to do, besides the mac icons are:

* understand the limits of the extensibility here, see if the current method is robust enough to ship
* understand the limits of using abusing the toolbar XBL the way we are and see whether they're robust enough to ship
* test against ovidiu-header and whether this breaks it in any interesting ways
* port theme changes to Vista & Aero
* misc cleanup

Comment 36

[Thomas D. (:thomas8)]

(In reply to comment #35)
> Created an attachment (id=403158) [details]
> patch, v4 (WIP)
> * Icons and Text, Icons Only, Text Only implemented

Could you post a screenshot of the three modes?

> * Icons and Text is default

I am failing to understand why Trash button is treated as a different kind of animal. It should behave like any other of those buttons with respect to text, icon, icon and text display modes. Given the complete non-recognizability of the trash icon, I'd even say it is most important for trash icon to have text in the relevant modes. Plus, in default header view, there's ample space horizontally to host those buttons.

> lowercased for consistency with the buttons themselves.

I sometimes wonder why the buttons have lower case in the first place... is it supposed to look more modern? On the other hand, it matches with lower-case header labels like from, to, subject.

> The main things left to do, besides the mac icons are:
> * understand the limits of the extensibility here

Does this patch bring me some way of choosing which buttons I'd like to see on the header bar?

Comment 37

[Andreas Nilsson (:andreasn)]

Attachment: reply-to-list and reply-to-all for pinstripe

here are the pinstripe icons!

Comment 38

[Joachim Herb]

Are you sure it's a good idea to copy the definition of the toolbar buttons to another place (from primaryToolbar.css to messageHeader.css). This adds redundancy to the code which is hard to maintain. A cleaner solution would be the way described in bug 515091 comment #4 (change the definitions of the toolbar icons to classes). Another way would be to copy the icon definition from the toolbar to the header pane menu buttons (see http://www.mozdev.org/source/browse/compactheader/srcExtension/chrome/CompactHeader/content/compactHeaderOverlay.js?rev=1.25;content-type=text%2Fplain function copyButtonIcons)

Comment 39

[Magnus Melin [:mkmelin]]

Did a quick test of the wip patch. One problem i noticed: have multi-msg-summary show, going back to the msg header the buttons are gone and do not reappear.

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #36)
> (In reply to comment #35)
> > Created an attachment (id=403158) [details] [details]
> > patch, v4 (WIP)
> > * Icons and Text, Icons Only, Text Only implemented
> 
> Could you post a screenshot of the three modes?

Will do later today, I expect.

> > * Icons and Text is default
> 
> I am failing to understand why Trash button is treated as a different kind of
> animal.

In the new patch, it's not.
 
> > The main things left to do, besides the mac icons are:
> > * understand the limits of the extensibility here
> 
> Does this patch bring me some way of choosing which buttons I'd like to see on
> the header bar?

Unfortunately not as part of the default UI, as there's not time left in this release to make the "Customize Toolbar" UI work.  I suspect it would be possible to write an extension to do it, however.

(In reply to comment #37)
> Created an attachment (id=403204) [details]
> reply-to-list and reply-to-all for pinstripe
> 
> here are the pinstripe icons!

We actually need icons for all the buttons for pinstripe buttons for "icons only" mode, not just those two, since I can't steal 24x24 icons from the toolbar-small set.

(In reply to comment #38)
> Are you sure it's a good idea to copy the definition of the toolbar buttons to
> another place (from primaryToolbar.css to messageHeader.css). 

primaryToolbar.css seems to be mostly about the general look-and-feel of native-OS-style toolbars at the top of various windows, as well as the specifically about the ones that appear on the 3-pane window and the standalone message window.  The other native-OS-style toolbars (addressbook and compose) are already styled elsewhere.  

The thing I'm adding is somewhat different, in that it very specifically does not try and use native-OS-style, and is intended to be localized in effect only to the msgHdrViewOverlay.xul, which makes it seem like it belongs with that code, in messageHeader.css.

You're right that this could all use a good cleanup, but I don't think now's the right time for that.

> This adds redundancy to the code which is hard to maintain.

What redundancy are you referring to, exactly?  The references to the image areas from toolbar-main-small?

> A cleaner solution would be
> the way described in bug 515091 comment #4 (change the definitions of the
> toolbar icons to classes). 

Ah, I see what you mean.  If I get time, I'll look at this when I'm a little further along in the cleanup-before-I-post-for-review phase of this bug.

Also worth noting is that I expect bug 509044 to land before this patch, and I'll get to/have to do some cleanup as a result of that.

Comment 41

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

Should 509044 block this, then?

(BTW, I expect something similar to the latest patch for it to be what eventually lands, if you wanted to get started on the cleanup early.)

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v5 (WIP)

Patch v5 has a number of changes:

*) msgHeaderView-button hdr*Button rules in gnomestripe sorted for precedence and sanity
*) icons for archive & junk hidden in toolbar "full" mode; since the icons for those two aren't terribly meaningful, we'd rather trade them away for the horizontal space
*) removed <header-view-button-box> from XBL entirely and installed it into XUL.  This is significantly less fragile than what we had, largely because toolbar.xml and friends was not written to be used from XBL.  It makes it possible to customize the button bar in significantly more ways, and it should be less difficult to implement "Customize Toolbar" now (though this patch doesn't do that).  It fixes the problem mkmelin saw before, afaict.  It comes at the cost of no longer being quite so easy for extensions to reuse: instead of instantiating an XBL widget, they need to fork the front-end XUL.  Perhaps someday when someone has the bandwidth to fix up toolbar.xml and friends for use in XBL, we can try this again...

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v6

Changes in this iteration:

* fixed merge conflicts
* got toolbar mode persistence working
* various cleanup

Comment 44

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 403430 [details] [diff] [review]
patch, v6

Note that this patch isn't quite as scary it as looks: a fair bit of it is just code motion:

* the toolbar stuff has moved from XBL to XUL

* in the CSS, the more general msgHdrViewButton rules appear before the more specific hdr*Button rules, both because that had to happen for precedence reasons, and because it makes the rules notably more readable.

Two things are missing from this patch:

* a bunch of 16x16 Mac icons; waiting on these from andreasn
* the CSS rule changes to go along with those icons
* Windows CSS changes.  bwinton was kind enough to whip them up and mail them to me before he called it a night.  Unfortunately the intertubes seem all gummed up tonight, and the email hasn't yet arrived.

I'll get all this stuff in order tomorrow morning and cons up a second patch for separate review.

Comment 45

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: text only mode on linux

Comment 46

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: icons only mode on linux

Comment 47

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: text and icons mode for linux

Comment 48

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

Attachment: WIP - The Windows css changes.

(In reply to comment #44)
> * Windows CSS changes.  bwinton was kind enough to whip them up and mail them
> to me before he called it a night.  Unfortunately the intertubes seem all
> gummed up tonight, and the email hasn't yet arrived.

Just in case my mail still hasn't arrived, here's the patch.  It needs tweaking, but it seems mostly there.

Later,
Blake.

Comment 49

[Andreas Nilsson (:andreasn)]

Attachment: small updates to blakes patch

really hard to try this with a busted thunderbird :)

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 403430 [details] [diff] [review]
patch, v6

git apply says there are 20 lines with whitespace errors.

>+            oncommand="document.getElementById('header-view-toolbar').setAttribute('mode', 'full');
>+                       document.persist('header-view-toolbar', 'mode')"/>
>+  <menuitem id="header-toolbar-show-icons" type="radio"
>+            name="header-toolbar-menu-mode-group" 
>+            label="&hdrViewToolbarShowIcons.label;"
>+            accesskey="&hdrViewToolbarShowIcons.accesskey;"
>+            oncommand="document.getElementById('header-view-toolbar').setAttribute('mode', 'icons');
>+                       document.persist('header-view-toolbar', 'mode')"/>

Maybe the js here should be in a function?

>+                            observes="button_replyall"/>
>+                  <menuseparator/>

>+                            observes="button_replylist"/>
>+                  <menuseparator/>

Ids for the separators would be nice

>+<!ENTITY hdrArchiveButton.label "archive">
>+<!ENTITY hdrArchiveButton.tooltip "archive this message">

While the labels are lowercase, i don't see a reason not to capitalize the tooltips.


>+              <toolbarbutton id="hdrTrashButton" anonid="hdrTrashButton"

Don't need both id and anonid i assume?

>+/* This icon isn't that meaningful, and we'd rather save horizontal space */
>+toolbar:not([mode="full"]) > .hdrArchiveButton {

It's a bit odd to have an "text+icons" mode but then don't show images for more than part of the buttons.

Works fine afaikt, so r=mkmelin

Comment 51

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

Comment on attachment 403433 [details]
text only mode on linux

easy! looks good

Comment 52

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

Comment on attachment 403434 [details]
icons only mode on linux

easy!  looks good

Comment 53

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #50)
> (From update of attachment 403430 [details] [diff] [review])
> git apply says there are 20 lines with whitespace errors.

Doh!  I figured the jst-bot would have caught those, but apparently not. 
Fixed, and I've figured out how to drive "hg qdiff" and "less" together so that
I can see these things more easily.
 
> >+            oncommand="document.getElementById('header-view-toolbar').setAttribute('mode', 'full');
> >+                       document.persist('header-view-toolbar', 'mode')"/>
> >+  <menuitem id="header-toolbar-show-icons" type="radio"
> >+            name="header-toolbar-menu-mode-group" 
> >+            label="&hdrViewToolbarShowIcons.label;"
> >+            accesskey="&hdrViewToolbarShowIcons.accesskey;"
> >+            oncommand="document.getElementById('header-view-toolbar').setAttribute('mode', 'icons');
> >+                       document.persist('header-view-toolbar', 'mode')"/>
> 
> Maybe the js here should be in a function?

setAndPersistToolbarMode() added to msgHdrViewOverlay.js.

> Ids for the separators would be nice

Good catch; thanks!

> >+<!ENTITY hdrArchiveButton.label "archive">
> >+<!ENTITY hdrArchiveButton.tooltip "archive this message">
> 
> While the labels are lowercase, i don't see a reason not to capitalize the
> tooltips.

Bryan's already done a ui-review here, and I don't care much either way, so unless he chimes back in suggesting that I change it, I'm going to leave this as is.

> >+              <toolbarbutton id="hdrTrashButton" anonid="hdrTrashButton"
> 
> Don't need both id and anonid i assume?

Indeed not; the anonid was leftover from before.  Removed.

> >+/* This icon isn't that meaningful, and we'd rather save horizontal space */
> >+toolbar:not([mode="full"]) > .hdrArchiveButton {
> 
> It's a bit odd to have an "text+icons" mode but then don't show images for
> more than part of the buttons.

Agreed that it's weird on the consistency front, but trading away consistency
in favor of winning back some real estate seems worth it to me.

Comment 54

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch 1, v7

Comment 55

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

Comment on attachment 403435 [details]
text and icons mode for linux

here's where we try to strike a balance between consistency (for the sake of consistency) and space saving and usability.  Not sure why all three of those are balancing but that's what is happening.

We have very strong icons for reply, forward, and delete while our icons for archive and junk are much weaker visual elements so we lose those to save space and don't lose usability in the process.

Comment 56

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch 1, v8

Fix a typo.

Comment 57

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch 2, v1

Patch 2, version 1, is intended to be applied on top of patch 1.  I'll land them both at once, possibly merging them first.  Changes:

* get rid of the CSS that applied the now-obsolete header-view-button-box binding
* uses folderpane icons for junk and archive; these should be permanent
* uses a couple of 16x11 symbol icons for "reply to sender" and "forward".  We'll need new ones of the right size, but Andreas isn't feeling well, and the icons can happen after string freeze.  I'll file a followup bug.
* merges in the Qute CSS port from Blake and Andreas with a bit of comment cleanup.  I haven't tested this yet, but am kicking off a build now.

Comment 58

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: icons and text mode for mac

Note that as stated earlier, the "reply to sender" and "forward" icons are only temporary; we'll spin off getting better ones into a second patch.

Comment 59

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: icons only mode for mac

Comment 60

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: text only mode for mac

Comment 61

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

I'm not sure about the shift in icons on the Mac.  I like the simple left and right arrows for reply and forward however the reply list and reply all button really stand out from those.  We need to bring the designs of those together.  (this of course could be done after string freeze)

Comment 62

[Thomas D. (:thomas8)]

This looks great and well done, big improvement, thank you!!!

Nit: Although I agree with Magnus as I'm not really seeing the "balance" of those missing icons for trash and junk in icons+text mode. Screen resolutions where an extra 32px for those 2 icons cause space problems will have space problems to accomodate /any/ icons, at all...

Comment 63

[Andreas Nilsson (:andreasn)]

Attachment: reply and forward icons in 16x16 for mac

Comment 64

[Andreas Nilsson (:andreasn)]

Attachment: reply and forward icons in 16x16 for mac

(with more focus on the details)

Comment 65

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

Comment on attachment 403569 [details]
icons and text mode for mac

assuming updated icons

Comment 66

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

Comment on attachment 403570 [details]
icons only mode for mac

assuming updated icons.  I'd also be curious what the forward button looks like with the new icons.  It's very small in this pic and my tests.

Comment 67

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

Comment on attachment 403571 [details]
text only mode for mac

so easy

Comment 68

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch 1, v9 (checked in)

Unbitrotted, fixed CSS buglet, added new icons from Andreas.

Comment 69

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch 2, v2

Updated version of the Windows patch, with various Windows XP cleanups.  I suspect this is still going to want to some margin/padding jockeying for the modes other than "text" after it lands.

I'm pretty sure mkmelin is doing the sane thing and sleeping now, as he hasn't been around for a number of hours, so I'm going to delegate review to clarkbw, as he knows CSS well and has also spent a bunch of time monkeying around with the CSS in the header code specifically.

Comment 70

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: text mode for windows

Comment 71

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: icons and text for windows

Comment 72

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: icons mode for windows

Comment 73

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

Comment on attachment 403681 [details] [diff] [review]
patch 2, v2

The buttons have taken quite a diet :)  especially in the icon only mode.  I think we'll need to, as you said, wrangle some padding and margin support in later.

Review comments inline:

>+#header-view-toolbox {
>+  border-top-width: 0;
>+  -moz-appearance: none;
>+  -moz-padding-end: 6px;
>+  padding-top: 3px;
>+  padding-right: 6px;

padding-end and padding-right?

>+#header-view-toolbar {
>+  -moz-appearance: none;
>+  min-height: 0;
>+  padding: 0;
>+  border-bottom-width: 0;

these could use quick comments of what you are overriding, other than the moz-appearance; min-height is especially interesting

>+/* toolbar[mode="text"] is necessary so that we end up with more specificity
>+ * than the !import rule in toolkit's toolbar.css.

!important rule @import does something not to be confused with

>+/* Unlike the primary toolbar, we want icons to be horizontally in line with
>+ * the text, rather than vertically.
>+ */

Not exactly the case anymore

>+.hdrReplyListButton {
>+  list-style-image: url("chrome://messenger/skin/icons/mail-toolbar-small.png");
>+  -moz-image-region: rect(0px 96px 16px 80px);
>+}

Seems to be the same icon as forward

Some of the !important rules make me wonder if we should just be more specific in our selector than the parent selector but I didn't take the time to check it out for each case.  r+/ui-r+ with those nits fixed.

Comment 74

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #73)
> these could use quick comments of what you are overriding, other than the
> moz-appearance; min-height is especially interesting

min-height must have been left over from a previous iteration; it wasn't necessary, so I removed it.  

> Some of the !important rules make me wonder if we should just be more specific
> in our selector than the parent selector but I didn't take the time to check it
> out for each case.

Quite likely true, but fixing that is going to have to wait for a later date.

All the rest of the comments have been addressed.

As an added bonus, I've fixed the padding in the modes other than text only.  Patch coming up...

Comment 75

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch 2, v3

Comment 76

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch 2, v4 (checked in)

Comment 77

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

Comment on attachment 403707 [details] [diff] [review]
patch 2, v4 (checked in)

looks good, thanks!

Comment 78

[Dan Mosedale (:dmosedale, :dmose)]

Pushed:

http://build.mozillamessaging.com/mercurial/comm-central/rev/d9eb02634efe
http://build.mozillamessaging.com/mercurial/comm-central/rev/9b4609fb429f

Comment 79

[Peter Lairo]

(In reply to comment #72)
> Created an attachment (id=403686) [details]
> icons mode for windows

The icon, especially at this tiny size, looks almost indistinguishable from the "Reply" (to sender) icon. We are almost inviting problems and negative reactions with this.

Why is the single "smart-reply" button being forced through despite the abundance of reasons that it is a bad idea and that two buttons (Reply-to-sender and Smart-Reply-to-many) would be a better solution?

Comment 80

[rsx11m]

Looking at today's nightly, I definitely like the lean icon-only mode for the header pane buttons, that's a great addition!

(In reply to comment #40)
> > Does this patch bring me some way of choosing which buttons I'd like to see on
> > the header bar?
> 
> Unfortunately not as part of the default UI, as there's not time left in this
> release to make the "Customize Toolbar" UI work.  I suspect it would be
> possible to write an extension to do it, however.

Since we don't have a "Customize" context-menu item yet, the only way to hide individual buttons remains a userChrome.css entry. Adding this by extension is certainly one possibility, however - given that this is now a "real" toolbar - would be good to keep it in mind for the 3.1 release. Unless you have filed already a follow-up on the Customize entry, I can spin this off to a new bug.

Comment 81

[rsx11m]

Continued in bug 519956 to cover the remaining items.