Implement Customizable Toolbars in SeaMonkey Message Compose

RESOLVED FIXED in seamonkey2.0

Status

SeaMonkey
MailNews: Composition
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {fixed-seamonkey2.0})

Trunk
seamonkey2.0
fixed-seamonkey2.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch for checkin in comment 54)

Attachments

(4 attachments, 13 obsolete attachments)

31.96 KB, image/png
Details
10.96 KB, image/png
Details
70.21 KB, image/png
Details
202.01 KB, patch
Philip Chee
: review+
Philip Chee
: superreview+
Robert Kaiser
: approval-seamonkey2.0+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

8 years ago
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.
Attachment #393370 - Flags: review?(mnyromyr)

Comment 2

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

Comment 3

8 years ago
Created attachment 394537 [details]
editoricons-small.png for mac classic

Comment 4

8 years ago
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
(Assignee)

Comment 5

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

Cancelling review due to bit-rot
Attachment #393370 - Flags: review?(mnyromyr)
(Assignee)

Comment 6

8 years ago
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?
Attachment #394538 - Attachment is obsolete: true
Attachment #395018 - Flags: review?(stefanh)
(Assignee)

Comment 7

8 years ago
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.
Attachment #393370 - Attachment is obsolete: true
Attachment #395019 - Flags: review?(mnyromyr)

Comment 8

8 years ago
(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?
(Assignee)

Comment 9

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

Comment 10

8 years ago
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.

Updated

8 years ago
Attachment #395018 - Flags: review?(stefanh)

Comment 11

8 years ago
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

8 years ago
Actually, it's not about size/position, it's just that the icons differs a bit in color/blur.
(Assignee)

Comment 13

8 years ago
> 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.
(Assignee)

Comment 14

8 years ago
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.
Attachment #395018 - Attachment is obsolete: true
Attachment #395019 - Attachment is obsolete: true
Attachment #395115 - Attachment is obsolete: true
Attachment #395804 - Flags: review?(mnyromyr)
Attachment #395019 - Flags: review?(mnyromyr)

Comment 15

8 years ago
+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.
(Assignee)

Comment 16

8 years ago
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;
Attachment #395804 - Attachment is obsolete: true
Attachment #397534 - Flags: superreview?(neil)
Attachment #397534 - Flags: review?(mnyromyr)
Attachment #395804 - Flags: review?(mnyromyr)

Comment 17

8 years ago
(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

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #397534 - Flags: superreview?(neil)
Attachment #397534 - Flags: review?(mnyromyr)
(Assignee)

Comment 19

8 years ago
> 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.
(Assignee)

Comment 20

8 years ago
>> +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().
(Assignee)

Comment 22

8 years ago
> * 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

8 years ago
(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

8 years ago
> 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.
(Assignee)

Comment 25

8 years ago
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();
Attachment #397534 - Attachment is obsolete: true
Attachment #397613 - Flags: ui-review?(stefanh)
(Assignee)

Updated

8 years ago
Attachment #397613 - Flags: review?(mnyromyr)

Comment 26

8 years ago
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.
(Assignee)

Comment 27

8 years ago
I'll see what Mnyromyr has to say first.

Updated

8 years ago
Attachment #397613 - Flags: ui-review?(stefanh) → ui-review-

Comment 28

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

minusing because of the hiding.

Comment 29

8 years ago
(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.
(Assignee)

Comment 30

8 years ago
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.
Attachment #397613 - Attachment is obsolete: true
Attachment #397856 - Flags: ui-review?
Attachment #397856 - Flags: review?(mnyromyr)
Attachment #397613 - Flags: review?(mnyromyr)
(Assignee)

Updated

8 years ago
Attachment #397856 - Flags: ui-review? → ui-review?(stefanh)
(Assignee)

Updated

8 years ago
Blocks: 513913

Comment 31

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

Much better, thanks
Attachment #397856 - Flags: ui-review?(stefanh) → ui-review+
(Assignee)

Comment 32

8 years ago
>> 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

8 years ago
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.
(Assignee)

Comment 34

8 years ago
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.
Attachment #397856 - Attachment is obsolete: true
Attachment #398318 - Flags: superreview?(neil)
Attachment #398318 - Flags: review?(mnyromyr)
Attachment #397856 - Flags: review?(mnyromyr)
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.
Attachment #398318 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 36

8 years ago
>>+  </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.

Updated

8 years ago
Attachment #398318 - Flags: review?(mnyromyr) → review-

Comment 37

8 years ago
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.
(Assignee)

Comment 38

8 years ago
>+      <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

8 years ago
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.
(Assignee)

Comment 40

8 years ago
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.
Attachment #398318 - Attachment is obsolete: true
Attachment #398947 - Flags: superreview?(neil)
Attachment #398947 - Flags: review?(mnyromyr)
(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.
(Assignee)

Comment 43

8 years ago
>>+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.
(Assignee)

Comment 44

8 years ago
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.
Attachment #398947 - Attachment is obsolete: true
Attachment #399146 - Flags: superreview?(neil)
Attachment #399146 - Flags: review?(mnyromyr)
Attachment #398947 - Flags: superreview?(neil)
Attachment #398947 - Flags: review?(mnyromyr)
(Assignee)

Updated

8 years ago
Depends on: 515105
(Assignee)

Comment 45

8 years ago
> 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.
Attachment #399146 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 47

8 years ago
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?
(Assignee)

Comment 48

8 years ago
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.
Attachment #399146 - Attachment is obsolete: true
Attachment #400362 - Flags: superreview+
Attachment #400362 - Flags: review?(mnyromyr)
Attachment #399146 - Flags: review?(mnyromyr)

Comment 49

8 years ago
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.
(Assignee)

Comment 50

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

Comment 51

8 years ago
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).
(Assignee)

Comment 52

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

Comment 53

8 years ago
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.
Attachment #400362 - Flags: review?(mnyromyr) → review+
(Assignee)

Comment 54

8 years ago
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!
Attachment #400362 - Attachment is obsolete: true
Attachment #401798 - Flags: superreview+
Attachment #401798 - Flags: review+
Attachment #401798 - Flags: approval-seamonkey2.0?

Updated

8 years ago
Attachment #401798 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
(Assignee)

Updated

8 years ago
Attachment #401798 - Attachment description: Patch v1.9 r=mnyromyr sr=neil → [for checkin] Patch v1.9 r=mnyromyr sr=neil
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [patch for checkin in comment 54
http://hg.mozilla.org/comm-central/rev/00f6b1b884fc
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed → fixed-seamonkey2.0
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
(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.
Blocks: 613435
Depends on: 613443
Blocks: 613443
No longer depends on: 613443
You need to log in before you can comment on or make changes to this bug.