Last Comment Bug 509209 - Implement Customizable Toolbars in SeaMonkey Message Compose
: Implement Customizable Toolbars in SeaMonkey Message Compose
Status: RESOLVED FIXED
[patch for checkin in comment 54
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Composition (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0
Assigned To: Philip Chee
:
:
Mentors:
Depends on: 515105 413385
Blocks: 513913 613435 613443
  Show dependency treegraph
 
Reported: 2009-08-08 08:27 PDT by Philip Chee
Modified: 2011-01-10 09:42 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 (132.84 KB, patch)
2009-08-08 09:09 PDT, Philip Chee
no flags Details | Diff | Splinter Review
editoricons-small.png for mac classic (31.96 KB, image/png)
2009-08-14 11:20 PDT, Stefan [:stefanh] (away until December 6)
no flags Details
smimeicons-small.png for mac classic (16.68 KB, image/png)
2009-08-14 11:21 PDT, Stefan [:stefanh] (away until December 6)
no flags Details
Shrink down smimeicons-small.png (12.42 KB, image/png)
2009-08-18 01:31 PDT, Philip Chee
no flags Details
Patch v1.1 Now with small Mac icons and css. (201.12 KB, patch)
2009-08-18 01:36 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Slightly better smimeicons-small.png (10.96 KB, image/png)
2009-08-18 11:51 PDT, Stefan [:stefanh] (away until December 6)
no flags Details
smimeicons-small.png made from non-mac version (10.45 KB, image/png)
2009-08-18 12:19 PDT, Stefan [:stefanh] (away until December 6)
no flags Details
Patch v1.2 with better smimeicons-small.png (199.22 KB, patch)
2009-08-21 03:31 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.3 Fixed Stefans nits. (201.84 KB, patch)
2009-08-30 09:38 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Screenshot (70.21 KB, image/png)
2009-08-30 11:22 PDT, Stefan [:stefanh] (away until December 6)
no flags Details
Patch v1.4 fix more nits. (198.66 KB, patch)
2009-08-31 06:56 PDT, Philip Chee
stefanh: ui‑review-
Details | Diff | Splinter Review
Patch v1.5 fix UI nits. (198.44 KB, patch)
2009-09-01 07:41 PDT, Philip Chee
stefanh: ui‑review+
Details | Diff | Splinter Review
Patch v1.5a inc relevant bits from Bug 512548 (198.64 KB, patch)
2009-09-02 21:08 PDT, Philip Chee
mnyromyr: review-
neil: superreview-
Details | Diff | Splinter Review
Patch v1.6 Fixed Review Nits. (203.26 KB, patch)
2009-09-06 10:40 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Patch v1.7 (201.55 KB, patch)
2009-09-07 19:49 PDT, Philip Chee
neil: superreview+
Details | Diff | Splinter Review
Patch v1.8 (201.74 KB, patch)
2009-09-13 09:18 PDT, Philip Chee
mnyromyr: review+
philip.chee: superreview+
Details | Diff | Splinter Review
[for checkin] Patch v1.9 r=mnyromyr sr=neil (202.01 KB, patch)
2009-09-21 01:20 PDT, Philip Chee
philip.chee: review+
philip.chee: superreview+
kairo: approval‑seamonkey2.0+
Details | Diff | Splinter Review

Description Philip Chee 2009-08-08 08:27:53 PDT

    
Comment 1 Philip Chee 2009-08-08 09:09:20 PDT
Created attachment 393370 [details] [diff] [review]
Patch v1.0

Patch for first review.

1. For classic I took the existing editoricons.png and smimeicons.png and shrunk them down using Paintshop Pro. I *think* I managed to perserve the alpha transparency.

2. There are no small icons (in this patch) for Modern since the existing graphics are GIFs with no transparency whatsoever and are designed so strangely that simply shrinking them won't produce suitable small icons.
Comment 2 Stefan [:stefanh] (away until December 6) 2009-08-14 11:09:28 PDT
JFTR, I have editoricons-small.png and smimeicons-small.png ready for mac classic somewhere on my HD.
Comment 3 Stefan [:stefanh] (away until December 6) 2009-08-14 11:20:07 PDT
Created attachment 394537 [details]
editoricons-small.png for mac classic
Comment 4 Stefan [:stefanh] (away until December 6) 2009-08-14 11:21:44 PDT
Created attachment 394538 [details]
smimeicons-small.png for mac classic

There are only 3 states for those icons:
1) Normal
2) active:hover
3) Disabled
Comment 5 Philip Chee 2009-08-17 20:25:11 PDT
Comment on attachment 393370 [details] [diff] [review]
Patch v1.0

Cancelling review due to bit-rot
Comment 6 Philip Chee 2009-08-18 01:31:44 PDT
Created attachment 395018 [details]
Shrink down smimeicons-small.png 

