Closed Bug 519956 Opened 15 years ago Closed 14 years ago

Finalize customization of header pane toolbar, needs palette and View toggle

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set
normal

Tracking

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed)

VERIFIED FIXED
Tracking Status
blocking-thunderbird3.1 --- beta2+
thunderbird3.1 --- beta2-fixed

People

(Reporter: rsx11m.pub, Assigned: joachim.herb)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(7 files, 22 obsolete files)

20.06 KB, patch
Details | Diff | Splinter Review
95.80 KB, image/jpeg
Details
636 bytes, patch
dmosedale
: review+
Details | Diff | Splinter Review
11.02 KB, image/png
Details
93.61 KB, image/png
Details
62.71 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review
37.81 KB, patch
joachim.herb
: review+
Details | Diff | Splinter Review
This is a spin-off of the spin-off per bug 465138 comment #80 to complete the work on the new header-pane toolbar. The original description of that bug, reiterated below for reference, was asking for the ability to remove/hide buttons as desired by the user. While the patches for bug 465138 established the necessary infrastructure by making this a real toolbar, focus then shifted somewhat on establishing icon/text views (incorporating bug 505169) before the string-freeze for TB 3.0 hit. While the optional icon-only view is great and easy on the eyes, I'd still like to get rid of certain buttons I don't need, or hide the toolbar completely to use the buttons on the main toolbar instead.

Following tasks for this bug:

 - Finalize customization functionality, i.e., customization palette;
 - add "Customize" to toolbar context menu;
 - add View > Toolbars > Message Buttons to toggle visibility of the toolbar.

This should be a native function of the default Thunderbird UI, in analogy to the other toolbars.


+++ This bug was initially created as a clone of Bug #465138 +++

> 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.

