509209 - Implement Customizable Toolbars in SeaMonkey Message Compose

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect)

Comment 1

[Philip Chee]

Attachment: 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]]

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

Comment 3

[Stefan [:stefanh]]

Attachment: editoricons-small.png for mac classic

Comment 4

[Stefan [:stefanh]]

Attachment: 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]

Comment on attachment 393370 [details] [diff] [review]
Patch v1.0

Cancelling review due to bit-rot

Comment 6

[Philip Chee]

Attachment: 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]

Attachment: 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]]

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

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

Comment 10

[Stefan [:stefanh]]

Attachment: 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]]

Attachment: 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]]

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

Comment 13

[Philip Chee]

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

Attachment: 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]]

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

Attachment: 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]]

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

Attachment: 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]

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

>> +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)]

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]

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

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

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

Attachment: 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]]

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]

I'll see what Mnyromyr has to say first.

Comment 28

[Stefan [:stefanh]]

Comment on attachment 397613 [details] [diff] [review]
Patch v1.4 fix more nits.

minusing because of the hiding.

Comment 29

[Stefan [:stefanh]]

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

Attachment: 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]]

Comment on attachment 397856 [details] [diff] [review]
Patch v1.5 fix UI nits.

Much better, thanks

Comment 32

[Philip Chee]

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

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]

Attachment: 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

[[e-mail]]

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]

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

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]

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

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]

Attachment: 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

[[e-mail]]

(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

[[e-mail]]

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]

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

Attachment: 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]

> 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

[[e-mail]]

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]

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]

Attachment: 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]]

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]

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

Comment 51

[Stefan [:stefanh]]

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]

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

Comment 53

[Karsten Düsterloh]

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]

Attachment: [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]]

http://hg.mozilla.org/comm-central/rev/00f6b1b884fc

Comment 56

[[e-mail]]

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