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.

JFTR, I have editoricons-small.png and smimeicons-small.png ready for mac classic somewhere on my HD.

There are only 3 states for those icons: 1) Normal 2) active:hover 3) Disabled

Comment on attachment 393370 [details] [diff] [review] Patch v1.0 Cancelling review due to bit-rot

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

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.

(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?

> I'll look into it. Why did you asked me to review your icon? To get your attention. :D

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.

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

Actually, it's not about size/position, it's just that the icons differs a bit in color/blur.

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

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.

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

> +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;

(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?

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.

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

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

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

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

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

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

[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();

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.

I'll see what Mnyromyr has to say first.

Comment on attachment 397613 [details] [diff] [review] Patch v1.4 fix more nits. minusing because of the hiding.

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

> 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 on attachment 397856 [details] [diff] [review] Patch v1.5 fix UI nits. Much better, thanks

>> That said, the user-input stuff could surely be fixed in a follow-up. > OK, pushing this to a followup. Filed Bug 513913

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.

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

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

>+ <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?

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.

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

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

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

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

> For the colour buttons, file a followup bug, since they are currently active > wherever the focus is, which is incorrect. Filed Bug 515105.

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.

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?

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 on attachment 400362 [details] [diff] [review] Patch v1.8 Is the formatting toolbar supposed to be customizable? Because with this patch it's not.

> Is the formatting toolbar supposed to be customizable? No it isn't. Neither is it customizable in Thunderbird.

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

That's the ultimate intention but it's too complicated to be part of this patch.

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.

> (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!

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