(Quoting bug 465138 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.
Summary: Finalize customization of header pane toolbar → Finalize customization of header pane toolbar, needs palette and View toggle
Thanks for filing.  My suspicion is that having pulled the markup out of XBL back into XUL will have made implementing this significantly easier.  Although this is indeed a Toolbar bug, I'm moving it over to Message Reader UI, as I think it's more likely to stay on the appropriate radar there.
Component: Toolbars and Tabs → Message Reader UI
Flags: wanted-thunderbird3+
QA Contact: toolbars-tabs → message-reader
Flags: blocking-thunderbird3.1+
This sounds like a good idea, but I think we should also look at how to tie this in with the "other actions" dropdown. "Other actions" would be a great place to drop any deleted toolbar items. (Don't use "junk" very often? Remove it from the toolbar and get to it via "other actions" when you need it.)

It would also be a good source of actions to "lift" into toolbar buttons. "Mark as read", "save as", and "print" are all good candidates.
(In reply to comment #2)
> This sounds like a good idea, but I think we should also look at how to tie
> this in with the "other actions" dropdown. "Other actions" would be a great
> place to drop any deleted toolbar items. (Don't use "junk" very often? Remove
> it from the toolbar and get to it via "other actions" when you need it.)
> 
> It would also be a good source of actions to "lift" into toolbar buttons. "Mark
> as read", "save as", and "print" are all good candidates.

This is a great idea! For me, "Show conversation" would be a candidate for a dedicated button, as using GloDa to pull messages from a folder, Sent and Trash together into one list is one of TB3's killer features for me!
(In reply to comment #3)
> For me, "Show conversation" would be a candidate for a
> dedicated button, as using GloDa to pull messages from a folder, Sent and Trash
> together into one list is one of TB3's killer features for me!

I have been dreaming of this idea for weeks now. Were you in my dreams?
I like that idea as well.  That said, I would caution that we have a ton of blockers between now and RC1, so it's going to be harder and harder to get review / testing / landing attention on things that don't block.  So my suggestion would be to split this into small, discrete, low-risk patches.  In particularly, I think just having "Customize Toolbar" working at all has so much value that it's worth its own patch.
I agree, if there is anything that can still be squeezed into 3.0, please
let's do it and work for a full and "nice" solution in the 3.1 time frame.
Having the three items listed in the bug description above implemented would tremendously help to customize the header toolbar already.
Attached patch Partial patchSplinter Review
I'm probably *not* going to work on this beyond this one patch (the customize dialog is all kinds of confusing to me), but this gets things partway there.

What's done:
* You can right-click to customize the message header toolbar
* The smart reply button is now one item (which is really an hbox with three buttons inside, only one of which is visible at a time)
* There's a special reply to sender button that will automatically hide if the "reply" mode of the smart reply button is active

What's not done:
* Hiding the message header toolbar (how would you get it back?)
* Customizing the header causes the main menu items and the "Customize..." option to be disabled for some reason
* Localization for the "Customize..." option doesn't work for some reason
* Lots of problems with the actual Customize dialog (changing display type doesn't work right, icons are screwy, etc).

Someone else is welcome to finish this off (please!), since I'd really rather not try to figure out the Customize dialog. :)
Oh, and I updated the Gnomestripe theme a bit to account for the new smart reply button, but people are welcome to clean that up and/or move the changes to the other themes (I changed a few child selectors to descendants to account for the new organization of the smart reply button).
Jim, thanks for getting this started, I hope someone can take it from here.

(In reply to comment #7)
> * Hiding the message header toolbar (how would you get it back?)

I had the View > Toolbars submenu in mind, where you can already toggle the status bar (that's in utilityOverlay.xul for the main window).
I really like this idea of swapping between toolbar buttons and other actions, lets go with it!  Maybe I can try this patch out over the end of the weekend.

(In reply to comment #9)
> I had the View > Toolbars submenu in mind, where you can already toggle the
> status bar (that's in utilityOverlay.xul for the main window).

Sounds like a good spot to me.  Maybe call it "[x] Message Toolbar"?
For the main toolbar it says "Mail Toolbar" (maybe make that "Main Toolbar" in the process?), so "Message Toolbar" would fit as title for the menu entry. "Status Bar" is not a toolbar from the user's perspective, thus "Bar" vs. "Toolbar" would retain that distinction.
This is the only change to the first version of the patch:
+  var toolbarset = document.getElementById('customToolbars');
+  toolbox.toolbarset = toolbarset;
(in /mail/base/content/msgHdrViewOverlay.js)
In bug 509044 a new way to display toolbar items was introduced: You can now also place the label besides the icon, not only beneath. I would suggest, that this setting is also used for the menu header toolbar: 

So 4 different ways to display the buttons: With icon and text beneath or besides the icon, only icon or only text. I think it would also be worthwhile to think about using both icon sizes (small to be the default). 

At the moment the settings of the customize dialog in the bottom line are mostly ignored (icon size, text/icon).

This leads me to my actual suggestions:
Why not clone the toolbar buttons of the main toolbar into the customization dialog so they can used in the header toolbar? 

The forward, archive, junk, delete buttons are already there. Only the new reply button has to be added to the palette of the main toolbar. Then when cloning the palette to the header toolbar, all buttons would be available (plus all the others like tag, print etc.).

Beside this suggestion: I do not see the correct icons inside the customization dialog: The image assigned to the icon seems to be different inside the customization dialog, so the -moz-image-region does not work and the whole list-style-image is displayed.
> +  var toolbarset = document.getElementById('customToolbars');
> +  toolbox.toolbarset = toolbarset;
Nit: toolbox.toolbarset = document.getElementById("customToolbars");
Also this file inconsistently uses a mixture of single and double quotes. Consider standardizing on double quotes for the lines you add or touch.

> Beside this suggestion: I do not see the correct icons inside the customization
> dialog: The image assigned to the icon seems to be different inside the
> customization dialog, so the -moz-image-region does not work and the whole
> list-style-image is displayed.

In mail/base/jar.mn you need to add the following line:

% style chrome://global/content/customizeToolbar.xul chrome://messenger/skin/messageHeader.css
> In mail/base/jar.mn you need to add the following line:
>
> % style chrome://global/content/customizeToolbar.xul
> chrome://messenger/skin/messageHeader.css
Where do I have to add this line?
Below messenger.jar:, comm.jar: or toolkit.jar:?
>> % style chrome://global/content/customizeToolbar.xul
>> chrome://messenger/skin/messageHeader.css
> Where do I have to add this line?
> Below messenger.jar:, comm.jar: or toolkit.jar:?

Together with all the other similar lines that say:
% style chrome://global/content/customizeToolbar.xul chrome://something/foobar.css
If change jar.mn by adding the following line (mark with + in line 13):

% style chrome://global/content/customizeToolbar.xul chrome://messenger/skin/primaryToolbar.css
+% style chrome://global/content/customizeToolbar.xul chrome://messenger/skin/messageHeader.css
    content/messenger/mailWindow.js                 (content/mailWindow.js)

does not help: The icons in the customization dialog of the buttons in the header pane are not shown correctly. Even worse: The icons in the customization dialog of the main toolbar are also not shown correctly.
Yes well as we discovered in SeaMonkey - which has more windows with customizable toolbars than Thunderbird - the more overlay styles you apply to the customizeToolbar window the more likely you'll have css images/image-regions clashing. What you need to do is to audit all the style files that apply to the Customize window and remove any default list style images for e.g. the toolbarbutton-1 class. And assign explicit list-style-images to each button. (and good luck on that).
Conveniently enough, I filed bug 386007 for doing that, though inconveniently enough I never actually wrote the patch.
This might be the wrong place to ask, nevertheless:
I try to fix this. The problem is I am using the separate shared libraries build options (https://developer.mozilla.org/en/Configuring_Build_Options#Statically_Linking_Components). After I changed the jar.mn file and rebuild (in the directory mail) the customization dialog of the main toolbar is broken. If I remove the line with the style in messageHeader.css and rebuild again, the dialog stays broken. I have to rebuild in the root directory of the sources to get the dialog to work again.
For further discussion this issue I started a thread at mozillazine.org:
http://forums.mozillazine.org/viewtopic.php?f=29&t=1529715
The problem described in comment #20 might be a problem of the build system: for details see bug 521674
If the order of the lines in the file messenger.manifest in mozilla/dist/bin/chrome is changed so that the line
style chrome://global/content/customizeToolbar.xul chrome://messenger/skin/primaryToolbar.css
is the last one (or at least after all other lines referencing customizeToolbar.xul) the main toolbar customization dialog works again. 

Why is the order of the lines mixed up by adding the reference to messageHeader.css in the jar.mn file?

The (or some of the) problems of the customization dialog of the header pane  seems to be related to the icon size:
The header pane button icons are defined using mail-toolbar-small.png. But the dialog tries to use the normal icons. I guess by defining normal and small icons for the header pane buttons these problems should go away. I will try this tomorrow...
This patch adds normal size icons. The setting of the customization dialog about icon size is obeyed. 

For this patch to work/not to break the main toolbar customization dialog you have to reorder the lines in messenger.manifest as described in comment #22

Now the archive and junk buttons have always icons, because I do not know how to combine conditions in the css file.
(In reply to comment #20)
> This might be the wrong place to ask, nevertheless:

#maildev on irc or mozilla.dev.apps.thunderbird are better places.

If you want your patches to get in the tree you need to ask reviews as described at https://developer.mozilla.org/en/comm-central#Requirements
(In reply to comment #24)
> If you want your patches to get in the tree you need to ask reviews as
> described at https://developer.mozilla.org/en/comm-central#Requirements
This patch is work in progress. You can now use it without fiddle with the messenger.manifest if you also apply this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=405886 for bug 386007

TODO for this patch:
Disable "Add New Toolbar" in customization dialog.
??? Disable "Use Small Icons" in dialog and always use small icons ???
Remove "Show Icons/Text" in context menu of header buttons as this is handled by the customization dialog.
Use "Icons beside Text" as default for button style. Remove special handling from messageHeader.css and use "normal" toolbar settings.

Anybody willing to test?
Just tried comm-central with both patches on OS X.  Seems to basically work.  I noticed that dragging things from the toolbar to the dialog works, but subsequently trying to drag them from the dialog back into the toolbar doesn't.  Saw a bunch of errors in the error console:

Error: aElt is null
Source File: chrome://global/content/customizeToolbar.js
Line: 657

I think the l10n string you want is customizeToolbar.label from messenger.dtd.

I've been told that this may cause concern from some localizers, even though it's not adding a new string, since it is reusing an existing string in a different context, and the localization opt-in thread has already started.
I'm hoping that we're unlikely to have any issues, given that the English version of the string in question is "Customize..." from a context menu.

Adding sipaq and thunderbird@localization.bugs for input...
(In reply to comment #26)
> I think the l10n string you want is customizeToolbar.label from messenger.dtd.
> 
> I've been told that this may cause concern from some localizers, even though
> it's not adding a new string, since it is reusing an existing string in a
> different context, and the localization opt-in thread has already started.
> I'm hoping that we're unlikely to have any issues, given that the English
> version of the string in question is "Customize..." from a context menu.

This should most likely be okay, but I don't know all the edge cases. Let's hear what localizers have to say about this. But if this gets in, we need a followup bug right away that will block TB 3.1 creating a new string, because even if this is does not turn out to be a problem now, it will sooner or later will be.
Of the TODO of comment #25 this patch should include:
> Use "Icons beside Text" as default for button style. Remove special handling
> from messageHeader.css and use "normal" toolbar settings.
> Remove "Show Icons/Text" in context menu of header buttons as this is handled
> by the customization dialog.

Still missing is:
> Disable "Add New Toolbar" in customization dialog.
> ??? Disable "Use Small Icons" in dialog and always use small icons ???

This should work using Windows and also probably Linux. For OS X I probably need help. Especially the larger icons are missing.
Attachment #404509 - Attachment is obsolete: true
Attachment #405808 - Attachment is obsolete: true
Just a nit from some stuff in my original patch that stuck around: the hbox inside hdrSmartReplyButton isn't actually necessary and should probably be removed for simplicity's sake.
I just quickly checked the WINxp/Windows NT 5.1; build .. no problems seen there.
GREAT!!!
Herb, gozer, thanks for pushing on this!  Trying this out on the Mac, I see the following issues in the Customize "sheet" (the OS X replacement for some types of dialogs):

* The "Show" dropdown menu defaults to blank, rather than whatever mode the toolbar starts off in.  
* Once the "reply" or "reply all" buttons are dragged from the toolbar to the palette, it doesn't seem to be possible to drag them back ever again.  Nothing relevant in the error console.
* Since we only have one size of icons, I think it's probably best to just hide the "Use small icons" checkbox and label entirely.

Possibly Mac only:

* The Customize sheet itself appears to be centered against the toolbar, which makes half of it stick out to the right of the window, which looks strange.  

Interestingly, "Icons beside text" works just fine on Mac.
And, other than those things, this seems to be working great!
(In reply to comment #30)
> 
> I just quickly checked the Linux build: The only issue I have seen is that the
> trash button does not obey to the Icons beside Text setting.

Linux here, too. "Icons beside Text" works when the "Use Small Icons" is left blank-- but, when you click the checkbox, the text ends up underneath.

It looks as if the width of the "Delete" button is calculated too narrow when "small icons" is selected, and the insufficient width forces the text to be painted on a second line.
New comment for a different issue:

I was disappointed not to see a "Toggle Header View" item within the "other actions" menu of the header. Without this item, you must drag your mouse all the way up and over to the View-->Headers menu item. (That's completely across the screen to the left.)

It's AWK, IMO.
A few issues I observe:

If you remove all buttons and close the customization dialog, there is no way to open the dialog again (would it be a good idea to add a minwidth?).

As default I used the icons beside text as default. This is handle by the attribute labelalign="end" in the toolbox xul tag. But there seems to be no defaultlabelalign tag like defaulticonsize.

The button reply to sender can only be removed from the toolbar if it is visible, i.e. if currently selected mail has more the one recipient. But if you add this button to the toolbar when a message is selected which should not show this button it stays there at first. If another message is selected and you go back to the first message the button disappears. What should be the correct behavior? Display the button, when the customization dialog is opened, and hide it after closing the dialog if the message does not need it?
> If you remove all buttons and close the customization dialog, there is no way
> to open the dialog again (would it be a good idea to add a minwidth?).

In SeaMonkey we use a min-height.

> As default I used the icons beside text as default. This is handle by the
> attribute labelalign="end" in the toolbox xul tag. But there seems to be no
> defaultlabelalign tag like defaulticonsize.

This is how I did it in SeaMonkey:
<http://mxr.mozilla.org/comm-central/search?string=defaultlabelalign&find=suite>

For the third issue. In SeaMonkey we let all buttons to be visible on entering customization mode and then revert back to the normal behaviour (for whatever is normal for that button) on exit.
This should solve all items of the TODO list in comment #25

I will try to get new try server builds and publish the links here.

Another patch for the customizeToolbar.xul file is needed for this to work and the patch of bug 386007. So this bug is depending on it.
Attachment #406106 - Attachment is obsolete: true
This patch is needed for patch 406812 to work. I am not sure but I think it is for the mozilla-1.9.1 repository, because this file is mozilla/toolkit/content
Sorry, you'll need to find another way: not only has 1.9.1 already shipped, with Firefox 3.5, but 1.9.1.4 which Thunderbird 3.0 is going to ship with is already finalized. So even if you could land that in 1.9.1 (which you shouldn't be able to, since adding an id to something that an extension could be overlaying, expecting that there wasn't anything with that id, is the sort of thing that shouldn't be allowed to land on a stable branch), you couldn't land it in the 1.9.1 we're going to ship with.
The id for smallicons checkbox is already in mozilla-1.9.1. The other two (addnewtoolbar, restoredefault) are not need with this patch.
Attachment #406813 - Attachment is obsolete: true
Attachment #406815 - Attachment is obsolete: true
Attached patch Now for all oses... (obsolete) — Splinter Review
Attachment #406827 - Attachment is obsolete: true
Attachment #406828 - Attachment is obsolete: true
+/* XXX is handled by the default customization dialog
+toolbar[mode="full"] .msgHeaderView-button,
+toolbar[mode="full"] .msgHeaderView-button > .toolbarbutton-menubutton-button
 {
   -moz-box-orient: horizontal;
 }
-
+*/
....
+/*  padding-top: 5px !important;
+  padding-bottom: 3px !important; */

Don't comment out code. Just remove it.
(In reply to comment #45)
> +/* XXX is handled by the default customization dialog
> +toolbar[mode="full"] .msgHeaderView-button,
> +toolbar[mode="full"] .msgHeaderView-button > .toolbarbutton-menubutton-button
>  {
>    -moz-box-orient: horizontal;
>  }
> -
This will be removed in the next version of the patch.

But about removing the following lines, I am not sure: At the moment the icons for all five buttons are displayed after applying the patch. Without the patch the archive and junk buttons don't have icons, when using the icons and text mode.
> +*/
> ....
> +/*  padding-top: 5px !important;
> +  padding-bottom: 3px !important; */
> 
I have asked for another set of try server builds. If these work I will ask for review of the patch (without the comments).
(In reply to comment #46)
> But about removing the following lines, I am not sure: At the moment the icons
> for all five buttons are displayed after applying the patch. Without the patch
> the archive and junk buttons don't have icons, when using the icons and text
> mode.

The design of hiding the archive and junk buttons, even in text+icons mode is intentional, and should be preserved.  That said, I'm not sure what the following lines have to do with that:

> > +*/
> > ....
> > +/*  padding-top: 5px !important;
> > +  padding-bottom: 3px !important; */
To get the "Restore Default Set" button to work I have to overlay the button, which does not have an id. I was not able to use the function getAnonymousElementByAttribute so I worked around by using getElementsByTagName. Any other ideas how to do this? (Begin of function overlayOnLoad in mailCore.js)

As I am using the qute theme, the buttons might need cleanup using Linux/OS X.
Attachment #407062 - Attachment is obsolete: true
neat!

- text below icons feels really ugly to me, introducing huge vertical gaps.

- i got a crash on the mac, but don't know if it's due to this patch.  no crash stats sorry.
> To get the "Restore Default Set" button to work I have to overlay the button,
> which does not have an id. I was not able to use the function
> getAnonymousElementByAttribute so I worked around by using
> getElementsByTagName. Any other ideas how to do this? (Begin of function
> overlayOnLoad in mailCore.js)

Any of the following work for me:

document.getElementById("main-box").getElementsByAttribute("oncommand", "restoreDefaultSet();")[0];
(this one depends on an exact string match in the oncommand attribute)

document.getElementById("main-box").querySelector("box>button+button");
(this one assumes that the second button in the main-box is the correct one)

document.getElementById("main-box").querySelector("[oncommand*='restore']");
(this one is probably the most robust I think)

>+	let dialog = document.getElementById("main-box");
..^^^^^^

By the way your patch is full of tabs. Please get rid of all of them. Use spaces only. Thanks.
(In reply to comment #51)
> document.getElementById("main-box").querySelector("[oncommand*='restore']");
> (this one is probably the most robust I think)
Thank you. Now I use this version.

> 
> >+	let dialog = document.getElementById("main-box");
> ..^^^^^^
> 
> By the way your patch is full of tabs. Please get rid of all of them. Use
> spaces only. Thanks.
Hopefully I got rid of them now. See the next version of the patch.

What I have tried and did not accomplish: To reduce the width of the junk button in "icons besides text" mode (where it doesn't have a icon). There seems to be a min-width for the label. Any idea?
(In reply to comment #52)
> What I have tried and did not accomplish: To reduce the width of the junk
> button in "icons besides text" mode (where it doesn't have a icon). There seems
> to be a min-width for the label. Any idea?

You can use the rule you commented out in messageHeader.css:

toolbar[mode="full"] > .hdrJunkButton {
  min-width: 1em;
}

But for for me this will only have a gain of 2px (55px to 53px). Is it this worth?

If you like to remove the "Icons and Text" menuitem in customize toolbar, you can add to your hiding rules this line: window[isHeaderPane] #modelist menuitem:first-child
Not so beautiful, but without IDs not better doable in CSS.
This is looking really good.  I just spent some time looking at the patch, and it touches a fair bit of CSS.  This makes me concerned that even if we were to get this landed this week, it feels like there's a fair amount of risk that we wouldn't get all of the fallout dealt with noticed, filed, and fixed by code freeze, which is at the end of next week (Friday, October 30th).  

My inclination, therefore, is to instead target this to land on the trunk after we branch (after this Thursday, October 22nd).  The intent would be that we'd then get this for Thunderbird 3.next, which is currently expected for ~4 months after Thunderbird (I'm going to start drafting roadmap stuff tomorrow; I probably won't have anything substantial until next week-ish).

Thoughts?
@55 Dan's inclination
I fully understand your 'restrictive' position .. to be careful for landing things at such a late point in time. And I'm not experienced enough to fully understand the risk etc ..

On the other side we all know about the "acceptance" of the "new" message header. Herb did a grandiose job here fighting against time also running into a lot of problems *not* invented by him. My impression was/is there are more experienced people around which could/should be able to help here. Not to finally give Herb the glory of deliver a great solution -- but to solve a showstopper. And it was very often posted by many people at different places as such -- *showstopper for TB3.0*!!

TB3.next is too far away. It has to be delivered with TB3.0. Period!

Let me call for: "help Herb / the TB3.0 audience to get a smooth product -- also with that new message header.

Thanks
Günter
I completely agree that the value of the feature is high, Herb has done great work, and in an ideal world, we'd all like to see this ship in Thunderbird 3.0.  

The decision at this point is not about any of those things; it's purely a risk management question.  I'm particularly interested in Herb's thoughts here...
Not to mention Jim's good work in getting us rolling, which I completely spaced; sorry Jim!  Thoughts from Jim or Philor would be good as well...
(In reply to comment #54)
> Created an attachment (id=407287) [details]
> Hide "icon and text" mode, cleanup overlaying restore default set function,
> cleanup button width

Even though this isn't up for review just yet, I thought I'd comment on these:

+<!ENTITY % customizeToolbarDTD SYSTEM "chrome://global/locale/customizeToolbar.dtd">
+%customizeToolbarDTD;

Probably isn't necessary anymore. That's from when I was trying to localize the "Customize..." text.

+            accesskey="c"/>

Should be "&customizeToolbar.accesskey;"


As for getting this into 3.0, I think this is fairly key, and I'd be willing to help out however I can, since I narrowly avoided doing this patch in the first place. :)
I think it would be an awesome extension for 3.0, where it could go through a half-dozen iterations before 3.0 ships, unlike shipping with 3.0, where it might or might not get one chance for changes after it lands.

Offhand, I don't see anything that wouldn't work as an extension, but if any roadblocks to that show up in the next two or three days, we ought to do what we can to clear them.

I also think it would be lovely if anyone can figure out how we can get people interested in contributing *before* it's too late, since I've seen this exact same thing with every single release I've been involved with, since Firefox 1.0.
> I also think it would be lovely if anyone can figure out how we can get people
> interested in contributing *before* it's too late, since I've seen this exact
> same thing with every single release I've been involved with, since Firefox
> 1.0.

Unfortunately I think it's the OMG it's too late! realization that panics people into contributing - at least for Thunderbird. We've never had that in SeaMonkey ;).
@61 Philip
Today - Oct.20th - is around 10 (ten) month ago the work started to get that XX header layout changed. And for a long time there was no / very minimum help around. We have heard like 'it's time' 'we will come back to this issue' 'there are other points we have to do now' etc etc 

I don't want to start a finger pointing, just to a 'call for help' to get the necessary things done NOW. Not to have it slipped for another xx months.
Header pane is one of the most obvious changes in TB3, which will also make it most threatening for existing users if they feel it's not under their control. This bug goes a long way in making the newness acceptable to users by making it fully customizable. Please let it land. There's enough other showstoppers around, let's get this one out of the way if there's any chance we can.
Again, this is a risk-management decision, not a decision based on the various other factors mentioned in recent comments here.  We've already said we won't block on this bug, and that also means we won't accept undue risk from a patch for it, particularly since, as philor points out, this should be shippable as an add-on.

Jamming risky features in at the last second is a recipe for shipping low-quality software.  One of the consequences of this is that any given version of software _always_ ships without all the features the that developers (including me, I might add!) would like.  Additionally, whether or not this makes it into Tb 3, we'll very much get this for 3.next, and we're planning on 3.next being a short cycle (~4 months is the current thinking).

I should have been more clear about the kind of feedback I'm looking for: I'm interested in technical assessment of exactly what the areas of breakage risk are if we land this now, and what the magnitude of that risk is.
Because the first landing is not likely to be sufficient ... scoping time, not risk ... assume initial patch lands today, allow for 4 days of final baking prior to 2009-10-30 23:59 PST, assume 3 day patch+review+land cycles (perhaps optimistic as it implies roughly 1 day turnaround on reviews), and good weather => time for ~2 patch revisions.

the tryserver builds can help get more eyes on it through quicker testing and perhaps speed things up. Strictly visually and using the buttons, the windows tryserver build looks lovely - no issues
...and then the question will be anyway if a single RC1 will be sufficient, or if a RC2 may give you another week or so. That aside, what is Thunderbird's policy in terms of follow-up fixes in later 3.0.x releases?

Since comment #61 was mentioning SeaMonkey, they've let a couple of things slip for the SM 2.0 release next week which will be covered in SM 2.0.1, if I'm not mistaken. Thus, if there are no major issues in herb's patch, it should be ok to go ahead with landing it and follow up with any minor CSS or other issues (if they exist) in the next minor release.
And don't forget while you are planning how much time other people will spend on this that you are also planning out reviewer time: since dmose is the most likely, how much of his time do you want to budget for this, for the first and then for subsequent reviews? Keep in mind that you are then taking him away from working on his own blockers, and taking him away from reviewing blockers, and taking him away from helping to drive the release.

Scope of the risk is probably broken or wrong looking toolbar buttons (in any window and any toolbar), toolbar customization completely broken, in the 3pane or in all windows, or that old favorite from an error as customization closes where opening and closing customization disables all menus until you force the app closed. And all of those in some circumstance that you are not currently imagining, much less running, so no, you can't promise to test it really hard to make the risk go away. 

Traditionally, Thunderbird's policy for .0.1 has been exactly that of Firefox: promise people that they can land things for .0.1, that because .0 was rushed and they didn't have time to land that .0.1 will have a wider scope than usual for stable branches, and then make reality maybe halfway between what was promised and a real stable branch. So yeah, if we took it and if as a result we broke the app, we would take a fix for that in .0.1, pretending to ourselves that _next time_, we really wouldn't take features after the last beta, instead of just saying that we wouldn't.
(In reply to comment #68)
> And don't forget while you are planning how much time other people will spend
> on this that you are also planning out reviewer time

I'm not daring to plan anybody else's time, and btw Wayne's estimate included respective review time. The motivation for my comment was to point to the possibility that RC1 is not the end of the day for landing 3.0 patches.

> Scope of the risk is probably broken or wrong looking toolbar buttons (in any
> window and any toolbar), toolbar customization completely broken, in the 3pane
> or in all windows, or that old favorite from an error as customization closes
> where opening and closing customization disables all menus until you force the
> app closed. And all of those in some circumstance that you are not currently
> imagining, much less running, so no, you can't promise to test it really hard
> to make the risk go away. 

Ok, so how do you expect herb or Jim to make that assessment?

> broke the app, we would take a fix for that in .0.1, pretending to ourselves
> that _next time_, we really wouldn't take features after the last beta, 

Just for the record, this is not a feature. The header-pane feature itself was introduced by bug 449691, and it was clear in September 2008 already that this would need some customization. My follow-up bug 465138 was initially filed for that purpose, but the customization palette wasn't implemented there either. Hence I filed the bug here, so maybe that's close enough to get finalized over the next 10 days without too much more discussion if it should be finalized.

Back to the topic, any advise how herb or someone else (ideally dmose himself as he developed the underlying code) can estimate the risk of breaking anything and how much more work is potentially needed?
As I have no access to OS X this is as far as I get without help. Using my computers (Windows Vista / Linux (Gentoo)) the header pane toolbar looks tidy now.
(Why is doing a build using Linux so much faster than using Windows???)

What do you think is missing before starting the review process (independent of getting the patch included into the trunk now or later).
Attachment #407287 - Attachment is obsolete: true
(In reply to comment #52)
> What I have tried and did not accomplish: To reduce the width of the junk
> button in "icons besides text" mode (where it doesn't have a icon). There seems
> to be a min-width for the label. Any idea?

This should be fixed by bug 519688 when it gets merged.
I think this is extremely important for the tradition of keeping customization possible. So I'm willing to beat these changes to death.
Unfortunately the tryserver builds don't seem to be working for me. (I'm assuming right-click in the headerpane should bring up the customization box in winxp)
Am I missing something.(I didn't try with a fresh profile)
Yes, you just have to right-click over one of the buttons and "Customize" should appear. When dragging a button onto the toolbar you have to be very close to the buttons already there, otherwise you see the "not allowed" cursor and can't drop it. Upon initial start, this opened for me with icons above text, only after selecting icons beside text or icons only I got the desired appearance.

Other than that, it works great (testing the WinXP try build of comment #65). Buttons can be selectively removed or rearranged as desired. I've also tested the scenario in the Archives folder. You can drag the "Archive" button onto the toolbar, and as soon as you close the customization palette, it will disappear as it is supposed to in the Archives folder.
Some more observations from the try-server build:

 - with an existing profile, it started up with large icons. Only after I've
   clicked "Restore Default" it reverted to small icons (main toolbar is set
   to use large buttons, in case that's relevant).

 - The constant space works fine, whereas the flexible space is reduced to 0px
   after closing the customization palette. Separator produces a space but not
   a '|' line as the icon would suggest.

 - Testing a worst-case scenario, I've removed all buttons from the header pane
   and put them onto the palette. It looks empty as desired, the question being
   if I can still get to the Customize menu, and indeed no problem to put all
   buttons back (it will appear on right-click just over the message time).

So, this needs some more tweaking, but I don't see it breaking anything. Customization of other toolbars (tested main toolbar) is not affected, and the header-pane buttons are fully functional from what I can tell.
(In reply to comment #74)
>  - The constant space works fine, whereas the flexible space is reduced to 0px
>    after closing the customization palette. Separator produces a space but not
>    a '|' line as the icon would suggest.

This can be solved with

#header-view-toolbar toolbarspacer,
#header-view-toolbar toolbarseparator {
  height: 25px;
}

#header-view-toolbar toolbarspring {
  height: 25px;
  min-width: 5px;
}

The height in px isn't optimal as it does not adapt for different font heights and should be optimized.
The min-width for the toolbarspring is used so you can grab it from the toolbar. As the toolbarspring will be always 5px and not be flexible it can be used as a small spacer. But as it has not his main function it should perhaps be removed from the palette with

window[isHeaderPane] #wrapper-spring {
  display: none;
}
Attached image test with extension
Not sure how this test relates to the main task getting it ready for TB3.0.

Just used the last mentioned WIN build 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5pre) Gecko/20091020 Shredder/3.0pre 
and an extension build (ReminderFox) with using the "header-view-toolbar-palette" to have extension related buttons added to the message header.

The extension XUL now has added:
<!-- Thunderbird message header  NEW to TB3.0pre  // gWTEST  -->
<toolbarbutton id="reminderFox_openButton" label="&rf.main.icons.openReminderfox.label;"
	tooltiptext="&rf.main.icons.openReminderfox.tooltip.label;" oncommand="reminderFox_openAddRemindersDialog(true)"
	class="toolbarbutton-1 chromeclass-toolbar-additional msgHeaderView-button" />

<toolbarpalette id="header-view-toolbar-palette">
	<toolbarbutton id="reminderFox_openButton"/>
	<toolbarbutton id="reminderFox_addReminderButton"/>
	<toolbarbutton id="reminderFox_showCalendar"/>
	<toolbarbutton id="reminderFox_quickAlarmButton"/>
</toolbarpalette>

Test result (for the layout see the snapshot):
- all normal actions to/with the message header are intact
- customize to add the extension buttons is possible 
- the button layout on the message header doesn't have the surrounding box
- the button layout on the customize palette have the the surrounding box

- buttons added to the 'MailToolbarPalette' now have the surrounding box

- the buttons on the message header are not active! Saying I can click them, but no action. The "requested" reminderFox_openAddRemindersDialog(true) is not called. Whereas a call from the main toolbar works.

Any idea for the extension, more important: any effect for the "main" project, getting this ready for TB3.0??

Günter
Just for record, SeaMonkey didn't let any _features_ slip to 2.0.1, we just agreed that some fixes don't need to block the final itself and can make 2.0.1 instead. We had feature freeze at the last beta, and did let one feature slip that one and go in between beta and the first RC, with multiple weeks to bake in between, and incidentally that one features was mailcompose toolbar customization.

The header changes in Thunderbird 3.0 are one of the more visible changes, and I'm sure there will be both cheers and critics about it in any case. This bug would surely improve the experience, but recent comments show that it still needs testing and updates until it's really fit for a stable release, and I fully agree with dmose that one needs to set clear, sometimes unfortunate, marks and even cut back on what you'd like to have in the release so that you're able to ship it with satisfying quality.
The business of managing releases is much less fun than the one of developing new things, as not just "hey, it works!" is important but also how reliable it works and how polished it is.
Hi KaiRo, sorry for not being specific in comment #67, "a couple of things" is obviously ambiguous with respect to the kind of fixes. Anyway, the try-server builds were quite helpful to get an idea what works and what still needs some work, and there is already a solution posted for some of the issues. I agree that "hey, it works!" is not a good estimate for reliability, in the end it's certainly the drivers' decision whether or not this can go into 3.0, with or without follow-ups being necessary for 3.0.1.

Re comment #76 on extensions. If this goes in, then the add-on author would probably have to resolve any conflicts with the release. However, if the shared icons between the main toolbar and the header-pane toolbar causes the issue to start with, my guess is that this would have to be resolved here.
herb, I'm not sure exactly when we'll know whether or not we'll be able to take this for Tb3.  The conservative thing to do would be to assume that we're not.

That said, I think it's worth pushing to get landed on the trunk ASAP after the branch is cut, and then drivers will make the call about whether or not to take it on the branch.

I'll request ui-review from clarkbw on the current patch, and if it's granted, I'm delegating core review to bwinton (though perhaps philor might be interested in reviewing as well), since I'm going to be travelling.
Attachment #407433 - Flags: ui-review?(clarkbw)
If this functionality should be added as add-on and not be part of the release, then nevertheless patches to the trunk are necessary, especially to some of the .js files. Where should an corresponding patch be publish? In this bug or should a new bug be created? When will the decision be announced?
I have created an add-on which has the functionality discussed in this bug:
http://customizeheadertoolbar.mozdev.org/index.html

The source is hosted at mozdev.

You can download the current version here:
http://customizeheadertoolbar.mozdev.org/download.php/CustomizeHeaderToolbar-0.1.0.xpi

The add-on should work with the current nightly builds. But a patch for Thunderbird is needed to avoid an error (message) if you remove the junk button.
For the CustomizeHeaderToolbar add-on to work it is necessary to apply this patch. Otherwise there will be errors (see error console log) when the junk button is removed.
Attachment #408208 - Flags: review?
Attachment #407433 - Attachment is obsolete: true
Attachment #407433 - Flags: ui-review?(clarkbw)
Attachment #408209 - Flags: ui-review?(clarkbw)
Herb
Could you open a discussion thread in mozillazine, or mozdev.
I can't seem to get a populated customize panel to appear.
Winxp with the latest nightly.

Error: gToolbox.palette is null
Source file: chrome://global/content/customizeToolbar.js
Line: 330
I started a thread at mozillazine.org to discuss the CustomizeHeaderToolbar add-on: http://forums.mozillazine.org/viewtopic.php?f=48&t=1553385&start=0

Just two remarks:
- Whenever you encounter problems with this add-on (or the patch) try the "Restore Default Set" button in the customization dialog. This should also be selected before uninstalling the add-on. Then you should get back all buttons in the header pane.

- The patch https://bugzilla.mozilla.org/attachment.cgi?id=408208 is needed if you use the add-on and remove the junk button. Otherwise the "smart reply button" is not updated anymore.
Comment on attachment 408209 [details] [diff] [review]
The latest state of the patch for Thunderbird (without add-on)

Finally got to try this out (only on the mac so far).  Here are some comments so far.

We need to add some kind of inset border + dark background color to indicate the toolbox area available when customizing.  I had a really hard time adding the ( reply ) button to the far left because I couldn't see where it was possible to land.

Along with that idea we should probably extend the toolbox area beyond the buttons with some padding or something (at least start/end padding).

There seems to be some odd spacing problems between the archive and junk buttons in text beside icons mode.

The reply button doesn't ever seem to get an icon and so when in icon mode only it just appears as a small dot beside the smart button.

We should probably also put the Customize menu in the ( other actions | v) drop down so that it's always available even if you removed all the buttons from the header.  Otherwise (as was noted before) when all the buttons are gone it's hard to figure out where to click to get the context menu again.

Looks good so far, nice work.
Thanks to the spread of the add-on, new bugs pop up ;-)

If switching the layout (View->Layout->Wide/Vertical View) the header pane toolbar is no longer customizable. When opening the customization dialog, the error message reported in comment #84 appears. 

The reason for this is, that when switching the layout the header-view-toolbox is moved around. As the palette is not part of the DOM it gets lost (I observed the same problem in the CompactHeader add-on). If you start Thunderbird not in the classical view the layout is switched during start up. So the palette gets lost already at this point of time. I do not know how to save it in an addon.

With the following patch, the problems disappears, but I think this is not a very elegant solution (hard coding ids of one toolbox/toolbar):

diff -r 83e8ff7342e8 mail/base/content/msgMail3PaneWindow.js
--- a/mail/base/content/msgMail3PaneWindow.js	Wed Oct 21 16:26:47 2009 +0100
+++ b/mail/base/content/msgMail3PaneWindow.js	Mon Oct 26 01:29:46 2009 +0100
@@ -200,6 +200,12 @@
   var messagePane = GetMessagePane();
   if (messagePane.parentNode.id != desiredId) {
     ClearAttachmentList();
+    var hdrToolbox = document.getElementById("header-view-toolbox");
+    var hdrToolbar = document.getElementById("header-view-toolbar");
+    var firstPermanentChild = hdrToolbar.firstPermanentChild;
+    var lastPermanentChild = hdrToolbar.lastPermanentChild;
+    var cloneToolboxPalette = hdrToolbox.palette;
+    var cloneToolbarset = hdrToolbox.toolbarset;
     var messagePaneSplitter = GetThreadAndMessagePaneSplitter();
     var desiredParent = document.getElementById(desiredId);
     // See Bug 381992. The ctor for the browser element will fire again when we
@@ -210,6 +216,14 @@
     desiredParent.appendChild(messagePaneSplitter);
     desiredParent.appendChild(messagePane);
     messagePaneSplitter.orient = desiredParent.orient;
+    hdrToolbox = document.getElementById("header-view-toolbox");
+    hdrToolbar = document.getElementById("header-view-toolbar");
+    hdrToolbox.palette = cloneToolboxPalette;
+    hdrToolbox.toolbarset = cloneToolbarset;
+    hdrToolbar.firstPermanentChild = firstPermanentChild;
+    hdrToolbar.lastPermanentChild = lastPermanentChild;
+    
     if (aMsgWindowInitialized)
     {
       messenger.setWindow(null, null);

(The choice of the four data to save (first/lastPermanentChild (are needed, so all buttons can be removed), toolbarset, palette) is based on the experience from the development of CompactHeader. If one or more are left out you get errors.)

Would a patch like this be acceptable/any idea how to solve this problem in the add-on (which xul file to overlay and how to save the data)?
Since it's seeming unlikely that this is going to get into TB3, would it make sense to *just* handle the "View toggle" part of the bug now (as a hidden pref) and save the rest for later? That should be fairly easy to test, and would probably resolve the biggest issue this attempts to address ("I don't like the message header buttons").
If we're talking about the buttons being shown in the message header, then I agree with Jim-- the "Toggle View" button is the BIG issue. Dragging all the way up the screen to accomplish the "View-->Headers" toggle, with multiple clicks the double drag-within-the-menu motion, is totally nasty.

Making cute icons in small versus normal sizes is vastly less important. Heck, if it's turned into a major problem, my "vote" is just leave the durn icons out of the buttons...

thanks, I hope this wasn't a horrible spamming of the bug.
(In reply to comment #84 and comment #87)
> Herb
> Could you open a discussion thread in mozillazine, or mozdev.
> I can't seem to get a populated customize panel to appear.
> Winxp with the latest nightly.
> 
> Error: gToolbox.palette is null
> Source file: chrome://global/content/customizeToolbar.js
> Line: 330

I found a way to solve this in the add-on. No need for the patch in comment #87. But the 2-line patch of https://bugzilla.mozilla.org/attachment.cgi?id=408208 is still needed!
Sorry for the spam, but I also found a way to solve the junk button problem within the add-on. So no real need for the patch (beside the fact, that this would be the cleaner solution).
Comment on attachment 408208 [details] [diff] [review]
[checked in] Patch to make the add-on work with Thunderbird when the junk button is removed.

r=dmose, a=dmose for Thunderbird trunk and branch landing.
Attachment #408208 - Flags: review?
Attachment #408208 - Flags: review+
Attachment #408208 - Flags: approval-thunderbird3+
Am traveling and still have a blocker to my name, so my attention is likely to be spotty in the upcoming days.  I've given review to the core warning fix for the add-on, so it can be landed on trunk, and approval so that it can be landed on branch for Thunderbird 3 as well.  (In the future, please remember to request review a particular person, otherwise the review is likely to simply get lost!)

As mentioned previously, I'm happy to delegate review to bwinton for stuff in this bug...
Keywords: checkin-needed
Whiteboard: [reviewed patch needs check-in on trunk _AND_ branch]
Comment on attachment 408208 [details] [diff] [review]
[checked in] Patch to make the add-on work with Thunderbird when the junk button is removed.

Checked in:
http://hg.mozilla.org/comm-central/rev/7840e7777ca8
http://hg.mozilla.org/releases/comm-1.9.1/rev/38625544d8c5
Attachment #408208 - Attachment description: Patch to make the add-on work with Thunderbird when the junk button is removed. → [checked in] Patch to make the add-on work with Thunderbird when the junk button is removed.
Keywords: checkin-needed
Whiteboard: [reviewed patch needs check-in on trunk _AND_ branch]
Comment on attachment 408209 [details] [diff] [review]
The latest state of the patch for Thunderbird (without add-on)

Just to note: I got some bit rot here with attachment 408208 [details] [diff] [review] now checked in.
At the moment I only work on the add-on code (assuming, that this will not be integrated into Thunderbird 3.0).

You can get it here:
http://forums.mozillazine.org/viewtopic.php?f=48&t=1553385
http://customizeheadertoolbar.mozdev.org/
https://addons.mozilla.org/thunderbird/addon/45601
Comment on attachment 408209 [details] [diff] [review]
The latest state of the patch for Thunderbird (without add-on)

I need to clear this off my list.  Comment #86 has most of my comments so far for this patch.
Attachment #408209 - Flags: ui-review?(clarkbw) → ui-review-
(In reply to comment #86)
> (From update of attachment 408209 [details] [diff] [review])
> We need to add some kind of inset border + dark background color to indicate
> the toolbox area available when customizing.  I had a really hard time adding
> the ( reply ) button to the far left because I couldn't see where it was
> possible to land.
I added this to the newest version (0.1.6) of the add-on.
> Along with that idea we should probably extend the toolbox area beyond the
> buttons with some padding or something (at least start/end padding).
I think now this should not be necessary but you might be able to convince me to add it anyway.

> There seems to be some odd spacing problems between the archive and junk
> buttons in text beside icons mode.
I could not reproduce this. Could you create a screenshot and post it?

> The reply button doesn't ever seem to get an icon and so when in icon mode only it just appears as a small dot beside the smart button.
This should be fixed (or it is Mac OS X specific and I cannot test it).

> We should probably also put the Customize menu in the ( other actions | v) drop
> down so that it's always available even if you removed all the buttons from the
> header.  Otherwise (as was noted before) when all the buttons are gone it's
> hard to figure out where to click to get the context menu again.
I added the context customization menu to the whole header pane (with the exception of the subject). Now it shouldn't be a problem to access it.

Unfortunately the add-on gets a little bit complicated due to add-on specific things like setup after install and cleanup before uninstall.

You might also want to have a look at the CompactHeader add-on. It includes the code of the CustomizationHeaderToolbar add-on but also some things more, e.g. it copies some buttons from the (original) mail toolbar, like the tag button to the header toolbar. If the customization is not included in Thunderbird 3.0 it might be worthwhile to integrate this functionality of this add-on.

https://addons.mozilla.org/de/thunderbird/addon/45601
https://addons.mozilla.org/de/thunderbird/addon/13564
Depends on: 456169
Depends on: 532009
herb, now that TB 3.0 is released and you have made some progress with your extension, are you planning to get back to this bug and provide a patch for
the soon upcoming 3.1? This would be great, otherwise someone else would have
to take over and finalize it to become part of the regular release.
The css style for Mac OS X is not tested. For Windows/Linux this is what I think makes sense.
Attachment #408209 - Attachment is obsolete: true
Attachment #417317 - Flags: ui-review?(clarkbw)
Attachment #417317 - Flags: review?
Attachment #417317 - Attachment description: Current state of the patch based on add-on CustomizeHeaderToolbar-0.2.1.xpi → Current state of the patch based on add-on CustomizeHeaderToolbar-0.2.2.xpi
Attachment #417317 - Flags: review? → review?(dmose)
Thanks for the prompt action!
With the current patch, the customization dialog opens so that half of it is "displayed" outside of the screen (to the right). Is there a way to display the customization dialog so that its right border is aligned with the right border of the screen?
(In reply to comment #100)
> Created an attachment (id=417317) [details]
> Current state of the patch based on add-on CustomizeHeaderToolbar-0.2.1.xpi
> 
> The css style for Mac OS X is not tested. For Windows/Linux this is what I
> think makes sense.

Try build for this patch queued just now.
I noticed that the try build for Windows 7 displays the label of the archive and junk button at the correct hight if selecting the icon beside text mode. The same build displays the labels one pixel too low if started on Windows XP. 

Therefore I created this new patch where the width of the icons is set to zero so the buttons keep the correct vertical spacing but do not show the icons. I am not sure if this is really better, because it could make things more difficult for overlays in add-ons.
(In reply to comment #47)
> The design of hiding the archive and junk buttons, even in text+icons mode is
> intentional, and should be preserved. 

I'm sorry for being a little too late in the thread, but may I say how stupid this decision is from my point of view? Who is that smart guy who knows better than me that "icons+text" means "icons+text with a few exceptions"? I'd like to throw a rotten tomato at him.
We at Mozilla continue to strive to grow the community around building our software, in part by improving the experience of contributing and making it more enjoyable.  For that to work, it's important to keep in mind that everyone who is contributing is doing what they believe to be best for Thunderbird, regardless if whether it's something you happen to agree with.  It's also important to keep in mind that neither venting nor personal attacks appropriate behavior in Bugzilla.  Please be civil in the future.
I'm trying to be civil, and that's why I explicitly stated that I'm defending my PoV.
I've been using 3.0 betas for quite some time, and I thought that missing icons were a bug that would be fixed eventually. It's probably my fault for not reporting this, but I did think that it was obvious enough to be spotted by developers.
Would you please enlighten me how degrading "icons+text" to "Icons+Text here and just Text there" is best for Thunderbird? I didn't see any discussion regarding this in the comments of this bug (did a text search for "archive"), and I don't know where to look for it, and I DO find this design ugly.
There was already some kind of discussion about the icons for the archive/junk buttons in comments #46, #47.

Actually I agree with comment #109 that if you choose icons+text you would expect to have it for all buttons. This is the reason why I turned them on for the two buttons in my add-on CompactHeader (https://addons.mozilla.org/thunderbird/addon/13564).

Nevertheless if you patch TB 3.1pre with https://bugzilla.mozilla.org/attachment.cgi?id=417754&action=edit you could use the following code in your userChrome.css to turn the buttons on again:

toolbox[labelalign="end"] toolbar[mode="full"] .hdrArchiveButton .toolbarbutton-icon,
toolbox[labelalign="end"] toolbar[mode="full"] .hdrJunkButton .toolbarbutton-icon {
  width: auto !important;
}

toolbar[mode="full"] .hdrArchiveButton,
toolbar[mode="full"] .hdrJunkButton {
  -moz-padding-start: 4px !important;
}

For TB 3.0 you can add the following to userChrome.css (for Windows; check the file mail/themes/***/mail/messageHeader.css for the other OS to find the url/rect):

.hdrArchiveButton {
  list-style-image: url("chrome://messenger/skin/icons/mail-toolbar-small.png") !important;
  -moz-image-region: rect(0px 320px 16px 304px);
}

.hdrJunkButton {
  list-style-image: url("chrome://messenger/skin/icons/mail-toolbar-small.png") !important;
  -moz-image-region: rect(0px 144px 16px 128px);
}
(In reply to comment #112)
> Actually I agree with comment #109 that if you choose icons+text you would
> expect to have it for all buttons.

I would second that (notwithstanding comment #110 being correct that rotten tomatoes shouldn't be part of a bug report), the current way of some buttons opting out from displaying an icon is not well motivated and looks a bit buggy. Thus, I'd prefer attachment 417317 [details] [diff] [review] which consistently shows the icons
for all buttons as selected (as I understand it). If it's a deal breaker for allowing the toolbar to get its full customization palette, then maybe spin it off into a separate bug as it isn't really part of my bug report.

Bryan, any UX opinion yet on either this issue or herb's patch as a whole?
Here's another thing that also looks a little buggy: the small arrow near the Reply button. It doesn't look like a part of the button until is hovered, but I think it should.
First of all, a happy New Year 2010 to everyone! I hope that we'll all continue to build on the great cooperative work that's gone into TB3 and make it even better with ideas from and for the community. A big thank you to TB developers who braved all the scepticism regarding the remaining shortcomings in favour of shipping all the benefits gained in recent years. I've migrated a couple of family PC's and the experience was great, perfectly smooth and easy. Having said which, there's still a lot to be desired. So let's get rolling... ;)

I must admit that if it wasn't against the nettiquette, I'd be very tempted to join Rimas with the tomatoes... It's not about attacking anyone (we all know Rimas is a nice guy, and Bryan does a great job), it's really more like a way of showing how bad it feels for users to look at that eyesore of the missing icons, and to have our UI preferences ignored when we deliberately choose "text+icons".

(In reply to comment #111)
> Would you please enlighten me how degrading "icons+text" to "Icons+Text here
> and just Text there" is best for Thunderbird?

I agree with Rimas, herb (comment 112), rsx11m (comment 113), jmordax (reporter of bug 529496), and Magnus (bug 465138 comment 50) that the design decision to skip those two icons creates much more problems than it might potentially solve. There's two bugs about this: bug 465138, and bug 521303 where I explain in detail why this decision is based on weak reasons and should therefore be reconsidered.

> I didn't see any discussion regarding this in the comments of this bug (did a
> text search for "archive"), and I don't know where to look for it, and I DO
> find this design ugly.

It's not just ugly, we also lose usability, and it violates the UI principles of choice and consistency. There hasn't been real discussion about this, and the comments by Dan and Bryan where afasik this was first introduced actually don't sound too sure about it (Bug 465138, comment #53 and bug 465138, comment #55):

> 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.

There were two reasons given:
1) junk and archive are visually "weak" icons (thus less useful to show)
2) save horizontal space in the header (based on 1)

How to move on...

Fortunately, for both of these reasons, the situation has changed since the design decision was taken.

1) (weak icons) We have re-introduced the "strong" junk icon used in TB2 (the orange flame), so I'll skip 1) as it no longer applies and is kind of invalid (pls read bug 521303 comment 0 for explanation).

2) (save space) After this bug 519956 will be implemented, the user can customize which/how many buttons he wants to show on the header toolbar. Thus the space problem will become even less relevant than before:

a) for users with 17" or wide screens (on the increase), there is no space problem from the start (see screenshots in bug 521303)
b) for users of small screens or Lightning calendar addon, there will be a space problem, but more or less randomly skipping two out of 5 icons will NOT solve that problem!
c) user can choose "text only" or "icon only" modes to save space
d) user can hide unwanted buttons to save space (after implementing bug 519956)

e) junk is a destructive action that deserves the added clarity of an icon
f) junk and archive buttons are hard to parse without icons
g) junk and archive buttons are hard to tell apart without icons

To sum up:
- many voices call for a return of the missing icons, with sound arguments
- the relevance of the of the original arguments has changed (less relevant)
- many users won't have the space problem we seek to alleviate, and otherwise
- there's many other existing options to alleviate the space problem
- apart from avoiding the immediate impression of a UI bug, we should respect the users' choice "icons + text" and be consistent throughout the UI

Hopefully in view of the new situation, especially after implementing this bug, it will be easier to reconsider the current design.

Thomas (awaiting to be chastised by Wayne for the unabridged presentation of this comment) :)
Anyone who wants to have the missing archive and junk icons back on the message header toolbar buttons (for icons+text mode), should vote for bug 529496 using the (vote) feature.
Strongly agree with Thomas D's point (in comment 115) about the "orange flame" icon being very strong and self-explanatory. Since his big post was here, rather than #529496, I posted here-- even though I understand the implementation to  probably be via re-opening #529496.
(In reply to comment #114)
> Here's another thing that also looks a little buggy: the small arrow near the
> Reply button. It doesn't look like a part of the button until is hovered,
> but I think it should.

But I have an even more fundamental question: What are these arrows (on Reply and Reply-All) supposed to do? On Linux, it looks like an actual button, separate from the Action button to the left-- but when you click it, nothing happens. (They put up descriptions when hovering, but perform absolutely nothing). The "REAL" Action icons work fine. If they're not intended to do something special, then they're an eye-catching focus problem (i.e., "what is that thing, why am I looking at it?"

If they ARE supposed to do something, then it's broken in Linux, please advise and I'll open a bug on that issue.
(In reply to comment #118)
> But I have an even more fundamental question: What are these arrows (on Reply
> and Reply-All) supposed to do? On Linux, it looks like an actual button,
> separate from the Action button to the left-- but when you click it, nothing
> happens. (They put up descriptions when hovering, but perform absolutely
> nothing). The "REAL" Action icons work fine. If they're not intended to do
> something special, then they're an eye-catching focus problem (i.e., "what is
> that thing, why am I looking at it?"
> 
> If they ARE supposed to do something, then it's broken in Linux, please advise
> and I'll open a bug on that issue.

I think it worked fine for me in Linux. The arrows are supposed to bring a menu, like here: http://img138.imageshack.us/img138/2594/replydropdown.png. I wonder why the menu for "Reply" button seems to always be just this one item - Reply.
By the way, on Windows, those arrows don't look like part of a button even when hovered/clicked (that's even worse than in Linux). Perhaps such issues should be reported as separate bugs?
The reply-button menu definitely is not related to the customization of the header-pane toolbar handled here, thus yes please, move that discussion elsewhere and keep this bug on topic.
Assignee: nobody → herb
This would (should) solve also bug 529496.
Attachment #417317 - Attachment is obsolete: true
Attachment #417754 - Attachment is obsolete: true
Attachment #421398 - Flags: ui-review?(clarkbw)
Attachment #421398 - Flags: review?(dmose)
Attachment #417317 - Flags: ui-review?(clarkbw)
Attachment #417317 - Flags: review?(dmose)
Oops, I though that attachment 417317 [details] [diff] [review] had already taken care of the
inconsistency in the icons, and that attachment 417754 [details] [diff] [review] was the same
but retaining the special handing, hence my bug 529496 comment #15.
Sorry for any extra work...
Just to get this bug active again: At the moment the buttons in the customization dialog are displayed not as flat buttons (like it is done in the customization dialog for the main mail toolbar). Should this be changed?

What is the target milestone for this bug (as alpha1 is (nearly) out)? When is this target milestone expected to be released. I would like to avoid that this bug doesn't make it into the final release again.
TB 3.1a1 is virtually out, the deadline for 3.1b1 is - per current schedule - February 9, thus in two weeks from now, which would also be the string freeze.

Bryan, Dan: Can we get some opinion from you on this patch, which has been up for commenting/review in one version or another for months now? Thanks.
Whiteboard: [needs ui-r clarkbw, review dmose]
I have published a new (beta) version of the CustomizeHeaderToolbar 0.3.0beta1. The buttons are now shown flat in the customization dialog (like in all other toolbar customization dialogs in Thunderbird or Firefox).
I found a way to fix the problem described in comment #102:

Either add to the arguments in line 139 of mailCore.js http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#139 "centerscreen". Then all customization dialogs for toolbars in Thunderbird would be displayed in the center of the screen. Perhaps it is also possible to add an (optional) argument for the the function CustomizeMailToolbar to decide if the dialog should be placed in the center of the screen.

And/or add an overlay for the customization dialog and add persist="screenX screenY" to the dialog tag. The if the dialog is displayed outside the screen and the user moves it inside, it will appear there the next time again.

Which way should be added to the patch?
Anything you do might conflict with customizeToolbars->repositionDialog()
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/customizeToolbar.js#118

I think it would be better if you could modify the toolkit code so that the customize window takes more arguments that can then be passed to repositionDialog().
> I think it would be better if you could modify the toolkit code

Uhm, that would probably make a solution for TB 3.1 impossible as 1.9.2 should be well beyond being frozen by now for such changes...
Well, I found this comment "refreshing" to say the least.
https://bugzilla.mozilla.org/show_bug.cgi?id=543954#c8

I'll go a step further, and say that IMO herb@leo.org single-handidly saved
the TB3 effort from complete disaster.

Anybody that reads the forums, Get satisfaction, or Hendrix knows that this option should be included in the basic TB package, without an extension.
>> I think it would be better if you could modify the toolkit code

> Uhm, that would probably make a solution for TB 3.1 impossible as 1.9.2 should
> be well beyond being frozen by now for such changes...

Thunderbird has forked the Customize Toolbar code before.

And I temporarily forked the Customize Toolbar code in SeaMonkey while waiting for my patches to make it into mozilla-central.

In the current case I think just overlaying the onLoad() and repositionDialog() functions in customizeToolbar would suffice.
(In reply to comment #130)
> In the current case I think just overlaying the onLoad() and repositionDialog()
> functions in customizeToolbar would suffice.

So if I understand you correctly, I would replace the call to onLoad() inside overlayOnLoad():
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#90
by my own function? Wouldn't it be possible to just call a function overlayRepositionDialog() after the call to onLoad() and place the dialog on the screen so, that is fully visible (if possible).

Or should I remove the call to onLoad() altogether and copy the 10 lines of code to the overlayOnLoad() function and replace the call to repositionDialog() with my own new function overlayRepositionDialog()?

If it's not possible to show the full customization dialog on the screen which parts should be moved out of the screen? The right and the bottom? Or should the dialog be resized? How could this be done?
> So if I understand you correctly, I would replace the call to onLoad() inside
> overlayOnLoad():
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#90
Hmm. I forgot that Thunderbird already does this. This makes things easier!

> by my own function? Wouldn't it be possible to just call a function
> overlayRepositionDialog() after the call to onLoad() and place the dialog on
> the screen so, that is fully visible (if possible).
Yes the latter is what I meant. Sorry for the ambiguity.

> Or should I remove the call to onLoad() altogether and copy the 10 lines of
> code to the overlayOnLoad() function and replace the call to repositionDialog()
> with my own new function overlayRepositionDialog()?
This is more fragile and I don't recommend this except as a last resort.

But the best long term solution is (as we did in SeaMonkey) is to get some patches in to toolkit to add clearly defined intercept points for extensions to hook into. This isn't unprecedented. Both Firefox and Thunderbird contain code whose only purpose is to make it easier for extensions to hook into and modify behaviour. For example, both the Firefox/SeaMonkey implementations of the PageInfo window have onLoadRegistry[] and onReloadRegistry[] so that extensions don't have to override the onLoad() function.
Targetting at beta2, because that's string freeze, and this is likely to need strings.  Sorry for not getting back to this sooner, Herb!  

One thing we've changed about our tree policies is that we're now requiring patches to have automated tests in order for them to land in the tree.  That said, I want to be sensitive to the fact that you wrote this patch long before those polices were in effect. 

So I'll start by asking if you'd be up for taking a run at putting together some MozMill tests for these changes, because that would be a huge help in preventing the regressions that our message header pane has been so prone to in the past.  If not, let me know, and I'll try and work something else out.
blocking-thunderbird3.1: --- → beta2+
Flags: blocking-thunderbird3.1+
This patch adds overlayRepositionDialog() to move the customization dialog onto the screen completely (if possible).
Attachment #421398 - Attachment is obsolete: true
Attachment #426817 - Flags: ui-review?(clarkbw)
Attachment #426817 - Flags: review?(dmose)
Attachment #421398 - Flags: ui-review?(clarkbw)
Attachment #421398 - Flags: review?(dmose)
(In reply to comment #133)
> One thing we've changed about our tree policies is that we're now requiring
> patches to have automated tests in order for them to land in the tree.  That
> said, I want to be sensitive to the fact that you wrote this patch long before
> those polices were in effect. 
> 
> So I'll start by asking if you'd be up for taking a run at putting together
> some MozMill tests for these changes, because that would be a huge help in
> preventing the regressions that our message header pane has been so prone to in
> the past.  If not, let me know, and I'll try and work something else out.
In principle I am willing to supply Mozmill tests. BUT I have never done it before and I am not sure what and how to test.

I found this for header testing:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/message-header/test-message-header.js
@Dan ##133
Pushing for a better quality is more than OK, MozMill could be a step into that direction. 
Not sure how experienced the folk is about MozMill. At least I fell the add-on developers aren't. Is there any help about that for Thunderbird/extension? I just recognized "Learn how to testscript your Add-ons with MozMill!" [1] taking place online at March 5th, but that's for Firefox.
Any help for TB/ext also? 
I'm very much interested to use it for TB/extension development too.
Günter

PS    For this bug/development I think any help should be provided by the core team to get that feature ready with the next nearest TB release!! 

[1] http://quality.mozilla.org/events/2010/mar/05/learn-how-testscript-your-add-ons-mozmill
(In reply to comment #136)
> Not sure how experienced the folk is about MozMill. At least I fell the add-on
> developers aren't. Is there any help about that for Thunderbird/extension?

We have some information up at https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing that might help.

Thanks,
Blake.
Thanks Blake,
I don't want to use this bug to discuss "How to" running TB/MozMill, but ..

I've seen that page before, and sorry skipped it. Maybe I'm wrong, but I don't want to run a build to get the test running but just 'install' MozMill (as an extension??) and use it. You see my 'inexperienced' attitude ...
Any other TB/MozMill discussion/forum/.. going on elsewhere ?
Sorry, but this discussion on tests seems upside-down to me. Wouldn't it be necessary to decide in general on the patch pending review if this is what's desired and if everything works, maybe then start thinking about how to test?

I have to agree with comment #136, many if not most volunteer contributors
have no clue how the testing environment works (that includes myself) and are sufficiently occupied with getting their main patch working and approved. Additionally, this specific bug (not to mention the two bugs before it) was planned to be included into 3.0 before it ended up in an extension. So, this appears to be a perfect example for a "grandfathering" case, also considering that other mailnews patches went into the tree without any tests included even after the new "must have" policy was established.

The header pane is a moving target right now, various bugs are pending with or without patches, subject to change before 3.1 comes. Thus, I don't know which value incremental updates to changing tests would have compared with getting the feature work done first and then consolidate all of those into a single test-specific bug. Current tests aren't even up to date yet, there is still
bug 527550 for testing of bug 489609 pending review, to name an example.

So again, please let's do things in the right order: (1) review herb's patch and give feedback to UX and technical aspects so that it can be finalized to make everybody happy; (2) think about how to test it, and if the tests can or should be spun off into a second patch/bug to allow this to finally proceed.
(In reply to comment #134)
> Created an attachment (id=426817) [details]
> Includes repositioning of customization dialog
> 
> This patch adds overlayRepositionDialog() to move the customization dialog onto
> the screen completely (if possible).

Try build queued up with that patch
(In reply to comment #138)
> I've seen that page before, and sorry skipped it. Maybe I'm wrong, but I don't
> want to run a build to get the test running but just 'install' MozMill (as an
> extension??) and use it. You see my 'inexperienced' attitude ...
> Any other TB/MozMill discussion/forum/.. going on elsewhere ?

The best place for TB specific MozMill items is the mozilla.dev.apps.thunderbird newsgroup. We can point you at MozMill specific locations if necessary.

(In reply to comment #139)
> So again, please let's do things in the right order: (1) review herb's patch
> and give feedback to UX and technical aspects so that it can be finalized to
> make everybody happy; (2) think about how to test it, and if the tests can or
> should be spun off into a second patch/bug to allow this to finally proceed.

Please be aware, Dan's request was only a request. It was not a requirement imposed upon Herb in this case. AFAICT Herb's patch is up for review and so now is an ideal time to consider writing automated tests for it whilst the reviewers process their pending work and get time to review it.
(In reply to comment #130)
> >> I think it would be better if you could modify the toolkit code
> 
> > Uhm, that would probably make a solution for TB 3.1 impossible as 1.9.2 should
> > be well beyond being frozen by now for such changes...

This depends in large part of the scope of the change that would be necessary and how it effects Firefox, if at all.

> Thunderbird has forked the Customize Toolbar code before.
> 
> And I temporarily forked the Customize Toolbar code in SeaMonkey while waiting
> for my patches to make it into mozilla-central.

Indeed; we'll consider temporary forks if appropriate.

> In the current case I think just overlaying the onLoad() and repositionDialog()
> functions in customizeToolbar would suffice.

But overriding functions does indeed sound cleaner (note that the above comments are all general: I haven't actually looked at the code you guys are discussing directly).

(In reply to comment #135)
> In principle I am willing to supply Mozmill tests.

Great!

> BUT I have never done it before and I am not sure what and how to test.

The biggest thing to test is a judgement call: "does the most important functionality work?" 
Also worth looking at are edge cases or unusual parameters to function calls.  I wish we had a good document describing how to think about this in detail.  One question you can ask yourself is "what are the top several things that this patch adds that I would be most disappointed if some future change to the code broke?".  Note that we're not looking for perfection here, just coverage of some of the most important bits.   

A few possibilities that I can think of off the top of my head: "does dragging a button into the toolbar work?", "is the default set of buttons right?", "are all the important buttons there?", etc.  It's conceivable that the toolkit code here has some tests already that may offer inspiration as well, though that code is old enough that it might well not.  Or maybe there are even Firefox toolbar customization tests.  Either of those would probably written using mochitest or mochichrome rather than mozmill, however, so they might not be terribly applicable.

> I found this for header testing:
> http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/message-header/test-message-header.js

That's the one.  Skimming over that and other tests should give you some ideas about stuff you might want to test and how to go about it.

(In reply to comment #139)
> Sorry, but this discussion on tests seems upside-down to me. Wouldn't it be
> necessary to decide in general on the patch pending review if this is what's
> desired and if everything works, maybe then start thinking about how to test?

There is indeed truth in this!  I should have been more clear that I was just trying to be sure that Herb wasn't caught by surprise about the set of things that needs to happen in order to this code to make 3.1.  FYI, clarkbw is out of the office this week, so ui-review will be a bit yet.  All that said, I suspect a fair amount of basic functional testing won't change much because of UI-review modifications needed.
 
> I have to agree with comment #136, many if not most volunteer contributors
> have no clue how the testing environment works (that includes myself) and are
> sufficiently occupied with getting their main patch working and approved.
> Additionally, this specific bug (not to mention the two bugs before it) was
> planned to be included into 3.0 before it ended up in an extension. So, this
> appears to be a perfect example for a "grandfathering" case, also considering
> that other mailnews patches went into the tree without any tests included even
> after the new "must have" policy was established.

If we get to a point where we absolutely have to consider grandfathering this bug to avoid it slipping past 3.1, we can certainly put that option on the table.  But I don't think we're there now, and if we can get tests for it, we'll be in a happier place.

> The header pane is a moving target right now, various bugs are pending with or
> without patches, subject to change before 3.1 comes.

True.

> Thus, I don't know which value incremental updates to changing tests would have
> compared with getting the feature work done first and then consolidate all 
> of those into a single test-specific bug. 

That sounds significantly harder to me, in part because it's always easy for those bugs to get cut at the last minute.  It also loses one of the important benefits to test-driven development: writing tests at the same time as (or before) the code is written generally finds design problems early, while they still can be fixed easily, and additionally tends to result in code factored in way that's actually more testable.  

Not all of the above paragraph applies to this bug specifically, but I really believe that the bundling strategy proposed is likely to lead to a less good result.

> Current tests aren't even up to date yet, there is still
> bug 527550 for testing of bug 489609 pending review, to name an example.

Indeed, but as far as I can tell, that's orthogonal.
Comment on attachment 426817 [details] [diff] [review]
Includes repositioning of customization dialog

I just spent some time playing the try builds from comment 142 today and I only have 2 real issues to report.

1. Initially the toolbar was set to text below icons and as such the buttons looked huge.

2. The reply to sender button doesn't have an icon associated with it, not matter what mode it's in.

I like the the reply to sender (only) has been made a regular button instead of a dual-button.
* It would be good if the smart reply button did this as well when there was only reply to sender.

I like the vertical positioning of the customize dialog.
* However the way it moves out to the right is a little awkward and flashes the screen.  I'm not sure why it needs to be repositioned the way it is, it would be good to just have it align under the buttons but not off the window as you said in earlier comments.



When I switch back to a previous version the button order becomes somewhat random.  I'm wondering if we'll have issues with people trying this, not liking it and going back but being stuck with buttons they can't customize.

removing ui-review so this is out of my queue for now.  I feel like I can ui-r+ this given a bit more testing, some comments to the above with fixes to the issues.
Attachment #426817 - Flags: ui-review?(clarkbw)
(In reply to comment #144)
> 1. Initially the toolbar was set to text below icons and as such the buttons
> looked huge.
This only happens if you upgrade to the new version (Thunderbird 3.1). To avoid it, some code at the startup of Thunderbird has to be added which checks if the old settings are used.

> 2. The reply to sender button doesn't have an icon associated with it, not
> matter what mode it's in.
Which operating system are you using? Here (Windows 7/XP, Linux) I see one.

> I like the the reply to sender (only) has been made a regular button instead of
> a dual-button.
> * It would be good if the smart reply button did this as well when there was
> only reply to sender.
I will look into this.

> I like the vertical positioning of the customize dialog.
> * However the way it moves out to the right is a little awkward and flashes the
> screen.  I'm not sure why it needs to be repositioned the way it is, it would
> be good to just have it align under the buttons but not off the window as you
> said in earlier comments.
What do you exactly mean? Here the customization dialog is placed below the icons and the right border of the dialog is aligned to the right border of the screen.

> When I switch back to a previous version the button order becomes somewhat
> random.  I'm wondering if we'll have issues with people trying this, not liking
> it and going back but being stuck with buttons they can't customize.
This would happen only, if you switch back from Thunderbird 3.1 to 3.0.x. I am not sure how to fix this. One way would be to rename header-view-toolbox and header-view-toolbar to e.g. header-view-toolbox-v2. This would also solve the problem with the wrong button icon/text mode when starting the first time. But it would break add-ons/themes. So probably not a good idea?!?

P.S. I cannot test on Mac OS X.
Herb, first of all a huge thank you for the great and continuous work you do in this bug. Once landed, this will be a true blessing for the community!

Herb, as in the attached screenshot, I also see the positioning problem of the customization dialogue (mentioned by Bryan) on my WinXP, using tryserver build 2010-02-16 from commment 142 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100215 Shredder/3.2a1pre).

More nits:

a) starts up with text below icons as mentioned by Bryan. I don't understand where it comes from, and I don't understand Herb's comment about it.

b) in the customize dialogue, "Show: " dropdown is empty, but it should show "Icons and Text" (which is old language for "Icons above Text", maybe we should change that string?). It's empty because "Icons and Text" option is missing (and shouldn't be).

c) positioning problems
c0) customization dialogue is in the far right corner, which looks bad, maybe also because of c1 and c2
c1) customization dialogue completely covers header buttons that I want to customize. Oh, I just realize it remembers size after resizing, which will fix c1 and c2. But this was the original size it came up with from the box (way too large, see d).
c2) customization dialogue bottom part including action buttons is covered by windows task bar (due to c1 and c2, user cannot act in any way on this dialogue without moving it first)

d) dialogue initially was very large (but remembers size when reduced, that's why Herb doesn't see this any more). Maybe it's better not to remember size and default to just enough to hold all icons that are currently in the dialogue and a bit of blank space (say one extra row) for users to drop stuff on?

e) dialogue doesn't remember position. Instead of remembering size, maybe we should remember position? Or maybe not? (I haven't fully reflected this yet)

f) there are huge gaps between the buttons that you drop onto the dialogue
(probably toolkit to blame)

g) Icons are all big (probably 24px). Cannot customize to use small icons. Checkbox for "use small icons" is missing. You'll get into serious trouble with Bryan who has fought tooth and nail about saving (icons') pixels in header pane.

h) summary of this bug promises "View toggle" to switch on and off the whole header toolbar, as we can do with right click on "Mail toolbar". On header pane, context menu should be:
_/ Message Header Toolbar (where _/ is a tick)
   Customize...
Herb, is this possible? Would be really cool.

Some of these can probably be blamed on the toolkit...
Is the starting point for this dialog the file chrome://global/content/customizeToolbar.xul

If so, I invested hours tweaking it for Tb2 by writing new CSS rules into my userChrome.css. Primary result was reduction of wasted white space between rows and the icons per row. I also achieved  reduced need for the overflow stroller and have 5 rows if icons.

One of the things I found out about was the hardcoding of the boxes position and size. Two things I wanted to change, but decided hacking the Js was out of my depth. So I settled for the CSS mods which can provide if they will help.
(In reply to comment #146)
> a) starts up with text below icons as mentioned by Bryan. I don't understand
> where it comes from, and I don't understand Herb's comment about it.
I guess the reason is that the icon/icon+text/text setting of the header toolbar is saved in the localstore.rdf file in the profile of the user. If the new version of TB is started the first time somehow the old settings result in the described behavior. After setting this mode the in the customization dialog to one of the available options this inconsistency goes away. This could be solved by checking at each start up of TB for this inconsistency and fixing it if necessary. The other solution I mention in comment #145 is to rename the toolbar/toolbox. The the old settings do not interfere with the new design of the toolbar.

> b) in the customize dialogue, "Show: " dropdown is empty, but it should show
> "Icons and Text" (which is old language for "Icons above Text", maybe we should
> change that string?). It's empty because "Icons and Text" option is missing
> (and shouldn't be).
I removed this string because I thought the option Icons above Text should not be available in the header pane. If the inconsistency (see a)) is fixed also this problem should go away. 

On the other hand: If the Icons above Text mode should be available I could change this accordingly. 
Bryan(?)/Mozilla team: What do you think?

> c) positioning problems
> c1) customization dialogue completely covers header buttons that I want to
> customize. Oh, I just realize it remembers size after resizing, which will fix
> c1 and c2. But this was the original size it came up with from the box (way too
> large, see d).
> d) dialogue initially was very large (but remembers size when reduced, that's
> why Herb doesn't see this any more). Maybe it's better not to remember size and
> default to just enough to hold all icons that are currently in the dialogue and
> a bit of blank space (say one extra row) for users to drop stuff on?
> 
> e) dialogue doesn't remember position. Instead of remembering size, maybe we
> should remember position? Or maybe not? (I haven't fully reflected this yet)
I can confirm this. You are right: For the dialog either the position and the size should be set automatically or both, position and size, should be saved (it would be done by the above mentioned localstore.rdf file). I think both is possible so this is a "management decision" (Bryan?) again.

> f) there are huge gaps between the buttons that you drop onto the dialogue
> (probably toolkit to blame)
I also guess so.

> g) Icons are all big (probably 24px). Cannot customize to use small icons.
> Checkbox for "use small icons" is missing. You'll get into serious trouble with
> Bryan who has fought tooth and nail about saving (icons') pixels in header
> pane.
The icons are only displayed big (actually "normal" size) in the customization dialog. In the header pane toolbar they are always display small. The reason I chose to display the big icons in the customization dialog is that it is always done this way (see the dialogs of all other toolbars). Normally you have the choice between normal (big) and small icons for the toolbar itself but the size the icons are displayed in the customization dialog is not changed.

I removed the size selection option from the dialog because I thought the icons in the header pane should always shown with small size. 

I could also change this: Either allow both sizes in the header pane or display the small icons in the dialog (or leave it the way it is at the moment). Again management: Which way to go?

> h) summary of this bug promises "View toggle" to switch on and off the whole
> header toolbar, as we can do with right click on "Mail toolbar". On header
> pane, context menu should be:
> _/ Message Header Toolbar (where _/ is a tick)
>    Customize...
> Herb, is this possible? Would be really cool.
I guess it would be quite easy to add a menu entry to the context menu to do this. I think I will add this to the next version of the patch (if there are no objections). Perhaps I will first test it in the CustomizeHeaderToolbar add-on.
(In reply to comment #147)
> Is the starting point for this dialog the file
> chrome://global/content/customizeToolbar.xul
> 
> If so, I invested hours tweaking it for Tb2 by writing new CSS rules into my
> userChrome.css. Primary result was reduction of wasted white space between rows
> and the icons per row. I also achieved  reduced need for the overflow stroller
> and have 5 rows if icons.
> 
Are you sure it is worth it to port this back to Tb2? This bug is certainly about adding this feature to Tb3.
Porting back to Tb2 was not my reason for the reference. 

Rather that I was able to modify the appearance of the toolbar pallet, such that issues raised by Thomas D in comment #146 were addressed by a CSS solution. I changed both left-to-right and vertical spacings.
Ok, I look further into the size problem: Inside the toolkit in customizeToolbar.xul the width and height of the customization dialog are set to be persistent:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/customizeToolbar.xul#56

I don't know how to override this. Also it seems not possible to use window.resizeTo() to change the window size in 

So it looks like the only possible solution seems to be to also make the position persistent.

Actually I would prefer the solution to set the position and size of the dialog each time it is opened newly. Then it could be placed always below the toolbar, which moves around by changing its relative position of the header pane.

For the "normal" mail customization dialog this seems not to be a problem because the mail toolbar keeps its position (most of the time), so there is no need to resize the dialog.

Any ideas?
> Any ideas?
Don't you already overlay customizeToolbar.xul?

I think this might work:

document.documentElement.setAttribute("style", "width: XXch; height: YYem;");

Where XX and YY are the values you want.

See: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/customizeToolbar.xul#55
(In reply to comment #151)
> Any ideas?

I think that if the user have changed the size or position, he has done it consciously and for a reason. Therefore, the changed size and position should be remembered across sessions. Of course, nothing speaks against having a good initial default setting...
(In reply to comment #153)
> document.documentElement.setAttribute("style", "width: XXch; height: YYem;");
Neither this suggestion nor window.resizeTo() work. I tried them inside the overlay function for window.onload. Actually the values are set inside this function. But when the window appears, the persistent values are used. 

If going with the suggestion in comment #154 there is the problem that when the user opens the customization dialog of the mail toolbar and resizes it to fill (more or less) the screen and then opens the customization dialog of the header pane toolbar these large values for width and height will be used again. Also if the position is made persistent the values of the mail toolbar would be used for the header pane toolbar and vice versa. 

This is an extract from the localstore.rdf:

<RDF:Description RDF:about="chrome://global/content/customizeToolbar.xul#CustomizeToolbarWindow"
                   width="801"
                   height="401" />

There is only one entry for all customization dialogs (mail toolbar, compose new message, address book, etc.). 

If possible I would suggest that the old behavior is kept, but if the window is too large to be displayed fully within the screen it is resized to fit within the screen. But for this to work it is necessary to override the persistent values. So any other suggestions how to achieve this?
This doesn't seem to be waiting on my review right now
Whiteboard: [needs ui-r clarkbw, review dmose] → [needs review dmose]
Correct; I hope to get to it in the next small number of days; sorry for the delay.
A bug was reported for the CompactHeader add-on (http://forums.mozillazine.org/viewtopic.php?p=9080445#p9080445) which also affects this patch: When switching the layout between classical/wide/vertical view the palette of the header toolbar is emptied. This also happens if Thunderbird starts up not in the classical view.

I think this is the full story of the bug:
In http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/toolbar.xml#125 the palette of a toolbar is removed from the DOM tree and saved as a javascript "class variable". (Can anybody explain why it is done so?)

When switching the layout in http://mxr.mozilla.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#196 the toolbar (as part of the message pane) is move around with the appendChild method. By using this method only the DOM tree gets copied but the javascript "class variables" get lost.

I think I fixed this problem in the new patch by temporarily clone the palette (and other variables) and write them back to the toolbox/toolbar after it was moved around. 

I think this fixes the symptoms of this bug but I am totally not sure that there is other code which could cause the same problem.

I will try to get a new try server build for this patch.
Attachment #426817 - Attachment is obsolete: true
Attachment #437458 - Flags: review?(dmose)
Attachment #426817 - Flags: review?(dmose)
SeaMonkey developers think that removing the toolbarpalette from the DOM isn't necessary and we haven't done that from our initial implementation. Nobody has complained or even noticed, even usually picky extension developers.

In the SeaMonkey override toolbox bindings:
<http://mxr.mozilla.org/comm-central/source/suite/common/bindings/toolbar.xml#79>
We make sure that toolbox.palette is non-null so the toolkit check at:
<http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/toolbar.xml#112>
fails and the palette node isn't removed from the DOM.

An advantage of this for Thunderbird is that you could point the message header toolbox palette property to the main toolbox palette and so share one set of customizable toolbarbuttons between the two toolboxes.

You could then use an additional mode on the header toolbox e.g. mode="mini" to style buttons if placed in the header toolbar.
Here are the links for the(In reply to comment #159)
> In the SeaMonkey override toolbox bindings:
> <http://mxr.mozilla.org/comm-central/source/suite/common/bindings/toolbar.xml#79>
> We make sure that toolbox.palette is non-null so the toolkit check at:
> <http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/toolbar.xml#112>
> fails and the palette node isn't removed from the DOM.
I am not sure that I understand you correctly: 
In line 126 of toolbar.xml is the code which removes the palette from the DOM. So is this code executed in SeaMonkey?

Here are the links to the try server builds:
http://s3.mozillamessaging.com/build/try-server/2010-04-08_11:30-gozer@mozillamessaging.com-bug519956-att437458/gozer@mozillamessaging.com-bug519956-att437458-mail-try-linux.tar.bz2
http://s3.mozillamessaging.com/build/try-server/2010-04-08_11:30-gozer@mozillamessaging.com-bug519956-att437458/gozer@mozillamessaging.com-bug519956-att437458-mail-try-mac.dmg
http://s3.mozillamessaging.com/build/try-server/2010-04-08_11:30-gozer@mozillamessaging.com-bug519956-att437458/gozer@mozillamessaging.com-bug519956-att437458-mail-try-win32.zip
http://s3.mozillamessaging.com/build/try-server/2010-04-08_11:30-gozer@mozillamessaging.com-bug519956-att437458/gozer@mozillamessaging.com-bug519956-att437458-mail-try-win32.installer.exe

I think some decisions have to be reached:
What to do with the questions of comment #148?
Is the handling of the palette problem acceptable as proposed in the most recent patch?
How to handle the problem of sizing and placing the customization dialog (comment #155)?
> I am not sure that I understand you correctly: 
> In line 126 of toolbar.xml is the code which removes the palette from the DOM.
> So is this code executed in SeaMonkey?

No we prevent the code that removes the palette from the DOM from running since at line 112, |toolbox.palette| is true (non-null) thus |if (!toolbox.palette)| fails.
Comment on attachment 437458 [details] [diff] [review]
Fix bug which caused palette to disappear after switching layout (wide/classic/verical view)

Both the code and the behavior are looking good; nice work!

Please add the code you described in section 1) of comment 145 to fix the blank menu for upgraders.

There are a couple of Mac-specific bugs that need to be fixed before this can get an r+:

* the reply button doesn't have an icon on mac, so it looks really odd in icon-only mode
* the "reply all" menubutton has "reply" as the default action and "reply all" in the dropdown

I gather from previous comments that you don't have easy access to a Mac.  Would it be difficult for you to find a Mac buddy to work through these issues?  If so, please send me email, and I'll try and recruit someone.

Re the management questions in comment 148, I think, in general, the way this patch is working now is fine.  If you want to spin out separate bugs for any of them (e.g. the sizing and placement issues in comment 155), you're more than welcome to.

I can live with the handling of the palette bug workaround as you've got it coded in the current patch.

I only saw a few code tweaks that I'd like to see:

* rather than commenting out min-height in the gnomestripe, I'd just remove the rule entirely
* the comment that explains the palette workaround is hard to understand without reading it and the code a few times.  Can you rephrase to make it clearer?
* please add your name and email address to the license boilerplate at the top of the files you're touching

r- (but almost there!)
Attachment #437458 - Flags: review?(dmose) → review-
Whiteboard: [needs review dmose] → [has patch with ui-review; needs updated patch herb, final review dmose]
(In reply to comment #162)
> Please add the code you described in section 1) of comment 145 to fix the blank
> menu for upgraders.
In my add-ons I use a preference setting to see if the add-on version was updated. I guess there must be something similar in Thunderbird itself, which e.g. starts the add-on update check. Could somebody please point me to this code?

> There are a couple of Mac-specific bugs that need to be fixed before this can
> get an r+:
> ...
> I gather from previous comments that you don't have easy access to a Mac. 
> Would it be difficult for you to find a Mac buddy to work through these issues?
>  If so, please send me email, and I'll try and recruit someone.
email sent.

> Re the management questions in comment 148, I think, in general, the way this
> patch is working now is fine.  If you want to spin out separate bugs for any of
> them (e.g. the sizing and placement issues in comment 155), you're more than
> welcome to.
I will do this and post the numbers here.
(In reply to comment #163)
> In my add-ons I use a preference setting to see if the add-on version was
> updated. I guess there must be something similar in Thunderbird itself, which
> e.g. starts the add-on update check. Could somebody please point me to this
> code?

It turns out that Gecko/Tb doesn't really have centralized code to do this, each caller that needs it does it for themselves, and typically using the exact mechanism you describe.  So I'd say stick with that.  :-)

> I will do this and post the numbers here.


Great; thanks!
Note that the canonical way to do this is to use the http user-agent as the version number to track.  See <http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#126> for how the browser does it.
This new version of the patch should solve the following problems:
> * the reply button doesn't have an icon on mac, so it looks really odd in
icon-only mode
There should be an icon now. But I was not able to test this patch for Mac yet.

> Please add the code you described in section 1) of comment 145 to fix the blank
> menu for upgraders.
I found a way to do this without a new preference setting: If there is no "mode" attribute in the header-view-toolbox, it means this is the first time, the header-pane is displayed after an update (it also works, if a message is opened in a "new window"). Then the icon/text mode from "full" to "textbesidesicon". If the mode was "icon" or "textonly" it should stay the same.

> * rather than commenting out min-height in the gnomestripe, I'd just remove the
> rule entirely
> * the comment that explains the palette workaround is hard to understand
> without reading it and the code a few times.  Can you rephrase to make it clearer?
> * please add your name and email address to the license boilerplate at the top
> of the files you're touching
done.

> * the "reply all" menubutton has "reply" as the default action and "reply all"
in the dropdown
I do not understand why the behavior of the Mac version is different from the other OSes. So this is not fixed.
Attachment #437458 - Attachment is obsolete: true
Attachment #439729 - Flags: review?(dmose)
Whiteboard: [has patch with ui-review; needs updated patch herb, final review dmose] → [has patch with ui-review; needs updated patch myk(?), final review dmose]
(In reply to comment #162)
> * the reply button doesn't have an icon on mac, so it looks really odd in
> icon-only mode
> * the "reply all" menubutton has "reply" as the default action and "reply all"
> in the dropdown

I tested the latest version (v18) of the patch (attachment 439729 [details] [diff] [review]) against a recent pull of the trunk on Mac OS X 10.6.3, and it looks like these problems have been fixed.

Specifically, the "reply" button now has an icon on Mac in both "Icons" and "Icons beside Text" modes.

And the "reply all" button appears the same as it does on today's Lanikai nightly on Linux: it defaults to "reply all" when reading a message with multiple recipients, and pressing the drop marker opens a dropdown menu with "reply all" and "reply" options:

[ reply all v ]
+-------------+
| reply all   |
| ----------- |
| reply       |
+-------------+

(FWIW, the "reply all" button also seems to behave correctly when it becomes a "reply list" button.)
Whiteboard: [has patch with ui-review; needs updated patch myk(?), final review dmose] → [has updated patch, needs final review dmose, landing]
Comment on attachment 439729 [details] [diff] [review]
New version of patch: Updatehandling, mac issue, cleanup

There was a bit of trailing whitespace which I cleaned up.  I also added a bit of vertical whitespace to improve readability in a few places.  Otherwise, looks good.  Thanks so much for your hard work!

r=dmose
Attachment #439729 - Flags: review?(dmose) → review+
Pushed: <http://hg.mozilla.org/comm-central/rev/f82d14d6c31d>; I also added you to the Special Thanks section of the credits in the About dialog.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [has updated patch, needs final review dmose, landing]
Thank you for all help. If I get finally mozmill running (see Bug 536542), I will supply the promised test cases.
The follow up bugs are:

Bug 561655 Save size of customization dialogs individually
Bug 561660 Do not remove palette of toolbars from DOM tree
Marking this verified fixed, that's a very good feature to have - thanks herb!
Status: RESOLVED → VERIFIED
Blocks: 565045
No longer blocks: 565045
Depends on: 565045
To create meaningful mozmill testcases for this bug requires Bug 515776  - Drag and drop doesn't work for xul elements  to be fixed.
Thanks for investigating; I've added that to the dependencies of this bug, and added in-testsuite? as well to help keep it from falling off the radar.
Depends on: 515776
Flags: in-testsuite?
Some mozmill tests for the customization of the header pane toolbar:

Moving around buttons within toolbar, drag'n drop them to the customization dialog and back, set defaults, change button styles.

As the drag'n drop code is the same as for the tabmail test, I move it to a new helper file: test-mouse-events-helpers.js, so also the test-tabmail-dragndrop.js file is affected but their should be no functional changes.

Also some removal of redundant code in test-message-header.js

There are try server builds, without the patch:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=a25ef5d9f5f2
and with the patch:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=548b64cd14ba

Part of the code could be use to mozmill test all mozilla/firefox/thunderbird toolbars. Also the drag'n drop code could be used elsewhere. Would there be a central helper-file directory to place this code in?
Attachment #541865 - Flags: review?(bwinton)
Some mozmill tests for the customization of the header pane toolbar:

Moving around buttons within toolbar, drag'n drop them to the customization dialog and back, set defaults, change button styles.

As the drag'n drop code is the same as for the tabmail test, I move it to a new helper file: test-mouse-events-helpers.js, so also the test-tabmail-dragndrop.js file is affected but their should be no functional changes.

Also some removal of redundant code in test-message-header.js

There are try server builds, without the patch:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=a25ef5d9f5f2
and with the patch:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=548b64cd14ba

Part of the code could be use to mozmill test all mozilla/firefox/thunderbird toolbars. Also the drag'n drop code could be used elsewhere. Would there be a central helper-file directory to place this code in?

There was a function name twice in the first version of the patch, so this is a minor correction of it.
Attachment #541865 - Attachment is obsolete: true
Attachment #541866 - Flags: review?(bwinton)
Attachment #541865 - Flags: review?(bwinton)
This version of the mozmill tests check for the preference "toolbar.customization.usesheet". It is set to true for MAC OS X and false for the rest of OSs by default. This was the reason why the tests original failed on OS X. The tests for the (header pane) toolbar are moved to a separate file.

The tests were used for a try server build:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=76ebf3f5e553

The new mozmill tests worked on Linux and OS X.
Attachment #541866 - Attachment is obsolete: true
Attachment #543251 - Flags: review?(bwinton)
Attachment #541866 - Flags: review?(bwinton)
Attachment #543251 - Flags: review?(sid.bugzilla)
Comment on attachment 543251 [details] [diff] [review]
New version of mozmill tests which supports all OS.

These tests pass for me on OS X, but it would be good to get Sid's review too.

Still, in the meantime, r=me.

Thanks,
Blake.
Attachment #543251 - Flags: review?(bwinton) → review+
Comment on attachment 543251 [details] [diff] [review]
New version of mozmill tests which supports all OS.

Review of attachment 543251 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay -- I was all tied up with the MozMill upgrade. The patch is mostly fine, but I'd like to look at it once more, so r- for now. (I promise a much quicker review next time!)

::: mail/test/mozmill/message-header/test-header-toolbar.js
@@ +14,5 @@
> + * The Original Code is Thunderbird Mail Client.
> + *
> + * The Initial Developer of the Original Code is
> + * the Mozilla Foundation.
> + * Portions created by the Initial Developer are Copyright (C) 2009

This should be 2011 I guess? And is the Mozilla Foundation correct?

@@ +51,5 @@
> +var controller = {};
> +Cu.import('resource://mozmill/modules/controller.js', controller);
> +
> +// The WindowHelper module
> +var WindowHelper;

You don't need this -- window-helpers now exports all the functions you need.

@@ +57,5 @@
> +var folder;
> +
> +const PREF = "toolbar.customization.usesheet";
> +var prefBranch = Cc["@mozilla.org/preferences-service;1"]
> +                    .getService(Ci.nsIPrefService).getBranch(null);

Please import resource://gre/modules/Services.jsm and use Services.prefs.

@@ +63,5 @@
> +function setupModule(module) {
> +  let fdh = collector.getModule('folder-display-helpers');
> +  fdh.installInto(module);
> +  WindowHelper = collector.getModule('window-helpers');
> +  WindowHelper.installInto(module);

Because you don't need this, you can change this to "let wh = collector..." etc.

@@ +69,5 @@
> +  abh.installInto(module);
> +  let meh = collector.getModule('mouse-event-helpers');
> +  meh.installInto(module);
> +
> +  folder = create_folder("MessageWindowB");

Please change the folder's name to HeaderToolbar or something similar.

@@ +73,5 @@
> +  folder = create_folder("MessageWindowB");
> +
> +  // create a message that has the interesting headers that commonly
> +  // show up in the message header pane for testing
> +  let msg = create_message({cc: msgGen.makeNamesAndAddresses(20), // YYY

What does the YYY stand for?

@@ +98,5 @@
> + */
> +function test_get_msg_button_customize_header_toolbar(){
> +  select_message_in_folder(0);
> +
> +  // It is necessary to press the Get Message Button to get the popup menu populated

This is a little ambiguous. I think you mean "It is necessary to open the Get Message button's menu to get the popup menu populated."

@@ +122,5 @@
> +    throw new Error("number of entries in Get Message Button popup menu after " +
> +                    "header toolbar customization " +
> +                    finalServerCount + " <> as before: " +
> +                    originalServerCount);
> +  }

You're using assert_true/assert_equals all over, so it'd be nice if you used them here as well.

@@ +184,5 @@
> +
> +  let ctc = open_header_pane_toolbar_customization(mc);
> +  let currentSet = toolbar.currentSet.split(",");
> +
> +  for (let i=1; i<currentSet.length; i++) {

Spaces around the binary operators please.

@@ +186,5 @@
> +  let currentSet = toolbar.currentSet.split(",");
> +
> +  for (let i=1; i<currentSet.length; i++) {
> +    let button1 = mc.e(currentSet[i]);
> +    let button2 = mc.e(currentSet[i-1]);

And here around the -.

@@ +188,5 @@
> +  for (let i=1; i<currentSet.length; i++) {
> +    let button1 = mc.e(currentSet[i]);
> +    let button2 = mc.e(currentSet[i-1]);
> +    // Move each button to the left of its left neighbour starting with
> +    // the second button, i.e. reverse the order of the buttons.

This comment confused me for a bit. You aren't moving each button to the left of its left neighbour right now, you're moving it to the left of its left neighbour in the original list. Please fix this somehow.

@@ +244,5 @@
> +  for (let i=1; i<currentSet.length; i++) {
> +    let button1 = msgc.e(currentSet[i]);
> +    let button2 = msgc.e(currentSet[i-1]);
> +    // Move each button to the left of its left neighbour starting with
> +    // the second button, i.e. reverse the order of the buttons.

The same comments as above apply here.

@@ +286,5 @@
> +
> +/**
> + *  Test header pane toolbar customization: Remove buttons
> + */
> +function test_customize_header_toolbar_remove_buttons(){

The brace should be on a new line in keeping with the other functions.

@@ +326,5 @@
> +  assert_equals(hdrToolbar.currentSet, hdrBarDefaultSet);
> +  assert_equals(hdrToolbar.getAttribute("currentset"), hdrBarDefaultSet);
> +  close_window(msgc);
> +
> +  // Check the persistency of the buttons.

probably better worded as "check button persistence"

@@ +363,5 @@
> +
> +/**
> + *  Test header pane toolbar customization dialog layout
> + */
> +function test_customize_header_toolbar_dialog_style(){

brace on new line

@@ +399,5 @@
> +
> +/**
> + *  Test header pane toolbar customization dialog for button style changes
> + */
> +function test_customize_header_toolbar_change_button_style(){

brace on newline

@@ +518,5 @@
> +  aCtc.click(aCtc.eid("donebutton"));
> +  // XXX There should be an equivalent for testing the closure of
> +  // XXX the dialog embedded in a sheet, but I do not know how.
> +  if (!prefBranch.getBoolPref(PREF, true)) {
> +   assert_true(aCtc.window.closed, "The customization dialog is not closed.");

fix the indentation please

@@ +532,5 @@
> +  let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
> +    getService(Ci.nsIWindowMediator);
> +  let mail3PaneWindow = windowMediator.getMostRecentWindow("mail:3pane");
> +  // close the 3pane window
> +  mail3PaneWindow.close();

What does this do that window-helpers' close_window doesn't?

@@ +537,5 @@
> +}
> +
> +function open3PaneWindow() {
> +  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"].
> +    getService(Ci.nsIWindowWatcher);

Services.ww.

@@ +548,5 @@
> +}
> +
> +function openAddressBook() {
> +  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"].
> +    getService(Ci.nsIWindowWatcher);

Services.ww.

::: mail/test/mozmill/shared-modules/test-mouse-event-helpers.js
@@ +45,5 @@
> +Cu.import('resource://mozmill/modules/mozmill.js', mozmill);
> +var EventUtils = {};
> +Cu.import('resource://mozmill/stdlib/EventUtils.js', EventUtils);
> +
> +const MODULE_NAME = 'mouse-event-helpers';

Do we still need this file now that we use MozMill 1.5.4 and bug 515776 has been fixed? I'm not clear what the relation between this and that is.

@@ +75,5 @@
> + *   the window the aDropObject is in
> + * @param {} aRelDropX
> + *   the relative x-position the element is dropped over the aDropObject
> + * @param {} aRelDropY
> + *   the relative y-position the element is dropped over the aDropObject

Please make it clear that these two are actually fractions and not pixels.

@@ +84,5 @@
> +                             aDropWindow, aRelDropX, aRelDropY, aListener)
> +{
> +  let dt = synthesize_drag_start(aDragWindow, aDragObject, aListener);
> +
> +  // Drop it onto the third tab ...

"the third tab"?

@@ +111,5 @@
> +  let dt;
> +
> +  var trapDrag = function(event) {
> +
> +    if ( !event.dataTransfer )

please remove the spaces inside the parentheses

@@ +116,5 @@
> +      throw "no DataTransfer";
> +
> +    dt = event.dataTransfer;
> +
> +    //event.stopPropagation();

commented code, please remove

@@ +167,5 @@
> +  EventUtils.synthesizeMouse(aDispatcher, 5, 5, {type:"mouseup"}, aWindow);
> +}
> +
> +/**
> + * Synthesizes a drop oevent.

event
Attachment #543251 - Flags: review?(sagarwal) → review-
I have created a new version of the patch incorporating your comments and have tested it on tryserver:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=14567a835784

I also had to change some other things, due to the mozmill upgrade. Especially in the function open_header_pane_toolbar_customization() the ewait("donebutton") call seems to not work anymore. So I replaced it by controller.sleep(1000). I thought I could do the same thing with a call to controller.waitForPageLoad(aController.eid("customizeToolbarSheetIFrame").node.contentDocument) but I received the error message that controller.waitForPageLoad is not a function. Any ideas?

(In reply to Siddharth Agarwal [:sid0] from comment #179)
> ::: mail/test/mozmill/message-header/test-header-toolbar.js
> @@ +14,5 @@
> > + * The Original Code is Thunderbird Mail Client.
> > + *
> > + * The Initial Developer of the Original Code is
> > + * the Mozilla Foundation.
> > + * Portions created by the Initial Developer are Copyright (C) 2009
> 
> This should be 2011 I guess? And is the Mozilla Foundation correct?
2011 corrected. What is the problem with Mozilla Foundation. The file contains code moved to it from other files of the Thunderbird mozmill tests.

> > +// The WindowHelper module
> > +var WindowHelper;
> 
> You don't need this -- window-helpers now exports all the functions you need.
Deleted.

> Please import resource://gre/modules/Services.jsm and use Services.prefs.
Changed.

> > +  WindowHelper = collector.getModule('window-helpers');
> > +  WindowHelper.installInto(module);
> 
> Because you don't need this, you can change this to "let wh = collector..."
> etc.
> 
> > +  folder = create_folder("MessageWindowB");
> 
> Please change the folder's name to HeaderToolbar or something similar.
Done.

> > +  let msg = create_message({cc: msgGen.makeNamesAndAddresses(20), // YYY
> 
> What does the YYY stand for?
No idea ;-) I copied it from another file (test-message-header.js)

> > +  // It is necessary to press the Get Message Button to get the popup menu populated
> 
> This is a little ambiguous. I think you mean "It is necessary to open the
> Get Message button's menu to get the popup menu populated."
> > +    throw new Error("number of entries in Get Message Button popup menu after " +
> You're using assert_true/assert_equals all over, so it'd be nice if you used
> them here as well.
> Spaces around the binary operators please.
Done.

> > +    // Move each button to the left of its left neighbour starting with
> > +    // the second button, i.e. reverse the order of the buttons.
> 
> This comment confused me for a bit. You aren't moving each button to the
> left of its left neighbour right now, you're moving it to the left of its
> left neighbour in the original list. Please fix this somehow.
I rephrased it.

> The brace should be on a new line in keeping with the other functions.
> > +  // Check the persistency of the buttons.
> 
> probably better worded as "check button persistence"
> > +  if (!prefBranch.getBoolPref(PREF, true)) {
> > +   assert_true(aCtc.window.closed, "The customization dialog is not closed.");
> 
> fix the indentation please
Done.

> > +  mail3PaneWindow.close();
> 
> What does this do that window-helpers' close_window doesn't?
Replaced with call to close_window().

> > +  let windowWatcher = Cc["@mozilla.org/embedcomp/window-watcher;1"].
> > +    getService(Ci.nsIWindowWatcher);
> 
> Services.ww.
Done.

> ::: mail/test/mozmill/shared-modules/test-mouse-event-helpers.js
> @@ +45,5 @@
> > +Cu.import('resource://mozmill/modules/mozmill.js', mozmill);
> > +var EventUtils = {};
> > +Cu.import('resource://mozmill/stdlib/EventUtils.js', EventUtils);
> > +
> > +const MODULE_NAME = 'mouse-event-helpers';
> 
> Do we still need this file now that we use MozMill 1.5.4 and bug 515776 has
> been fixed? I'm not clear what the relation between this and that is.
The function fixed in bug 515776 only drag'n'drops element in the same window (and the same controller). These functions work between different windows.

> > + * @param {} aRelDropX
> > + *   the relative x-position the element is dropped over the aDropObject
> > + * @param {} aRelDropY
> > + *   the relative y-position the element is dropped over the aDropObject
> 
> Please make it clear that these two are actually fractions and not pixels.
I rephrased it.

> > +  // Drop it onto the third tab ...
> 
> "the third tab"?
Removed (hangover of copy/paste).

> > +    if ( !event.dataTransfer )
> 
> please remove the spaces inside the parentheses
> > +    //event.stopPropagation();
> 
> commented code, please remove
> > +/**
> > + * Synthesizes a drop oevent.
> 
> event
Done.
See comment #180.
Attachment #543251 - Attachment is obsolete: true
Attachment #554761 - Flags: review?(sagarwal)
Comment on attachment 554761 [details] [diff] [review]
Mozmill tests including the comments of the review of :sid0

Review of attachment 554761 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/test/mozmill/message-header/test-header-toolbar.js
@@ +53,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +var folder;
> +
> +const PREF = "toolbar.customization.usesheet";

Could you rename this to something like USESHEET_PREF?

@@ +101,5 @@
> +  mc.click(mc.aid("button-getmsg", {class: "toolbarbutton-menubutton-dropmarker"}));
> +  mc.ewait("button-getAllNewMsgSeparator");
> +
> +  var getMailButtonPopup = mc.eid("button-getMsgPopup").node;
> +  var originalServerCount = getMailButtonPopup.childElementCount;

Since this is new code, please use let everywhere.

@@ +145,5 @@
> +  // buttons are shown there.
> +  let msgc = open_selected_message_in_new_window();
> +  assert_selected_and_displayed(msgc, curMessage);
> +  let hdrToolbar = msgc.eid("header-view-toolbar").node;
> +  let hdrBarDefaultSet = hdrToolbar.getAttribute("defaultset");

Note: it might be illegal to have multiple let declarations for the same variable within the same scope, even though we accept them right now. I'll try finding out today.

@@ +501,5 @@
> +  // either a normal window or embedded into a sheet.
> +  if (Services.prefs.getBoolPref(PREF, true)) {
> +    // XXX Sleep so the dialog has a chance to load. It seems that
> +    // ewait("donebutton") does not work after the update to mozmill 1.5.4b4.
> +    controller.sleep(1000);

This error's really strange. I spent some time looking into the failure but couldn't really get anywhere -- I'll try looking further at it later. Could you please file a followup bug?
Attachment #554761 - Flags: review?(sagarwal) → review+
Attachment #554761 - Attachment is obsolete: true
Attachment #554940 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n: comm-central] of latest mozmill tests patch
I think you've fixed the let redeclarations already? I had a discussion in #jsapi and I was going to ask you to remove them. Thanks.
Why is this CLOSED  and not assigned if you guys are still reviewing patches ? are these test only patches ?
(In reply to Ludovic Hirlimann [:Usul] from comment #185)
> Why is this CLOSED  and not assigned if you guys are still reviewing patches
> ? are these test only patches ?
The functionality requested by this bug was implemented some time ago. But the requested mozmill tests were still missing (see comment 143 and comment 174).

No I have created them and they are ready to be included into comm-central. 

I wanted to change to status of the bug to reopened but I cannot do this.
Checked in the unit tests:

http://hg.mozilla.org/comm-central/rev/72265fa31f49

Thanks for all your work on this Joachim, and for getting the tests complete.
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Whiteboard: [c-n: comm-central] of latest mozmill tests patch
Blocks: 717439
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: