413385 - Implement Customizable Toolbars in SeaMonkey MailNews

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Robert Kaiser]

Bug 394288 is trying to add support for customizable toolbars to the SeaMonkey browser, the bug here is about achieving the same for the MailNews component.

Comment 1

[Philip Chee]

Attachment: Patch v1.0

This patch:

0. Turns on customizable toolbars in MailNews.
1. Unhides the Mark button so it is visible all the time.
2. Forces the Delete button visible when in customize mode.

Comment 2

[Philip Chee]

Attachment: diff-w of mailWindowOverlay.xul

Comment 3

[Karsten Düsterloh]

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

>diff --git a/mailnews/base/resources/content/mailWindowOverlay.js b/mailnews/base/resources/content/mailWindowOverlay.js
>+function getMailToolbox()

The vast majority of functions in this file starts with an uppercase letter. Please stick to that meme.
(I don't think that it's necessary to stick to TB's _function_ names, given the rapidly diverting UI implementation.)

>diff --git a/mailnews/base/resources/content/mailWindowOverlay.xul b/mailnews/base/resources/content/mailWindowOverlay.xul

>-<menubar id="mail-menubar" grippytooltiptext="&menuBar.tooltip;">
>+  <toolbar type="menubar"
>+           id="mail-toolbar-menubar2"

I don't particularily like using "mail-toolbar-menubar2" when there never even is/was a "mail-toolbar-menubar". But since we try to share potential overlay mount points with TB, it's at least tolerable.

>+  <toolbarpalette id="MailToolbarPalette">

When I open the Customize... dialog for the mail toolbar, I get three(!) Separator widgets, but only one of them is actually functional. The other two can be dragged, but are causing "JavaScript error: chrome://global/content/nsDragAndDrop.js, line 458: transferData.first is null") and will not be accepted by the toolbar.

>+            <menu uri="..."  class="folderMenuItem menu-iconic" label="rdf:http://home.netscape.com/NC-rdf#Name"
>+             SpecialFolder="rdf:http://home.netscape.com/NC-rdf#SpecialFolder"
>+             BiffState="rdf:http://home.netscape.com/NC-rdf#BiffState"
>+             IsServer="rdf:http://home.netscape.com/NC-rdf#IsServer"
>+             IsSecure="rdf:http://home.netscape.com/NC-rdf#IsSecure"
>+             ServerType="rdf:http://home.netscape.com/NC-rdf#ServerType">

Please align with the uri attribute, including additional instances below.

>+                <menuitem label="&fileHereMenu.label;" accesskey="&fileHereMenu.accesskey;"
>+                  oncommand="MsgMoveMessage(event.target.parentNode.parentNode)"/>

One attribute per line if they don't fit onto one line, please.

>diff --git a/suite/themes/modern/messenger/primaryToolbar.css b/suite/themes/modern/messenger/primaryToolbar.css
> .toolbarbutton-1 {
>   list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>+  min-width: 0px !important;
> }

Is this important really necessary?

r- basically for the dragging issue.

Comment 4

[Karsten Düsterloh]

This is with the Classic theme:

> When I open the Customize... dialog for the mail toolbar, I get three(!)
> Separator widgets, but only one of them is actually functional. The other two
> can be dragged, but are causing "JavaScript error:
> chrome://global/content/nsDragAndDrop.js, line 458: transferData.first is
> null") and will not be accepted by the toolbar.

With Modern, the broken two Separators don't have an icon, just text. No JS error is shown (maybe they're not really draggable due to lack of image?). But another issue manifests: there are no insertion marks when trying to drag stuff onto the toolbar...

Comment 5

[Philip Chee]

> When I open the Customize... dialog for the mail toolbar, I get three(!)
> Separator widgets, but only one of them is actually functional. The other two
> can be dragged, but are causing "JavaScript error:
> chrome://global/content/nsDragAndDrop.js, line 458: transferData.first is
> null") and will not be accepted by the toolbar.

This is a toolkit bug caused by someone called Neil in Bug 407725. For some reason he has forgotten to land the bustage fix in  Bug 475711 on 1.9.1.

>>+  min-width: 0px !important;
>> }
> Is this important really necessary?
It wasn't in Navigator but for some reason I had to use !important in messenger.

> With Modern ... there are no insertion marks when trying to drag stuff
onto the toolbar...
Hmm. Investigating.

