Last Comment Bug 413385 - Implement Customizable Toolbars in SeaMonkey MailNews
: Implement Customizable Toolbars in SeaMonkey MailNews
Status: RESOLVED FIXED
: fixed-seamonkey2.0
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Message Display (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: seamonkey2.0b1
Assigned To: Philip Chee
:
:
Mentors:
Depends on: CustomToolbars 456940 470112 475711 488522 488533 491676 513446
Blocks: 489318 78532 413387 434974 509209 521927
  Show dependency treegraph
 
Reported: 2008-01-21 12:49 PST by Robert Kaiser
Modified: 2010-08-22 13:15 PDT (History)
11 users (show)
kairo: wanted‑seamonkey2.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 (46.40 KB, patch)
2009-04-02 06:58 PDT, Philip Chee
mnyromyr: review-
Details | Diff | Splinter Review
diff-w of mailWindowOverlay.xul (17.92 KB, patch)
2009-04-02 07:11 PDT, Philip Chee
no flags Details | Diff | Splinter Review
DOMi CSS pane on mailnews toolbarbutton. (20.12 KB, image/png)
2009-04-13 01:55 PDT, Philip Chee
no flags Details
Patch v1.1 (47.13 KB, patch)
2009-04-13 11:07 PDT, Philip Chee
neil: superreview+
Details | Diff | Splinter Review
Patch v.1.2 (47.29 KB, patch)
2009-04-15 09:43 PDT, Philip Chee
philip.chee: superreview+
Details | Diff | Splinter Review
Patch v1.3 (fix prefpane button listeners as well) (55.57 KB, patch)
2009-04-15 12:03 PDT, Philip Chee
mnyromyr: review+
neil: superreview+
Details | Diff | Splinter Review
[for checkin] Patch v1.4 Fix Nits (55.53 KB, patch)
2009-04-21 09:53 PDT, Philip Chee
philip.chee: review+
philip.chee: superreview+
Details | Diff | Splinter Review

Description Robert Kaiser 2008-01-21 12:49:42 PST
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 2009-04-02 06:58:30 PDT
Created attachment 370637 [details] [diff] [review]
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 2009-04-02 07:11:23 PDT
Created attachment 370642 [details] [diff] [review]
diff-w of mailWindowOverlay.xul
Comment 3 Karsten Düsterloh 2009-04-09 14:52:44 PDT
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 2009-04-09 15:08:40 PDT
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 2009-04-09 18:30:40 PDT
> 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 2009-04-13 01:55:04 PDT
Created attachment 372373 [details]
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 2009-04-13 11:07:35 PDT
Created attachment 372426 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-04-13 13:58:34 PDT
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 2009-04-13 21:40:48 PDT
>>+           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 neil@parkwaycc.co.uk 2009-04-14 02:56:14 PDT
(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 neil@parkwaycc.co.uk 2009-04-14 02:58:56 PDT
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 2009-04-15 09:43:30 PDT
Created attachment 372886 [details] [diff] [review]
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 2009-04-15 10:02:34 PDT
>>+  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 2009-04-15 12:03:33 PDT
Created attachment 372931 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-04-15 12:10:34 PDT
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 2009-04-20 11:12:30 PDT
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 2009-04-21 09:53:31 PDT
Created attachment 373864 [details] [diff] [review]
[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] (away until December 6) 2009-04-21 10:54:09 PDT
http://hg.mozilla.org/comm-central/rev/7ed66b843923
Comment 19 Robert Kaiser 2009-09-18 06:18:25 PDT
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query

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