> Created an attachment (id=394538)
> smimeicons-small.png for mac classic

This appears to be identical to smimeicons.png for mac classic. I've run it through an ancient version of Paintshop Pro I have installed but it's kind of blur. Can you upload a better version of smimeicons-small.png?
Comment 7 Philip Chee 2009-08-18 01:36:07 PDT
Created attachment 395019 [details] [diff] [review]
Patch v1.1 Now with small Mac icons and css.

Changes in new Patch:

1. Stefan has provided some mac icons for the Compose buttons in small mode. I've added these to the patch and updated the mac classic css.

2. Additional css rules to prevent small mode icons getting big when in Customize mode.
Comment 8 Stefan [:stefanh] (away until December 6) 2009-08-18 04:14:29 PDT
(In reply to comment #6)
> Created an attachment (id=395018) [details]
> Shrink down smimeicons-small.png 
> 
> > Created an attachment (id=394538) [details]
> > smimeicons-small.png for mac classic
> 
> This appears to be identical to smimeicons.png for mac classic. I've run it
> through an ancient version of Paintshop Pro I have installed but it's kind of
> blur. Can you upload a better version of smimeicons-small.png?
I'll look into it. Why did you asked me to review your icon?
Comment 9 Philip Chee 2009-08-18 04:27:32 PDT
> I'll look into it. Why did you asked me to review your icon?
To get your attention. :D
Comment 10 Stefan [:stefanh] (away until December 6) 2009-08-18 11:51:51 PDT
Created attachment 395108 [details]
Slightly better smimeicons-small.png

Not so much better than yours, I think - if your smimeicons-small for non-mac is better, I can do a mac version of that.
Comment 11 Stefan [:stefanh] (away until December 6) 2009-08-18 12:19:33 PDT
Created attachment 395115 [details]
smimeicons-small.png made from non-mac version

OK, here's one made from your non-mac version. Not really sure which one is best. Mine is a bit sharper, but that hits the quality in some aspects. I also think that we've used different algorithms in some way, because if you look at the 2 icons side-by-side, you'll notice a slight difference in position (size?).
Comment 12 Stefan [:stefanh] (away until December 6) 2009-08-18 12:22:12 PDT
Actually, it's not about size/position, it's just that the icons differs a bit in color/blur.
Comment 13 Philip Chee 2009-08-19 20:35:18 PDT
> Actually, it's not about size/position, it's just that the icons differs a bit
> in color/blur.

I don't have a Mac. Could you just pick which one looks better on a Mac? Thanks.
Comment 14 Philip Chee 2009-08-21 03:31:05 PDT
Created attachment 395804 [details] [diff] [review]
Patch v1.2 with better smimeicons-small.png

Fixed bit-rot!

(In reply to comment #10 From  Stefan)
> Created an attachment (id=395108)
> Slightly better smimeicons-small.png

This one looks sharper than mine or the one you made from the non-mac smimeicons, so I'm going with this one.
Comment 15 Stefan [:stefanh] (away until December 6) 2009-08-29 04:55:54 PDT
+toolbar[iconsize="small"] > toolbarpaletteitem > #button-send[disabled="true"]

We just talked about it on irc, I'm just posting it here so it doesn't get lost: None of those style rules will work since the disabled attribute is removed by the  .js. I think the only thing you want is the "normal" state.
Comment 16 Philip Chee 2009-08-30 09:38:05 PDT
Created attachment 397534 [details] [diff] [review]
Patch v1.3 Fixed Stefans nits.

> +toolbar[iconsize="small"] > toolbarpaletteitem > #button-send[disabled="true"]

> We just talked about it on irc, I'm just posting it here so it doesn't get
> lost: None of those style rules will work since the disabled attribute is
> removed by the  .js. I think the only thing you want is the "normal" state.

I decided to do the following instead:

-#button-send[disabled="true"] {
+#button-send[disabled="true"],
+toolbarpaletteitem[itemdisabled] > #button-send {
   -moz-image-region: rect(330px 89px 359px 60px) !important;

+toolbar[iconsize="small"] > #button-send[disabled="true"],
+toolbar[iconsize="small"] > toolbarpaletteitem[itemdisabled] > #button-send {
+  -moz-image-region: rect(220px 59px 239px 40px) !important;
Comment 17 Stefan [:stefanh] (away until December 6) 2009-08-30 11:08:37 PDT
(In reply to comment #16)
 
> I decided to do the following instead:
> 
> -#button-send[disabled="true"] {
> +#button-send[disabled="true"],
> +toolbarpaletteitem[itemdisabled] > #button-send {
>    -moz-image-region: rect(330px 89px 359px 60px) !important;

Since the toolbarbutton isn't disabled and you're only switching icon, the label text won't look disabled - see toolkit's toolbarbutton.css (mac has "-moz-mac-disabledtoolbartext" and windows has "GrayText").

Also, have you tried and seen that it works with hover:active/:hover on toolbarbuttons on the toolbar when your customizing (it doesn't work for me)? Isn't there some user-input none somewhere?
Comment 18 Stefan [:stefanh] (away until December 6) 2009-08-30 11:22:36 PDT
Created attachment 397540 [details]
Screenshot

I've tried the patch, and I think I've found some issues:

1) The sheet doesn't cover the addressingtoolbar, I think it should
2) Because of 1) I can type in the addressfield and attach files etc
3) If I close the window without dismissing the sheet and then open it again (new message), the sheet is there.
Comment 19 Philip Chee 2009-08-30 20:55:55 PDT
> 1) The sheet doesn't cover the addressingtoolbar, I think it should
> 2) Because of 1) I can type in the addressfield and attach files etc