Comment 6

[Philip Chee]

Attachment: DOMi CSS pane on mailnews toolbarbutton.

>>>+  min-width: 0px !important;
>>> }
>> Is this important really necessary?
> It wasn't in Navigator but for some reason I had to use !important in
> messenger.

Looking at it in DOMi first chrome://communicator/skin/button.css specifies:
.toolbarbutton-1 { min-width: 50px }

Then I specify in chrome://messenger/skin/primaryToolbar.css:
.toolbarbutton-1 {min-width: 0px }

Then after that chrome://communicator/skin/button.css is again loaded (twice!) putting the min-width back to 50px (since all these rules have equal weights, the last one loaded wins).

In Navigator buttons.css is only loaded once so I didn't have to use !important. I made sure I didn't have any extensions installed in the profile so this isn't caused by some extension so I guess two of the standard messenger overlays are also pulling in buttons.css but I'm not sure which.

Comment 7

[Philip Chee]

Attachment: Patch v1.1

>>+function getMailToolbox()
> 
> The vast majority of functions in this file starts with an uppercase letter.
> Please stick to that meme.

We agreed over IRC to keep the same case as Thunderbird for extension compatibility.

> When I open the Customize... dialog for the mail toolbar, I get three(!)
> Separator widgets, but only one of them is actually functional. The other two

Oops. This isn't a toolkit bug but too much (or not enough) cut and paste. And I went and blamed Neil! :S
Fixed.

>>+            <menu uri="..."  class="folderMenuItem menu-iconic" label="rdf:http://home.netscape.com/NC-rdf#Name"
>>+             SpecialFolder="rdf:http://home.netscape.com/NC-rdf#SpecialFolder"
> 
> Please align with the uri attribute, including additional instances below.
Fixed.

>>+                <menuitem label="&fileHereMenu.label;" accesskey="&fileHereMenu.accesskey;"
>>+                  oncommand="MsgMoveMessage(event.target.parentNode.parentNode)"/>
> 
> One attribute per line if they don't fit onto one line, please.
Fixed.

>> .toolbarbutton-1 {
>>   list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>>+  min-width: 0px !important;
>> }
> 
> Is this important really necessary?
See Comment 6

> But
> another issue manifests: there are no insertion marks when trying to drag stuff
> onto the toolbar...
Fixed.

Comment 8

[[e-mail]]

Comment on attachment 372426 [details] [diff] [review]
Patch v1.1

>+        gDeleteButton.setAttribute("hidden", true);
>+        gDeleteButton.removeAttribute("hidden");
[These could be switched to gDeleteButton.hidden = forNews;]

>+  UpdateMailToolbar(); // XXXRatty: do we need this?
No, you shouldn't need it.

>+           id="mail-toolbar-menubar2"
What's with this id?

>+      <menubar id="mail-menubar"/>
Heh, it's a bit of a cheat ;-)

>+  min-width: 0px !important;
Both msgReadSMIMEOverlay and msgHdrViewOverlay re-import messenger.css so file a bug and note it here too.

>+/* ::::: primary separator style rules ::::: */
>+
>+.toolbar-primary > toolbarseparator {
>+  margin: 0px;
>+  padding: 0px;
>+  border: none;
>+  width: 18px;
>+  background: none;
>+}
Modern doesn't seem to have primary toolbarseparators in any window, so this belongs somewhere global. (I also noticed that toolbarspacers take a different width when customising, which is really a separate bug.)

Comment 9

[Philip Chee]

>>+           id="mail-toolbar-menubar2"
> What's with this id?
>>+      <menubar id="mail-menubar"/>
> Heh, it's a bit of a cheat ;-)

Yeah but we want to maintain the same IDs and overlay points with Thunderbird as much as possible for extension compatibility e.g. Lightning.

>>+/* ::::: primary separator style rules ::::: */
>>+
>>+.toolbar-primary > toolbarseparator {
>>+  margin: 0px;
>>+  padding: 0px;
>>+  border: none;
>>+  width: 18px;
>>+  background: none;
>>+}
> Modern doesn't seem to have primary toolbarseparators in any window, so this
> belongs somewhere global.

