Implement Customizable Toolbars in SeaMonkey MailNews

RESOLVED FIXED in seamonkey2.0b1

Status

SeaMonkey
MailNews: Message Display
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Robert Kaiser, Assigned: Philip Chee)

Tracking

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

Trunk
seamonkey2.0b1
fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
wanted-seamonkey2.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Blocks: 413387
Blocks: 434974
Assignee: mail → nobody
QA Contact: message-display
(Reporter)

Updated

9 years ago
Flags: wanted-seamonkey2+
(Assignee)

Updated

8 years ago
Depends on: 470112
(Assignee)

Updated

8 years ago
Depends on: 456940
(Assignee)

Updated

8 years ago
Blocks: 78532
(Assignee)

Updated

8 years ago
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Comment 2

8 years ago
Created attachment 370642 [details] [diff] [review]
diff-w of mailWindowOverlay.xul
Attachment #370642 - Flags: review?(mnyromyr)

Comment 3

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

Updated

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

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

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

Comment 12

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

Updated

8 years ago
Depends on: 488522
(Assignee)

Comment 13

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

Filed Bug 488522.
(Assignee)

Updated

8 years ago
Depends on: 488533
(Assignee)

Comment 14

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

Updated

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

Comment 16

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

Comment 17

8 years ago
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.
Attachment #372931 - Attachment is obsolete: true
Attachment #373864 - Flags: superreview+
Attachment #373864 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 18

8 years ago
http://hg.mozilla.org/comm-central/rev/7ed66b843923
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b1

Updated

8 years ago
Keywords: checkin-needed
Blocks: 489318
Depends on: 491676
Blocks: 497400
(Assignee)

Updated

8 years ago
No longer blocks: 497400
(Assignee)

Updated

8 years ago
Blocks: 509209

Updated

8 years ago
Depends on: 513446
(Reporter)

Comment 19

8 years ago
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Keywords: fixed-seamonkey2.0

Updated

8 years ago
Blocks: 521927
You need to log in before you can comment on or make changes to this bug.