Thunderbird does (1) by putting the address and subject widget bars in a separate toolbox. However you can still type in the address fields etc in the parts not covered by the panel/sheet. I think we should hide these on entering Customize. I'll do this in a new patch.

> 3) If I close the window without dismissing the sheet and then open it again
> (new message), the sheet is there.

The same problem happens in Shredder. Setting |mail.compose.max_recycled_windows| to zero makes this problem go away. I guess this is because the compose window isn't actually closed, only hidden. CC philor. Perhaps he has some ideas on this.
Comment 20 Philip Chee 2009-08-30 21:29:40 PDT
>> +toolbarpaletteitem[itemdisabled] > #button-send {
>>    -moz-image-region: rect(330px 89px 359px 60px) !important;

> Since the toolbarbutton isn't disabled and you're only switching icon, the
> label text won't look disabled - see toolkit's toolbarbutton.css (mac has
> "-moz-mac-disabledtoolbartext" and windows has "GrayText").

> Also, have you tried and seen that it works with hover:active/:hover on
> toolbarbuttons on the toolbar when your customizing (it doesn't work for me)?
> Isn't there some user-input none somewhere?

I think all these are unnecessary optimizations.

toolbar[iconsize="small"] > toolbarpaletteitem > #button-security:hover {
toolbar[iconsize="small"] > toolbarpaletteitem > #button-security:hover:active {
toolbar[iconsize="small"] > toolbarpaletteitem[itemdisabled] > #button-security {

I propose to remove them all and just let the buttons have the default look during customize. What do you think.
Comment 21 Phil Ringnalda (:philor) 2009-08-30 21:58:57 PDT
Excellent options for closing a compose window without closing the customize sheet, that are not available to us:

* imitate native OS X customization, by disabling any option to close the window without first closing the customize panel

* remove the abomination of recycled_windows

Probably tolerable option:

* check whether customizeToolbarSheetIFrame.hidden == true in gComposeRecyclingListener.onClose(), and if not call MailToolboxCustomizeDone().
Comment 22 Philip Chee 2009-08-30 22:27:37 PDT
> * check whether customizeToolbarSheetIFrame.hidden == true in
> gComposeRecyclingListener.onClose(), and if not call
> MailToolboxCustomizeDone().

I thought of that but this would mean that customizeToolbars.js would leak listeners.
Comment 23 Stefan [:stefanh] (away until December 6) 2009-08-31 05:29:27 PDT
(In reply to comment #20)

> I think all these are unnecessary optimizations.
> 
> toolbar[iconsize="small"] > toolbarpaletteitem > #button-security:hover {
> toolbar[iconsize="small"] > toolbarpaletteitem > #button-security:hover:active
> {
> toolbar[iconsize="small"] > toolbarpaletteitem[itemdisabled] > #button-security
> {
> 
> I propose to remove them all and just let the buttons have the default look
> during customize. What do you think.

Yes, I agreee. That's also how it's done elsewhere.
Comment 24 Stefan [:stefanh] (away until December 6) 2009-08-31 05:30:12 PDT
> Yes, I agreee. That's also how it's done elsewhere.

I have a bug somewhere where I remove the redundant rules in navigator.css.
Comment 25 Philip Chee 2009-08-31 06:56:28 PDT
Created attachment 397613 [details] [diff] [review]
Patch v1.4 fix more nits.

[stefanh]
>> I propose to remove them all and just let the buttons have the default look
>> during customize. What do you think.
> Yes, I agreee. That's also how it's done elsewhere.
Fixed.

[philor]
> * check whether customizeToolbarSheetIFrame.hidden == true in
> gComposeRecyclingListener.onClose(), and if not call
> MailToolboxCustomizeDone()

I did this instead so that customizeToolbars.js can tidy up and unwrap the toolbar buttons.

+    if (getMailToolbox().customizing && gCustomizeSheet)
+      document.getElementById("customizeToolbarSheetIFrame").contentWindow.finishToolbarCustomization();
Comment 26 Stefan [:stefanh] (away until December 6) 2009-08-31 11:15:06 PDT
I didn't had the time to read comment #19 until now, but I think the hiding look a bit weird and it's really not how you should do it on at least mac. Since we're faking a sheet that is document/window - modal I'd say one solution would be to block user-input and focusability on all items after the customizable toolbar.
Comment 27 Philip Chee 2009-08-31 11:21:16 PDT
I'll see what Mnyromyr has to say first.
Comment 28 Stefan [:stefanh] (away until December 6) 2009-08-31 11:30:16 PDT
Comment on attachment 397613 [details] [diff] [review]
Patch v1.4 fix more nits.

minusing because of the hiding.
Comment 29 Stefan [:stefanh] (away until December 6) 2009-08-31 11:33:44 PDT
(In reply to comment #28)
> (From update of attachment 397613 [details] [diff] [review])
> minusing because of the hiding.

That said, the user-input stuff could surely be fixed in a follow-up.
Comment 30 Philip Chee 2009-09-01 07:41:47 PDT
Created attachment 397856 [details] [diff] [review]
Patch v1.5 fix UI nits.

> I didn't had the time to read comment #19 until now, but I think the hiding
> look a bit weird and it's really not how you should do it on at least mac.
> Since we're faking a sheet that is document/window - modal I'd say one solution
> would be to block user-input and focusability on all items after the
> customizable toolbar.

OK, I've adapted the Thunderbird UI where the addressing widget toolbar is in a separate toolbox so the sheet overlaps it.

> That said, the user-input stuff could surely be fixed in a follow-up.

OK, pushing this to a followup.
Comment 31 Stefan [:stefanh] (away until December 6) 2009-09-01 10:54:47 PDT
Comment on attachment 397856 [details] [diff] [review]
Patch v1.5 fix UI nits.

Much better, thanks
Comment 32 Philip Chee 2009-09-01 11:11:49 PDT
>> That said, the user-input stuff could surely be fixed in a follow-up.
> OK, pushing this to a followup.

Filed Bug 513913
Comment 33 Stefan [:stefanh] (away until December 6) 2009-09-01 12:53:36 PDT
I forgot to say that either I will bitrot you in bug 512548 or you will bitrot me (mac parts). In bug 512548 (I also remove the unneeded rules for buttons in toolbarpaletteitems) I add style rules so the behaviour of toolbarbuttons of type menu/menu-button has active:hover styling when their menus are open. That is, #button[open] should have the same style as #button:hover:active.
Comment 34 Philip Chee 2009-09-02 21:08:48 PDT
Created attachment 398318 [details] [diff] [review]
Patch v1.5a inc relevant bits from Bug 512548

Bug 512548 Adds the "pressed" look for toolbarbuttons when the menu is open to Mac Classic. I've added the relevant css for small toolbar buttons.
Comment 35 neil@parkwaycc.co.uk 2009-09-03 05:41:17 PDT
Comment on attachment 398318 [details] [diff] [review]
Patch v1.5a inc relevant bits from Bug 512548

>-    return(element.getAttribute('moz-collapsed') == "true");
>+    return(element.getAttribute("collapsed") == "true");
return element.collapsed;

>+function MailToolboxCustomizeInit()
>+{
>+  toolboxCustomizeInit("mail-menubar");
>+}
>+
>+function MailToolboxCustomizeDone(aToolboxChanged)
>+{
>+  toolboxCustomizeDone("mail-menubar", getMailToolbox(), aToolboxChanged);
>+}
Consider calling Disable/EnableEditableFields.

>+  </toolbox>
>+
>+  <toolbox id="headers-box"
>+           class="toolbox-top"
>+           mode="icons">
There's an ugly join between these toolboxes. It also means that the formatting toolbar can't be customised. Also, try clicking on the grippies :-P

> .toolbarbutton-1 {
>-  list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>+  min-width: 0px;
???

>+/* ::::: small primary toolbar buttons ::::: */ /* XXXRatty ToDo */
>+
Please don't leave this in.
Comment 36 Philip Chee 2009-09-03 05:57:32 PDT
>>+  </toolbox>
>>+
>>+  <toolbox id="headers-box"
>>+           class="toolbox-top"
>>+           mode="icons">
> There's an ugly join between these toolboxes. It also means that the formatting
> toolbar can't be customised. Also, try clicking on the grippies :-P
Ugh. So How do I put the panel/sheet on top of the addressing toolbarwidget without having a split toolbox like Thunderbird?

>> .toolbarbutton-1 {
>>-  list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>>+  min-width: 0px;
> ???
I think you asked the same question when I customized navigator.
Comment 37 Karsten Düsterloh 2009-09-04 09:15:56 PDT
Comment on attachment 398318 [details] [diff] [review]
Patch v1.5a inc relevant bits from Bug 512548

Looks good, just some nits:

>diff --git a/mailnews/extensions/smime/content/msgCompSMIMEOverlay.xul b/mailnews/extensions/smime/content/msgCompSMIMEOverlay.xul
>+    <toolbarbutton id="button-security" type="menu-button"
>                    oncommand="doSecurityButton();" class="toolbarbutton-1"
>                    label="&securityButton.label;" tooltiptext="&securityButton.tooltip;">
...
>+    <menuitem id="menu_viewSecurityStatus" label="&menu_viewSecurityStatus.label;"
>       accesskey="&menu_viewSecurityStatus.accesskey;" observes="cmd_viewSecurityStatus"/>

While you are touching these, please reformat the attributes according to <https://wiki.mozilla.org/index.php?title=SeaMonkey:MailNews:CodingStyle#XML.2FXUL>.

>diff --git a/suite/mailnews/compose/MsgComposeCommands.js b/suite/mailnews/compose/MsgComposeCommands.js
>+    //Reset the Customize Toolbars panel/sheet if open.
>+    if (getMailToolbox().customizing && gCustomizeSheet)
>+      document.getElementById("customizeToolbarSheetIFrame").contentWindow.finishToolbarCustomization();

Please wrap.

>+    return(element.getAttribute("collapsed") == "true");

No outer braces needed.

>+function MailToolboxCustomizeChange(event)

aEvent

>diff --git a/suite/mailnews/compose/messengercompose.xul b/suite/mailnews/compose/messengercompose.xul
>+  <command id="cmd_CustomizeToolbars" oncommand="goCustomizeToolbar(getMailToolbox());"/>

(Just a sidenote: Underscore + uppercase is weird. Too bad no-one saw this when implementing the other toolbar stuff, so keep it to be consistent at least. :-|)

>+<popup id="toolbar-context-menu"/>
>+

Useless empty line.

>+    <toolbar type="menubar"
>+           id="compose-toolbar-menubar2"

Wrong indent and wrong attribute order (see above).

>+              <menuseparator/>

All menuseparators should have an id, so that addons can use insertafter etc.
This applies as well to all other new menuseparators I may have overlooked above and below. ;-) 

>+              <menuitem id="menu_inlineSpellCheck"
>+                        label="&enableInlineSpellChecker.label;"
>+                        accesskey="&enableInlineSpellChecker.accesskey;"
>+                        type="checkbox"
>+                        oncommand="EnableInlineSpellCheck(!InlineSpellCheckerUI.enabled);"/>

The placement of the type attribute here is inconsistent with your usage above (or vice versa).
Please use the order used here (i.e. type after label and keys) everywhere, and don't forget to give ids to *all* menuitems.

>+    <toolbarpalette id="MsgComposeToolbarPalette">
>+

Drop empty line here.

>+      <toolbarbutton id="button-send"
>+                     class="toolbarbutton-1"
>+                     label="&sendButton.label;"
>+                     tooltiptext="&sendButton.tooltip;"
>+                     command="cmd_sendButton"
>+                     now_label="&sendButton.label;"
>+                     now_tooltiptext="&sendButton.tooltip;"
>+                     later_label="&sendLaterCmd.label;"
>+                     later_tooltiptext="&sendlaterButton.tooltip;">

Please reorder attributes.
Comment 38 Philip Chee 2009-09-04 10:28:33 PDT
>+      <toolbarbutton id="button-send"
>+                     class="toolbarbutton-1"
>+                     label="&sendButton.label;"
>+                     tooltiptext="&sendButton.tooltip;"
>+                     command="cmd_sendButton"
>+                     now_label="&sendButton.label;"
>+                     now_tooltiptext="&sendButton.tooltip;"
>+                     later_label="&sendLaterCmd.label;"
>+                     later_tooltiptext="&sendlaterButton.tooltip;">

> Please reorder attributes.
What order do you want these attributes? Alphabetical?
Comment 39 Stefan [:stefanh] (away until December 6) 2009-09-06 04:28:42 PDT
I think it'd be strange if the formatting toolbar is customizable when there's a whole bunch of ui between the formatting toolbar and the "main" toolbar. There's really no indicator that tells the user what's customizable or not, except the position of the dialog/sheet. In order to be able to drag & drop from/to the formatting toolbar, you need the sheet to be positioned below the formatting toolbar, but then users would expect the addressing toolbar to be customizable.
Comment 40 Philip Chee 2009-09-06 10:40:27 PDT
Created attachment 398947 [details] [diff] [review]
Patch v1.6 Fixed Review Nits.

> >-    return(element.getAttribute('moz-collapsed') == "true");
> >+    return(element.getAttribute("collapsed") == "true");
> return element.collapsed;
Fixed.

> >+function MailToolboxCustomizeInit()
> >+function MailToolboxCustomizeDone(aToolboxChanged)
> Consider calling Disable/EnableEditableFields.
Fixed.

> >+  </toolbox>
> >+
> >+  <toolbox id="headers-box"
> >+           class="toolbox-top"
> >+           mode="icons">
> There's an ugly join between these toolboxes. It also means that the formatting
> toolbar can't be customised. Also, try clicking on the grippies :-P
Reverted to using a single toolbox. Extended goCustomizeToolbar() to take an additional "anchor" parameter.

> > .toolbarbutton-1 {
> >-  list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
> >+  min-width: 0px;
> ???
The modern communicator/button.css sets .toolbar-button a min-width: 50px;
 
> >+/* ::::: small primary toolbar buttons ::::: */ /* XXXRatty ToDo */
> >+
> Please don't leave this in.
Fixed.

> >+    <toolbarbutton id="button-security" type="menu-button"
> >+    <menuitem id="menu_viewSecurityStatus" label="&menu_viewSecurityStatus.label;"
> While you are touching these, please reformat the attributes according to
> <https://wiki.mozilla.org/index.php?title=SeaMonkey:MailNews:CodingStyle#XML.2FXUL>.
Fixed.

> >+      document.getElementById("customizeToolbarSheetIFrame").contentWindow.finishToolbarCustomization();
> Please wrap.
Fixed.

> >+    return(element.getAttribute("collapsed") == "true");
> No outer braces needed.
Fixed.

> >+function MailToolboxCustomizeChange(event)
> aEvent
Fixed.

> >+<popup id="toolbar-context-menu"/>
> >+
> Useless empty line.
Fixed.

> >+    <toolbar type="menubar"
> >+           id="compose-toolbar-menubar2"
> Wrong indent and wrong attribute order (see above).
Fixed.

> >+              <menuseparator/>
> All menuseparators should have an id, so that addons can use insertafter etc.
> This applies as well to all other new menuseparators I may have overlooked
> above and below. ;-) 
Fixed (for the parts that I touched).

> >+              <menuitem id="menu_inlineSpellCheck"
> >+                        label="&enableInlineSpellChecker.label;"
> >+                        accesskey="&enableInlineSpellChecker.accesskey;"
> >+                        type="checkbox"
> >+                        oncommand="EnableInlineSpellCheck(!InlineSpellCheckerUI.enabled);"/>
> 
> The placement of the type attribute here is inconsistent with your usage above
> (or vice versa).
> Please use the order used here (i.e. type after label and keys) everywhere, and

I have decided that it makes more sense for type to go before label and keys; and together with id and class. e.g.
1. identity/selectors { id, type, class }
2. text { label, key, accesskey, tooltip }
3. misc & custom
4. handlers { command, oncommand, on* }

> don't forget to give ids to *all* menuitems.
Fixed (for the parts that I touched).

> >+    <toolbarpalette id="MsgComposeToolbarPalette">
> >+
> Drop empty line here.
Fixed.

> >+      <toolbarbutton id="button-send"
> >+                     class="toolbarbutton-1"
> >+                     label="&sendButton.label;"
> >+                     tooltiptext="&sendButton.tooltip;"
> >+                     command="cmd_sendButton"
> >+                     now_label="&sendButton.label;"
> >+                     now_tooltiptext="&sendButton.tooltip;"
> >+                     later_label="&sendLaterCmd.label;"
> >+                     later_tooltiptext="&sendlaterButton.tooltip;">
> Please reorder attributes.
Fixed.
Comment 41 neil@parkwaycc.co.uk 2009-09-06 11:47:32 PDT
(In reply to comment #40)
> > > .toolbarbutton-1 {
> > >-  list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
> > >+  min-width: 0px;
> > ???
> The modern communicator/button.css sets .toolbar-button a min-width: 50px;
And we override it on a window-by-window basis, rather than somewhere common?
Comment 42 neil@parkwaycc.co.uk 2009-09-06 13:05:13 PDT
Comment on attachment 398947 [details] [diff] [review]
Patch v1.6 Fixed Review Nits.

A possible alternative would be to temporarily hide the addressing toolbar.
This would make customising the formatting toolbar feasible.

>+              observes="cmd_viewSecurityStatus"/>
Would prefer that this got changed to command= if that works.

>+            .openPopup(anchor ? anchor : toolbox,
anchor || toolbox (if we keep this code)

>+  document.getElementById("msgIdentity").setAttribute("disabled", true);
Add disableonsend="true" to the element instead. (After all, it should be.)

>+function WrapOnCustomize()
The problem with this is that the element becomes draggable.
For the listbox, adding disableonsend="true" should work.
For the colour buttons, file a followup bug, since they are currently active wherever the focus is, which is incorrect.

>-  var headersbox = document.getElementById("headers-box");
>+  var headersbox = document.getElementById("compose-toolbox");
See bug 514416.
Comment 43 Philip Chee 2009-09-07 00:30:00 PDT
>>+function WrapOnCustomize()
> The problem with this is that the element becomes draggable.
Hrm. Right.

> For the listbox, adding disableonsend="true" should work.
I tried to add disabled="true" manually via the DOMi, but the onclick, onkeypress, and the drag'n'drop handlers are still active.
Comment 44 Philip Chee 2009-09-07 19:49:52 PDT
Created attachment 399146 [details] [diff] [review]
Patch v1.7

> (In reply to comment #40)
>>>> .toolbarbutton-1 {
>>>>-  list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>>>>+  min-width: 0px;
>>> ???
>> The modern communicator/button.css sets .toolbar-button a min-width: 50px;
> And we override it on a window-by-window basis, rather than somewhere common?

I poked at this in more detail and I notice that the rule in button.css is basically useless since the toolbar button images are all 50px wide anyway. So I just removed this rule from button.css (Note to self: remove the overrides in navigator.css and primaryToolbar1.css in a followup bug).
 
> (From update of attachment 398947 [details] [diff] [review])
> A possible alternative would be to temporarily hide the addressing toolbar.
> This would make customising the formatting toolbar feasible.

Now using moz-collapsed for the addressing toolbar.
>>+              observes="cmd_viewSecurityStatus"/>
> Would prefer that this got changed to command= if that works.

Fixed.

>>+            .openPopup(anchor ? anchor : toolbox,
> anchor || toolbox (if we keep this code)

Noted and removed.

>>+  document.getElementById("msgIdentity").setAttribute("disabled", true);
> Add disableonsend="true" to the element instead. (After all, it should be.)
Fixed.

>>+function WrapOnCustomize()
> The problem with this is that the element becomes draggable.
> For the listbox, adding disableonsend="true" should work.

I tried to add disabled="true" in the DOMi but the onclick and the drag and drop handlers are still active. But since the whole addressing toolbar is collapsed anyway this is moot.

>>-  var headersbox = document.getElementById("headers-box");
>>+  var headersbox = document.getElementById("compose-toolbox");
> See bug 514416.
Removed.
Comment 45 Philip Chee 2009-09-08 20:44:01 PDT
> For the colour buttons, file a followup bug, since they are currently active
> wherever the focus is, which is incorrect.
Filed Bug 515105.
Comment 46 neil@parkwaycc.co.uk 2009-09-11 07:51:32 PDT
Comment on attachment 399146 [details] [diff] [review]
Patch v1.7

>+function MailToolboxCustomizeInit()
>+{
>+  disableEditableFields();
>+  document.getElementById("MsgHeadersToolbar").setAttribute("moz-collapsed", true);
>+  document.getElementById("content-frame").setAttribute("moz-collapsed", true);
>+  toolboxCustomizeInit("mail-menubar");
>+}
>+
>+function MailToolboxCustomizeDone(aToolboxChanged)
>+{
>+  toolboxCustomizeDone("mail-menubar", getMailToolbox(), aToolboxChanged);
>+  document.getElementById("MsgHeadersToolbar").removeAttribute("moz-collapsed");
>+  document.getElementById("content-frame").removeAttribute("moz-collapsed");
>+  enableEditableFields();
>+}
Nit: need to hide the compose-toolbar-sizer splitter too.

>-        <vbox flex="1" id="addresses-box">
>+        <vbox id="addresses-box" flex="1">
>           <hbox align="center">
>             <label value="&fromAddr.label;" accesskey="&fromAddr.accesskey;" control="msgIdentity"/>
>-            <menulist id="msgIdentity" type="description" flex="1" oncommand="LoadIdentity(false);">
>+            <menulist id="msgIdentity"
>+                      type="description"
>+                      flex="1"
>+                      disableonsend="true"
Not needed now we hide the whole toolbar.
Comment 47 Philip Chee 2009-09-13 05:37:44 PDT
I just came across another problem.
1. Focus on the content area.
2. Enter Customize...
3. Items in the Formatting Toolbar are still active.

I tried various random things including moving the focus somewhere else:

+function MailToolboxCustomizeInit()
+{
+  if (document.commandDispatcher.focusedWindow == content)
+    window.focus();
+  disableEditableFields();
+  document.getElementById("MsgHeadersToolbar").setAttribute("moz-collapsed", true);
+  document.getElementById("compose-toolbar-sizer").setAttribute("moz-collapsed", true);
+  document.getElementById("content-frame").setAttribute("moz-collapsed", true);
+  toolboxCustomizeInit("mail-menubar");
+}

Neil, is this acceptable?
Comment 48 Philip Chee 2009-09-13 09:18:28 PDT
Created attachment 400362 [details] [diff] [review]
Patch v1.8

Carrying forward sr=neil

>>+function MailToolboxCustomizeDone(aToolboxChanged)
>>+{
>>+  toolboxCustomizeDone("mail-menubar", getMailToolbox(), aToolboxChanged);
>>+  document.getElementById("MsgHeadersToolbar").removeAttribute("moz-collapsed");
>>+  document.getElementById("content-frame").removeAttribute("moz-collapsed");
>>+  enableEditableFields();
>>+}
> Nit: need to hide the compose-toolbar-sizer splitter too.
Fixed.

>>+            <menulist id="msgIdentity"
>>+                      type="description"
>>+                      flex="1"
>>+                      disableonsend="true"
> Not needed now we hide the whole toolbar.
Fixed.

Also fixed in this patch:
On CustomizeInit, focus on the window; On CustomizeDone, focus on the content.
Comment 49 Stefan [:stefanh] (away until December 6) 2009-09-13 10:06:58 PDT
Comment on attachment 400362 [details] [diff] [review]
Patch v1.8

Is the formatting toolbar supposed to be customizable? Because with this patch it's not.
Comment 50 Philip Chee 2009-09-13 18:04:44 PDT
> Is the formatting toolbar supposed to be customizable?
No it isn't. Neither is it customizable in Thunderbird.
Comment 51 Stefan [:stefanh] (away until December 6) 2009-09-14 00:51:59 PDT
Aha, OK. I'm not saying it should be customizable, but comment #35 gave me the impression that it might be (and the reason why the the addressing toolbar is hidden and the customize sheet appear below the formatting toolbar).
Comment 52 Philip Chee 2009-09-14 02:07:29 PDT
That's the ultimate intention but it's too complicated to be part of this patch.
Comment 53 Karsten Düsterloh 2009-09-19 11:48:13 PDT
Comment on attachment 400362 [details] [diff] [review]
Patch v1.8

> function IsMsgHeadersToolbarCollapsed()
> {
>   var element = GetMsgHeadersToolbarElement();
>   if(element)
>-    return(element.getAttribute('moz-collapsed') == "true");
>+    return element.collapsed;
> 
>   return(0);

You could just replace the whole "if..return(0);" by "return element && element.collapsed;".

>+function getMailToolbox()

Any particular reason for the leading lowercase g? If not, please fix.

>+              <menuitem id="menu_checkspelling"
>+                        label="&checkSpellingCmd2.label;"
>+                        accesskey="&checkSpellingCmd2.accesskey;"
>+                        key="key_checkspelling" command="cmd_spelling"/>

Move the command attribute onto the next line.

>+          <listbox id="attachmentBucket" seltype="multiple" flex="1" rows="4"

Since you're touching this, please move each attribute onto its own line.


r=me with that.

Sorry for the delay.
Comment 54 Philip Chee 2009-09-21 01:20:51 PDT
Created attachment 401798 [details] [diff] [review]
[for checkin] Patch v1.9 r=mnyromyr sr=neil

> (From update of attachment 400362 [details] [diff] [review])
> > function IsMsgHeadersToolbarCollapsed()
> > {
> >   var element = GetMsgHeadersToolbarElement();
> >   if(element)
> >-    return(element.getAttribute('moz-collapsed') == "true");
> >+    return element.collapsed;
> > 
> >   return(0);
> 
> You could just replace the whole "if..return(0);" by "return element &&
> element.collapsed;".
Fixed.

> >+function getMailToolbox()
> Any particular reason for the leading lowercase g? If not, please fix.

API compatibility with Thunderbird.

> >+              <menuitem id="menu_checkspelling"
> >+                        label="&checkSpellingCmd2.label;"
> >+                        accesskey="&checkSpellingCmd2.accesskey;"
> >+                        key="key_checkspelling" command="cmd_spelling"/>
> 
> Move the command attribute onto the next line.
Fixed.

> >+          <listbox id="attachmentBucket" seltype="multiple" flex="1" rows="4"
> 
> Since you're touching this, please move each attribute onto its own line.
Fixed.

> r=me with that.
Thanks!
Comment 55 Frank Wein [:mcsmurf] 2009-09-22 12:36:06 PDT
http://hg.mozilla.org/comm-central/rev/00f6b1b884fc
Comment 56 neil@parkwaycc.co.uk 2009-09-22 16:17:24 PDT
(In reply to comment #53)
> (From update of attachment 400362 [details] [diff] [review])
> >+          <listbox id="attachmentBucket" seltype="multiple" flex="1" rows="4"
> Since you're touching this, please move each attribute onto its own line.
Except you don't need to touch this any more now you're collapsing the toolbar.

Note You need to log in before you can comment on or make changes to this bug.