I have to disagree with you. In Navigator you can drag normal looking (i.e. visible) separators on to a toolbar. I think that the reason MailNews has *different* invisible looking separators is that the mailnews primary buttons have this strange left/right borders on hover effect which is ruined with visible separators. In any case I want normal looking separators in Navigators at least.

> (I also noticed that toolbarspacers take a different
> width when customising, which is really a separate bug.)

I think that this is because the two separators change width in customize mode. I'll have a look to see what can be done.

Comment 10

[[e-mail]]

(In reply to comment #9)
>>>+/* ::::: primary separator style rules ::::: */
>>>+
>>>+.toolbar-primary > toolbarseparator {
>>>+  margin: 0px;
>>>+  padding: 0px;
>>>+  border: none;
>>>+  width: 18px;
>>>+  background: none;
>>>+}
>>Modern doesn't seem to have primary toolbarseparators in any window, so this
>>belongs somewhere global.
>I have to disagree with you. In Navigator you can drag normal looking (i.e.
>visible) separators on to a toolbar. I think that the reason MailNews has
>*different* invisible looking separators is that the mailnews primary buttons
>have this strange left/right borders on hover effect which is ruined with
>visible separators. In any case I want normal looking separators in Navigators
>at least.
Ordinary toolbars I have no issue with (indeed, we have one in our default set). It's primary toolbars that have invisible separators. (Have you looked at our other primary toolbars recently?)

Comment 11

[[e-mail]]

Comment on attachment 372426 [details] [diff] [review]
Patch v1.1

OK, so sr=me with all my comments addressed except for the toolbar id which I understand exists for Lightning compatibility.

Comment 12

[Philip Chee]

Attachment: Patch v.1.2

Carrying forward sr+ from Neil

>>+        gDeleteButton.setAttribute("hidden", true);
>>+        gDeleteButton.removeAttribute("hidden");
> [These could be switched to gDeleteButton.hidden = forNews;]
Fixed.

>>+  UpdateMailToolbar(); // XXXRatty: do we need this?
> No, you shouldn't need it.
Fixed.

>>+      <menubar id="mail-menubar"/>
> Heh, it's a bit of a cheat ;-)
As discussed over IRC, I'll file a followup bug to tidy this up.

> Modern doesn't seem to have primary toolbarseparators in any window, so this
> belongs somewhere global.
Moved to communicator/toolbar.css where some related styles live.

>> (I also noticed that toolbarspacers take a different
>> width when customising, which is really a separate bug.)
> I think that this is because the two separators change width in customize mode.
> I'll have a look to see what can be done.
Fixed.

> OK, so sr=me with all my comments addressed except for the toolbar id which I
> understand exists for Lightning compatibility.
OK.

Comment 13

[Philip Chee]

>>+  min-width: 0px !important;
> Both msgReadSMIMEOverlay and msgHdrViewOverlay re-import messenger.css so file
> a bug and note it here too.

Filed Bug 488522.

Comment 14

[Philip Chee]

Attachment: Patch v1.3 (fix prefpane button listeners as well)

I missed removing the old mail button prefs the mail prefpane UI and the mail button pref listeners.

Comment 15

[[e-mail]]

Comment on attachment 372931 [details] [diff] [review]
Patch v1.3 (fix prefpane button listeners as well)

>-/* file, print, and stop hidden by default.
You'd better change the defaultset then.

Comment 16

[Karsten Düsterloh]

Comment on attachment 372931 [details] [diff] [review]
Patch v1.3 (fix prefpane button listeners as well)

JFTR: Modern obviously isn't made for normal icons with text beside them - the gradient in the icons isn't part of the text background...
(That's probably something for long winter nights for someone extremely bored. *g*)

Comment 17

[Philip Chee]

Attachment: [for checkin] Patch v1.4 Fix Nits

Carrying forward r+/sr+

>>-/* file, print, and stop hidden by default.
> You'd better change the defaultset then.
Fixed.

> JFTR: Modern obviously isn't made for normal icons with text beside them - the
> gradient in the icons isn't part of the text background...
Yes we discovered this when doing toolbar customization in Navigator. Misak has done some new SVGs for small icon mode, but we can reuse them to make new button graphics with alpha transparency for the normal sized buttons.

Comment 18

[Stefan [:stefanh]]

http://hg.mozilla.org/comm-central/rev/7ed66b843923

Comment 19

[Robert Kaiser]

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query