Closed Bug 413385 Opened 13 years ago Closed 12 years ago

Implement Customizable Toolbars in SeaMonkey MailNews

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b1

People

(Reporter: kairo, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(3 files, 4 obsolete files)

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.
Blocks: 413387
Assignee: mail → nobody
QA Contact: message-display
Flags: wanted-seamonkey2+
Depends on: 470112
Depends on: 456940
Blocks: 78532
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #370637 - Flags: review?(mnyromyr)
Attachment #370642 - Flags: review?(mnyromyr)
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.
Attachment #370637 - Flags: review?(mnyromyr) → review-
Attachment #370642 - Flags: review?(mnyromyr)
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...
> 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.
Depends on: 475711
>>>+  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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
>>+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.
Attachment #370637 - Attachment is obsolete: true
Attachment #372426 - Flags: superreview?(neil)
Attachment #372426 - Flags: review?(mnyromyr)
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.)
>>+           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.
(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 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.
Attachment #372426 - Flags: superreview?(neil) → superreview+
Attached patch Patch v.1.2 (obsolete) — Splinter Review
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.
Attachment #372426 - Attachment is obsolete: true
Attachment #372886 - Flags: superreview+
Attachment #372886 - Flags: review?(mnyromyr)
Attachment #372426 - Flags: review?(mnyromyr)
Depends on: 488522
>>+  min-width: 0px !important;
> Both msgReadSMIMEOverlay and msgHdrViewOverlay re-import messenger.css so file
> a bug and note it here too.

Filed Bug 488522.
Depends on: 488533
I missed removing the old mail button prefs the mail prefpane UI and the mail button pref listeners.
Attachment #372886 - Attachment is obsolete: true
Attachment #372931 - Flags: superreview?(neil)
Attachment #372931 - Flags: review?(mnyromyr)
Attachment #372886 - Flags: review?(mnyromyr)
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.
Attachment #372931 - Flags: superreview?(neil) → superreview+
Attachment #372931 - Flags: review?(mnyromyr) → review+
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*)
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.
Attachment #372931 - Attachment is obsolete: true
Attachment #373864 - Flags: superreview+
Attachment #373864 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/7ed66b843923
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b1
Keywords: checkin-needed
Depends on: 491676
No longer blocks: 497400
Blocks: 509209
Depends on: 513446
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Blocks: 521927
You need to log in before you can comment on or make changes to this bug.