394288 - (CustomToolbars) Implement Customizable Toolbars in Navigator

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Philip Chee]

Attachment: WIP Patch v0.1

The existing bugs are rather old and there is too much noise in them. This bug is specifically for Navigator but includes generic code that can be used for other windows.

The patch I am attaching is a WIP patch for comments.
What works:
1. The context menu works.
1a. Can toggle on/off the various toolbars.
1b. Can invoke the Customize Toolbars window.
2. Can create custom toolbars and drag'n'drop toolbar buttons to and from these toolbars.

TODO:
1. Make the standard toolbars (menubar, nav-bar, personal toolbar) customizable.
2. Possibly make the link toolbar customizable (?)

I managed to get the CSS working. For some reason it has to be in communicator/skin/toolbar.css and not in global/skin/toolbar.css. Unfortunately I can't test on OSX, OS2, nor Linux, so someone will need to eyeball these CSS changes on these platforms.

Comment 1

[Philip Chee]

Attachment: Screenshot of WIP Patch 0.1

Comment 2

[Philip Chee]

Attachment: skin/modern/global/toolbar/spring.gif

This is a copy of skin/classic/global/toolbar/spring.gif which should be added to skin/modern/global/toolbar/spring.gif

Comment 3

[Philip Chee]

Attachment: Patch 0.2

New patch. What works:
Custom toolbars.
Individual button mode and icon size settings for each customizable toolbar.
Customizable: menubar and personal toolbar.

Making the navigation toolbar customizable will take more work so I'm leaving that to later.

Comment 4

[[e-mail]]

Comment on attachment 280347 [details] [diff] [review]
Patch 0.2

>-    this.updateSeparator();
You also need to remove the button preferences for home and bookmarks.

>+  getNavToolbox().customizeInit = BrowserToolboxCustomizeInit;
>+  getNavToolbox().customizeDone = BrowserToolboxCustomizeDone;
No point delaying this.

>+  var cmd = document.getElementById("cmd_CustomizeToolbars");
>+  cmd.setAttribute("disabled", "true");
Why?

>+    gURLBar = document.getElementById("urlbar");
>+    gProxyButton = document.getElementById("page-proxy-button");
>+    gProxyFavIcon = document.getElementById("page-proxy-favicon");
>+    gProxyDeck = document.getElementById("page-proxy-deck");
You're not actually customising these yet...

>+    updateHomeButtonTooltip(); // gHomeButton.updateTooltip();
Why?

>+    window.XULBrowserWindow.init();
You're not actually customising these yet... why does customising clone this stuff anyway?

>+  //XXXRatty: find a better place to put this.
>+  getNavToolbox().updateCustomToolbarModes();
What does this do anyway?

>+  // fix up the personal toolbar folder
This is more cloning stuff, right? [The RDF backend really hates that]

>+  // XXXRatty Firefox need this, does Suite?
>+  window.focus();
This is almost always wrong.

>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>+           mode="icons" defaultmode="icons">
Something needs to do done about this... our defaults are different per toolbar.

>+        <menubar id="main-menubar" xpfe="false"/>
Better still, change our CSS so that customisable menubars don't get grippies.

>+    <toolbarpalette id="BrowserToolbarPalette">
Please put the toolbarpalette at the end so that I can see what the actual differences are.

>+    <command id="cmd_CustomizeToolbars" oncommand="BrowserCustomizeToolbar()"/>
Shouldn't that be in utilityOverlay.js, and calling goCustomizeToolbar()?

>+  if ("customizeInit" in getNavToolbox())
Can't call getNavToolbox() from utilityOverlay.js - you need to use a generic name for the toolbox, or find some other way of locating it.

>+  window.openDialog("chrome://global/content/customizeToolbar.xul",
>+                    "CustomizeToolbar",
This will do silly things if you don't close the first window and start customising a second window.

>+    var toolbarName = toolbar.getAttribute("toolbarname");
>+    var type = toolbar.getAttribute("type");
>+    if (toolbarName && type != "menubar") {
But the menubar doesn't have a toolbarname, does it?

>+    toolbar = toolbar.nextSibling; // XXXRatty: this is a for loop so why Firefox why?
Well at least you didn't blindly copy them...

>+  while(toolbar.localName!="toolbar")
Nit: space before (

>+    if(radio.value==mode) {
Nit: spaces around ==

>+function onViewToolbarCommand(aEvent)
>+{
>+  goToggleToolbar(aEvent.originalTarget.getAttribute("toolbarid"));
This doesn't update the View Show/Hide submenu...

>+% style chrome://global/content/customizeToolbar.xul chrome://navigator/skin/navigator.css
Houston, we have a problem... all the suite windows use the same style rules to set the images on their primary toolbar buttons...

>+#menubar-items > menubar {
>+  border: 0px none;
>+  margin: 0px;
>+  padding: 0px;
>+}
Why is this even necessary?

>+toolbar[mode="icons"] .toolbarbutton-text {
I'm not sure this belongs in toolbar.css as it's supposed to affect toolbarbuttons so it only works because we import toolbar.css from communicator.css - which we shouldn't be doing, we should resource it from toolbar.xml (but that's a separate bug).

I didn't review the rest of the CSS because I think it's bitrotted anyway.

Comment 5

[Philip Chee]

(In reply to comment #4)
>>-    this.updateSeparator();
>You also need to remove the button preferences for home and bookmarks.
OK.

>>+  getNavToolbox().customizeInit = BrowserToolboxCustomizeInit;
>>+  getNavToolbox().customizeDone = BrowserToolboxCustomizeDone;
>No point delaying this.
Hmm. Why does Firefox put this in the delay section?

>>+  var cmd = document.getElementById("cmd_CustomizeToolbars");
>>+  cmd.setAttribute("disabled", "true");
>Why?

This will do silly things if you don't close the first window and open a second customize window. So we disable the command until the customize window is closed.

>>+    gURLBar = document.getElementById("urlbar");
>>+    gProxyButton = document.getElementById("page-proxy-button");
>>+    gProxyFavIcon = document.getElementById("page-proxy-favicon");
>>+    gProxyDeck = document.getElementById("page-proxy-deck");
>You're not actually customising these yet...
OK.

>>+    updateHomeButtonTooltip(); // gHomeButton.updateTooltip();
>Why?
If on startup the home button is in the palette and not in the DOM then when Startup() calls |updateHomeButtonTooltip()| it isn't updated (which reminds me I need to change this function to check if the home button is found at all in the first place). So every time we exit the customize window, we call this in case the home button has been dragged out of the palette.

>>+    window.XULBrowserWindow.init();
>You're not actually customising these yet... why does customising clone this
>stuff anyway?

I guess this is a lazy way of updating:
window.XULBrowserWindow.{urlBar|throbberElement} (yes, yes, I know I am not customizing at the moment).

>>+  //XXXRatty: find a better place to put this.
>>+  getNavToolbox().updateCustomToolbarModes();
>What does this do anyway?

KaiRo wanted individual settings for each toolbar, so this is a hack which isn't in Firefox.

>>+  // fix up the personal toolbar folder
>This is more cloning stuff, right? [The RDF backend really hates that]

On closer inspection of the customizeToolbar code, the buttons aren't cloned but are moved in and out between the DOM and the .palette property. I don't know why the observer needs to be reinstated but running a rebuild shouldn't be a problem should it?

>>+  // XXXRatty Firefox need this, does Suite?
>>+  window.focus();
>This is almost always wrong.
OK.

>>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>>+	     mode="icons" defaultmode="icons">
>Something needs to do done about this... our defaults are different per
>toolbar.

The only non-hacky way would be to enhance the |restoreDefaultSet()| function in customizeToolbar.js. I don't know how locked down toolkit is wrt Firefox 3.0a/b.

>>+	  <menubar id="main-menubar" xpfe="false"/>
>Better still, change our CSS so that customisable menubars don't get grippies.

Doesn't the xpfe="false" trigger the CSS to get non grippy toolbars?

>>+    <toolbarpalette id="BrowserToolbarPalette">
>Please put the toolbarpalette at the end so that I can see what the actual
>differences are.
OK

>>+    <command id="cmd_CustomizeToolbars"
>oncommand="BrowserCustomizeToolbar()"/>
>Shouldn't that be in utilityOverlay.js, and calling goCustomizeToolbar()?

I wanted to leave this in case some Firefox extension writers want to see:

+function BrowserCustomizeToolbar() // Function Name left for Firefox compatibility.
+{
+  goCustomizeToolbar();
+}

But sure, I could call it directly.

>>+  if ("customizeInit" in getNavToolbox())
>Can't call getNavToolbox() from utilityOverlay.js - you need to use a generic
>name for the toolbox, or find some other way of locating it.

Bug 321954 envisages that all windows that want a print preview toolbar have a |getNavToolbox()| function. I thought I'd extend this paradigm to the customizeToolbar functionality. Thus messenger, messageWindow, Compose, etc should implement their own getNavToolbox() function. I think that it would be better to be caled getMainToolbox but getNavToolbox already exists due to 321954.

>>+  window.openDialog("chrome://global/content/customizeToolbar.xul",
>>+		      "CustomizeToolbar",
>This will do silly things if you don't close the first window and start
>customising a second window.

+  window.openDialog("chrome://global/content/customizeToolbar.xul",
+                    "CustomizeToolbar",
+                    "chrome,all,dependent",
+                    getNavToolbox());
+}

When called from a different window, a different toolbox context is fed to customizeToolbar.xul via getNavToolbox().

>>+    var toolbarName = toolbar.getAttribute("toolbarname");
>>+    var type = toolbar.getAttribute("type");
>>+    if (toolbarName && type != "menubar") {
>But the menubar doesn't have a toolbarname, does it?

Good point. But what if some eager beaver comes around later and gives the menubar a toolbarname in the cause of consistency and doesn't realise that they have to update this code as well?

>>+    toolbar = toolbar.nextSibling; // XXXRatty: this is a for loop so why
>Firefox why?
>Well at least you didn't blindly copy them...
:)

>>+  while(toolbar.localName!="toolbar")
>Nit: space before (
OK

>>+    if(radio.value==mode) {
>Nit: spaces around ==
OK

>>+function onViewToolbarCommand(aEvent)
>>+{
>>+  goToggleToolbar(aEvent.originalTarget.getAttribute("toolbarid"));
>This doesn't update the View Show/Hide submenu...
Ooops.

>>+% style chrome://global/content/customizeToolbar.xul
>chrome://navigator/skin/navigator.css
>Houston, we have a problem... all the suite windows use the same style rules to
>set the images on their primary toolbar buttons...

Oh dearie me. Where can I put these? Perhaps a new file like:
chrome://communicator/skin/customizeToolbar.css ?

>>+#menubar-items > menubar {
>>+  border: 0px none;
>>+  margin: 0px;
>>+  padding: 0px;
>>+}
>Why is this even necessary?

It looks like I only need this in Windows/classic due to
/toolkit/themes/winstripe/global/toolbar.css which does:
toolbar:first-child, menubar {
  min-width: 1px;
  border-bottom: 1px solid ThreeDShadow;
  border-top: 0px !important;
}

And /toolkit/themes/pmstripe/global/toolbar.css
(what's pmstripe anyway)??
toolbar[type="menubar"], menubar {
  background-color: Menu;
  -moz-appearance: toolbar;
  min-width: 1px; /* DON'T DELETE! */
  border-top: none !important;
  border-bottom: 1px solid;
  -moz-border-bottom-colors: ThreeDShadow;
}

Hmm. DOM inspector says that the bottom border is 0px even with my additional rules. But I don't see any other styles that override this?

>>+toolbar[mode="icons"] .toolbarbutton-text {
>I'm not sure this belongs in toolbar.css as it's supposed to affect
>toolbarbuttons so it only works because we import toolbar.css from
>communicator.css - which we shouldn't be doing, we should resource it from
>toolbar.xml (but that's a separate bug).
>
>I didn't review the rest of the CSS because I think it's bitrotted anyway.

Er, thanks.

Comment 6

[Philip Chee]

> Hmm. DOM inspector says that the bottom border is 0px even with my additional
> rules. But I don't see any other styles that override this?
s/with/without/

Comment 7

[[e-mail]]

(In reply to comment #5)
>(In reply to comment #4)
>>>+  getNavToolbox().customizeInit = BrowserToolboxCustomizeInit;
>>>+  getNavToolbox().customizeDone = BrowserToolboxCustomizeDone;
>>No point delaying this.
>Hmm. Why does Firefox put this in the delay section?
They put everything there if they can get away with it, because they're so scared of regressing Ts/Txul.

>>>+  var cmd = document.getElementById("cmd_CustomizeToolbars");
>>>+  cmd.setAttribute("disabled", "true");
>>Why?
>This will do silly things if you don't close the first window and open a second
>customize window. So we disable the command until the customize window is closed.
How are you going to open a second customize window? Doesn't customising disable the menu anyway?

>>>+    updateHomeButtonTooltip(); // gHomeButton.updateTooltip();
>>Why?
>If on startup the home button is in the palette and not in the DOM then when
>Startup() calls |updateHomeButtonTooltip()| it isn't updated (which reminds me
>I need to change this function to check if the home button is found at all in
>the first place). So every time we exit the customize window, we call this in
>case the home button has been dragged out of the palette.
Wait, customising removes stuff from the DOM? How can that work?

>On closer inspection of the customizeToolbar code, the buttons aren't cloned
>but are moved in and out between the DOM and the .palette property. I don't
>know why the observer needs to be reinstated but running a rebuild shouldn't be
>a problem should it?

>>>+	  <menubar id="main-menubar" xpfe="false"/>
>>Better still, change our CSS so that customisable menubars don't get grippies.
>Doesn't the xpfe="false" trigger the CSS to get non grippy toolbars?
But what if some eager beaver adds toolbar customisation to their extension and doesn't realise that they have add this attribute as well?

>>>+    <command id="cmd_CustomizeToolbars"
>>oncommand="BrowserCustomizeToolbar()"/>
>>Shouldn't that be in utilityOverlay.js, and calling goCustomizeToolbar()?
>But sure, I could call it directly.
Right, that's what I meant.

>>>+  if ("customizeInit" in getNavToolbox())
>>Can't call getNavToolbox() from utilityOverlay.js - you need to use a generic
>>name for the toolbox, or find some other way of locating it.
>Bug 321954 envisages that all windows that want a print preview toolbar have a
>|getNavToolbox()| function.
I bet Thunderbird will appreciate that ;-)

>>>+  window.openDialog("chrome://global/content/customizeToolbar.xul",
>>>+		      "CustomizeToolbar",
>>This will do silly things if you don't close the first window and start
>>customising a second window.
>When called from a different window, a different toolbox context is fed to
>customizeToolbar.xul via getNavToolbox().
I didn't include the call to getNavToolbox because it's not relevant...

>>>+% style chrome://global/content/customizeToolbar.xul
>>chrome://navigator/skin/navigator.css
>>Houston, we have a problem... all the suite windows use the same style rules to
>>set the images on their primary toolbar buttons...
>Oh dearie me. Where can I put these? Perhaps a new file like:
>chrome://communicator/skin/customizeToolbar.css ?
No, I think you'll just have to rewrite all the style rules :-(

>(what's pmstripe anyway)??
OS/2

Comment 8

[Karsten Düsterloh]

(In reply to comment #4)
> >+        <menubar id="main-menubar" xpfe="false"/>
> Better still, change our CSS so that customisable menubars don't get grippies.

Wait a second - you want to kill the grippies?

Comment 9

[[e-mail]]

(In reply to comment #8)
>(In reply to comment #4)
>>>+        <menubar id="main-menubar" xpfe="false"/>
>>Better still, change our CSS so that customisable menubars don't get grippies.
>Wait a second - you want to kill the grippies?
Menubars aren't customisable. Instead, we wrap them in a toolbar. This means that we have to turn off grippies on the original menubar.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

The changes in bug 354048 might be relevant to this patch. I don't know whether that's going to land in 1.9 or not.

Comment 11

[Philip Chee]

Adding some comments from #IRC so I don't lose them:

[13:53:19]  <Ratty> NeilAway: You wrote in 394288: "No, I think you'll just have to rewrite all the style rules :-( "
[13:57:06]  <NeilAway>  Ratty: well, you won't have to do it as part of the patch, but it will need to be done before the next window is enabled
[14:00:04]  <NeilAway>  Ratty: let's say I drag the throbber off the toolbar, where does it go?
[14:01:00]  <Ratty> NeilAway: The throbber is removed from the toolbar.
[14:01:34]  <KaiRo> Ratty, Mnyromyr: what we should add in CSS is styles for a menubar that is a child of a toolbar - they nice thing is that this construct also allows us things like putting toobar buttons (or the throbber) in the same bar with the menubar through customization
[14:01:42]  <Ratty> A pointer to that node is still in the .palette property
[14:03:07]  <NeilAway>  Ratty: so when you do XULBrowserWindow.init() won't that say "hey, where'd the throbber go?"
[14:03:54]  <Ratty> NeilAway: document.getElementById() will just return null.
[14:04:52]  <NeilAway>  Ratty: so that means that all the code that updates the UI has to either a) update a hidden broadcaster that the element watches or b) deal with nulls?
[14:05:35]  <Ratty> NeilAway: Looks like it. what a PITA.

Comment 13

[Philip Chee]

Attachment: btn1.png from the modern aluminium theme

I got tired of the artifacts caused by the hard coded gradient in the modern nav-bar button backgrounds so I've been testing my patch with this instead. It's a PNG with transparent backgrounds for the buttons.

Comment 14

[Philip Chee]

Attachment: Some other btn1.png variants for navigator in modern mode.

The one from the ReallyModern theme looks very nice. Too bad the colour scheme departs too far from the standard modern.

Comment 15

[Philip Chee]

Attachment: Patch 1.0

I'm putting this patch up for review although there is room for more polish.

The CSS I use is pretty bogus since I just threw stuff at random at the wall until things started working.

Also some of the issues not addressed:

Separate defaults for some toolbars (e.g. the Personal Toolbar). I plan to submit a patch to the toolkit customizeToolbars.js to recognize the defaulticonsize and defaultmode attributes when placed on the individual toolbars.

Make the throbber observe a broadcaster that always exists in the DOM so that the code that updates the throbber "busy" attribute doesn't have to keep checking if it is in the DOM or not.

Comment 16

[Philip Chee]

The hidden state of custom toolbars is not restored on startup. Fixed in the next patch.

Comment 17

[Philip Chee]

Attachment: Patch 1.1

Changes from v1.0:

1. Make the throbber observe a broadcaster that always exists in the DOM so that
the code that updates the throbber "busy" attribute doesn't have to keep
checking if it is in the DOM or not.

2. The hidden state of custom toolbars is not restored on startup. Fixed.

Comment 18

[[e-mail]]

Comment on attachment 288990 [details] [diff] [review]
Patch 1.1

I now understand the disabled context menu item - I didn't realise that you could still right-click on toolbars while customising them.

Some issues I noticed while trying out this patch:
1. Throbber styling is wrong, and doesn't load SeaMonkey page
2. Just opening the customise window clears the URLbar
3. Back/Forward buttons are always disabled after being dragged on to the toolbar
4. Stop button is always enabled after being dragged on to the toolbar
5. Favicon is missing after URLbar is dragged on to the toolbar

Comment 19

[[e-mail]]

Comment on attachment 288990 [details] [diff] [review]
Patch 1.1

>+      <toolbarbutton id="throbber-box" align="center" pack="center">
>+        <button id="navigator-throbber" oncommand="goClickThrobber('browser.throbber.url')"
>+                observes="throbber-broadcaster" tooltiptext="&throbber.tooltip;"/>
>+      </toolbarbutton>
This is definitely wrong; you don't put buttons in toolbarbuttons...

Comment 20

[Philip Chee]

Attachment: Patch 1.2

(In reply to comment #18)
> 1. Throbber styling is wrong, and doesn't load SeaMonkey page
(In reply to comment #19)
> This is definitely wrong; you don't put buttons in toolbarbuttons...
Oops. Should be a <toolbaritem> instead. Fixed.

> 2. Just opening the customise window clears the URLbar
> 3. Back/Forward buttons are always disabled after being dragged on to the toolbar
Fixed in these two cases by using an <observes> element instead of an observes attribute.

> 4. Stop button is always enabled after being dragged on to the toolbar
Fixed using a command attribute and updating the disabled attribute on the command element.

> 5. Favicon is missing after URLbar is dragged on to the toolbar
Fixed. A bit more tricky. SetPageProxyState() in SeaMonkey doesn't quite work the same way as in Firefox. I changed it a bit with some logic borrowed from Firefox.

Also fixed: In classic the search button icon was not hidden with the toolbar was set to "text" mode.

Comment 21

[Philip Chee]

Index: themes/classic/communicator/mac/toolbar.css
[...]
+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-text,
+toolbar[mode="text"] .toolbarbutton-1 > stack > toolbarbutton > .toolbarbutton-text {
+  margin: 4px;
+}

I copied this from pinstripe but I'm beginning to think that the second rule with the stack is bogus. I grepped through suite/themes/ and toolkit/themes/ and as far as I can tell only the suite modern theme uses a stack for toolbarbutton-menu-buttons. I can't find any mac specific bindings for this type of toolbarbutton. Can anyone with a Mac confirm?

Comment 22

[Stefan [:stefanh]]

Comment on attachment 290127 [details] [diff] [review]
Patch 1.2

I just tried this patch on mac and I immediately see 2 problems (that I don't see in Minefield:

1) The customiza toolbar dialog moves behind th navigator window when you click on a toolbar item (that's confusing, because you'd want to drag the item to the dialog)

2) The main menubar disappears, instead you just see the application menu.

Then there's a odd grey field that gets added to the upper part of the toolbar when you have the dialog open and the navigation buttons gets switched to Firefox's - but maybe that's known?

Comment 23

[Stefan [:stefanh]]

Comment on attachment 290127 [details] [diff] [review]
Patch 1.2

Actually, it's like another toolbar with space in (hence the gray) above.  I also notice that the default is small buttons with this patch, but the "Home" button is large.

Comment 24

[Stefan [:stefanh]]

(classic theme)

Comment 25

[Philip Chee]

Could you put up some screenshots please? Thanks.

Comment 26

[Philip Chee]

AFAIK, Minefield uses a sheet (stack) over the appcontent instead of a separate window for OSX.

Comment 27

[Philip Chee]

Oh They've switched from using a stack to a hidden <panel>. Still pretty hacky looking at the XXX comments all over.

Comment 28

[Stefan [:stefanh]]

Attachment: Screenshots, before/after attachment #290127

Some additional notes:

- I was wrong about the small icons, but the Home button is larger
- Seems that the text & icons mode is affected, after the patch I just get icons
- note the effect on the menubar

I get a bunch of these assertions upon opening/closing the dialog (this could be totally irrelevant, because I haven't investigated what they mean and if they appear during normal use):

###!!! ASSERTION: attempt to remove an element that was never added: 'hep != nsnull && *hep != nsnull', file /Users/Stefan/mozilla-trunk/mozilla/content/xul/document/src/nsElementMap.cpp, line 280

Comment 29

[Stefan [:stefanh]]

(In reply to comment #21)
> Index: themes/classic/communicator/mac/toolbar.css
> [...]
> +toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-text,
> +toolbar[mode="text"] .toolbarbutton-1 > stack > toolbarbutton >
> .toolbarbutton-text {
> +  margin: 4px;
> +}

Btw, since those are in pinstripe/global we should get them anyway, no?

Comment 30

[Philip Chee]

(In reply to comment #22)
> Then there's a odd grey field that gets added to the upper part of the toolbar
> when you have the dialog open
(In reply to comment #23)
> Actually, it's like another toolbar with space in (hence the gray) above.

I forgot that Mac menus work differently. I put this in /suite/common/communicator.css:

+.menubar-items {
+  -moz-box-orient: vertical; /* for flex hack */
+}
+
+.menubar-items > menubar {
+  -moz-box-flex: 1; /* make menu items expand to fill toolbar height */
+}

Closer inspection of browser/themes/ shows that the equivalent styles are missing from /pinstripe/. Now I can't put these styles in /suite/themes/modern/ because of the no-OS-specific stuff in modern rule. If I make communicator.css preprocessed then I can wrap these styles in a #IFNDEF MAC_OSX. Anybody have a better suggestion?

(In reply to comment #22)
> 1) The customiza toolbar dialog moves behind th navigator window when you click
> on a toolbar item (that's confusing, because you'd want to drag the item to the
> dialog)
Do "dependent" dialogs work differently in OSX then?

(In reply to comment #22)
> - I was wrong about the small icons, but the Home button is larger
Mutter, mutter. In unpatched SeaMonkey the home button has a class of "bookmark-item" so:
http://lxr.mozilla.org/seamonkey/source/suite/themes/classic/communicator/bookmarks/bookmarksToolbar.css#130
http://lxr.mozilla.org/seamonkey/source/suite/themes/modern/communicator/bookmarks/bookmarksToolbar.css#82
.bookmark-item > .menu-iconic-left > .menu-iconic-icon {
  width: 16px;
  height: 16px;
}
But I removed that class since post-patch the home button can be moved to other toolbars.

> - Seems that the text & icons mode is affected, after the patch I just get
> icons

Well it's true that pre-patch the default for the nav-bar is icon+text but in modern the labels are hidden in icon+text mode. That's why I made icon-only the default mode in my patch.

(In reply to comment #29)
> Btw, since those are in pinstripe/global we should get them anyway, no?

You are correct. I was confused because I looked at classic/jar.mn and didn't see where the global styles were packaged. This simplifies things. Thanks.

Comment 31

[Philip Chee]

> width: 16px;
> height: 16px;

If I make the homebutton 16x16 then it won't match the other small mode buttons which are 19x19. I could special case the home button when on the PersonalToolbar but if someone drags another navigation button to that toolbar there would be a mismatch too. On the gripping hand how many users would do that? Anyway I all ready do special case the home button in Modern so to be consistent I guess I'll have to do the same for Classic.

Comment 32

[Robert Kaiser]

I think the slightly bigger home button is ok if the bookmarks toolbar next to it behaves well.
We should not preprocess the Modern theme, and as 3rd party themes aren't platform-specific as well usually, we should find a solution that can work on all platforms. Can the toolbar containing the menubar just be somehow excluded from appearing on mac at all?

Comment 33

[Stefan [:stefanh]]

(In reply to comment #30)
> (In reply to comment #22)
> > Then there's a odd grey field that gets added to the upper part of the toolbar
> > when you have the dialog open
> (In reply to comment #23)
> > Actually, it's like another toolbar with space in (hence the gray) above.
> 
> I forgot that Mac menus work differently. I put this in
> /suite/common/communicator.css:
> 
> +.menubar-items {
> +  -moz-box-orient: vertical; /* for flex hack */
> +}
> +
> +.menubar-items > menubar {
> +  -moz-box-flex: 1; /* make menu items expand to fill toolbar height */
> +}
> 
> Closer inspection of browser/themes/ shows that the equivalent styles are
> missing from /pinstripe/. Now I can't put these styles in /suite/themes/modern/
> because of the no-OS-specific stuff in modern rule. If I make communicator.css
> preprocessed then I can wrap these styles in a #IFNDEF MAC_OSX. Anybody have a
> better suggestion?

Those style rules doesn't seem to matter - I tried removing them, but I still see the extra toolbar.

> (In reply to comment #22)
> > 1) The customiza toolbar dialog moves behind th navigator window when you click
> > on a toolbar item (that's confusing, because you'd want to drag the item to the
> > dialog)
> Do "dependent" dialogs work differently in OSX then?

"dependent" isn't implemented on OSX. Afaics Firefox/Thunderbird uses customizeToolbarSheet.xul on mac. See for example bug 394873 for the Thunderbird implementation.

Btw:
     <!-- Menu -->
-    <menubar id="main-menubar" persist="collapsed" grippytooltiptext="&menuBar.tooltip;"/>
+    <toolbar type="menubar" id="toolbar-menubar" class="chromeclass-menubar" customizable="true"

If you look at suite/browser/hiddenWindow.xul:

108   <toolbox id="toolbox">
109     <menubar id="main-menubar" position="1"/>
110   </toolbox>
111 
112 </window>

I keep on asking myself why I don't see any effect of this, but it might be that I'm tired atm. I haven't seen any issue, though. Hmm.

Comment 34

[Stefan [:stefanh]]

(In reply to comment #32)

> Can the toolbar containing the menubar just be somehow excluded
> from appearing on mac at all?
>

Perhaps using platformCommunicatorOverlay.xul?

Btw, it looks like I was tired after all, I know see that the "main-menubar" is a bit further down in the code (re my previous comment).

Comment 35

[Philip Chee]

(In reply to comment #32)
> We should not preprocess the Modern theme.

I was talking about /suite/common/communicator.css: not /suite/themes/modern/.... But the point is moot as Stefan says removing those styles doesn't make the gray toolbar go away.

KaiRo, Strfan, If we use the customizeToolbarSheet.xul on Mac then I'll need someone with a Mac who can help test since I don't have a Mac.

Comment 36

[[e-mail]]

What's the difference between a sheet and a window?

Can you make menubar inherit from toolbar so you don't need an inner menubar?

Comment 37

[Stefan [:stefanh]]

(In reply to comment #36)
> What's the difference between a sheet and a window?

A sheet slides down from underneath the titlebar. I haven't looked at the code, but I suspect that it emulates a sheet (it's obviously a hack involving an iframe). If you used a "standard" sheet, you wouldn't be able to click on the toolbar.

> Can you make menubar inherit from toolbar so you don't need an inner menubar?
> 
 Can we overlay that stuff? No?

Comment 38

[Stefan [:stefanh]]

(In reply to comment #35)
> (In reply to comment #32)
> > We should not preprocess the Modern theme.
> 
> I was talking about /suite/common/communicator.css: not
> /suite/themes/modern/.... But the point is moot as Stefan says removing those
> styles doesn't make the gray toolbar go away.
> 
> KaiRo, Strfan, If we use the customizeToolbarSheet.xul on Mac then I'll need
> someone with a Mac who can help test since I don't have a Mac.
> 

Sure, np.

Comment 39

[Philip Chee]

Attachment: Patch 1.3

I've stared and stared and stared at browser.xul and Thunderbird's messenger.xul and the only differences that I see compared to my patch is that:

1) There are no other default elements on the menu toolbar except for "menubar-items", and
2) This inline style |style="border:0px;padding:0px;margin:0px;-moz-appearance:none"| on the <menubar id="main-menubar">.

I've also looked at the three OSX hiddenWindow.xul implementations but nothing leaps out at me. Thunderbird shoves the whole toolbox in. Firefox only the <menubar>.

Stefan, could you try out this modified patch? Thanks.

Comment 40

[Stefan [:stefanh]]

Attachment: Screenshot - attachment #290852 in action

OK, some improvement (and, yes the icons are seamonkey's - I was obviously confused yesterday). First, some clarifications: As you can see in the screenshots, the menubar on mac is not positioned in the application window - it sits above, at the top of the desktop. So, what the native widget code do is taking the xul menubar and puts it on top of the desktop (very simplified, it probably hides it and renders a native menubar with menus). What I'm saying is that the menubar is not the issue here, it's the toolbar:

-  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true">
+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
+           mode="icons" defaultmode="icons">
     <!-- Menu -->
-    <menubar id="main-menubar" persist="collapsed" grippytooltiptext="&menuBar.tooltip;"/>
+    <toolbar type="menubar" id="toolbar-menubar" class="chromeclass-menubar" customizable="true"
+             persist="collapsed" grippytooltiptext="&menuBar.tooltip;"
+             defaultset="menubar-items"
+             mode="icons" iconsize="small"
+             defaulticonsize="small"
+             context="toolbar-context-menu">
+      <toolbaritem id="menubar-items" class="menubar-items" align="center">
+        <menubar id="main-menubar"/>
+      </toolbaritem>
+    </toolbar>

What's left in navigator.xul is a empty, customizable, toolbar (see last screenshot). So, afaics you really need to find a way to not use that toolbar on mac.

Using the dialog has 2 issues:

1) Mac users would expect the whole, original, menubar to stay unaffected when the customizetoolbar window opens. This will require that your new window has a menubar - the widget code takes the menubar from the frontmost window and puts it at the top of the desktop. This is probably one of the reasons why toolkit has that iframe-hack on mac.

2) Once you click on the toolbar, the dialog vanishes behind the navigator window. While 1) is something we could live with (we have the same issue in lots of other dialogs), this problem is a bit worse since you can only drag stuff from the dialog to the toolbar. Thinking of it, this might be the main reason why it's done different on mac.

Btw, note the Home button and how it affects the toolbar. Note also the differences in look when the dialog is opened - this could be related to the fact that the DOM is modified - some child selectors might not work anymore.

Comment 41

[Stefan [:stefanh]]

Philip suggested adding xpfe="false" to the <toolbar type="menubar"> to see if it was the grippies that made the "extra" toolbar appear on mac. He was right! With the grippy, the toolbar is 9px high. Without grippy the height is 0. Looking at Minefield, it's the same there - DOMi confirms a "extra" non-visible toolbar with 0px height. So, one issue seems solved here :-)

Btw, the "customize" context menu doesn't appear on the personal toolbar to the right of the Home button, but it does just around the button.

Comment 42

[Philip Chee]

Attachment: Patch 1.3a using grippyhidden to hide the menubar grippy

Stefan could you test this patch? Thanks.

Changes:
1. Use the grippyhidden atttribute to show/hide the menubar toolbar grippy when in Customize mode.
2. Remove the inline style on the menubar as I think it shouldn't have any effect.
3. Fix typos |oncommand="Browser:Stop"| -> |command="Browser:Stop"|

> Btw, note the Home button and how it affects the toolbar.

4. On toolbox init if a customizable toolbar has a local default{mode|iconsize} and doesn't have user setting, then use the local defaults instead of the toolbox defaults. (e.g. Personal Toolbar should be in small icon mode.)

>  Note also the differences in look when the dialog is opened

Anything in particular? These are (semi)-intentional:

1. Bookmark toolbar items are replaced by a placeholder for DnD.
2. The navigation buttons don't show the disabled state. The toolkit customizeToolbar code removes observers from the buttons in customize mode and we rely on these to set the disabled state.

Comment 43

[Stefan [:stefanh]]

(In reply to comment #42)
> Created an attachment (id=291430) [details]
> Patch 1.3a using grippyhidden to hide the menubar grippy
> 
> Stefan could you test this patch? Thanks.
> 
> Changes:
> 1. Use the grippyhidden atttribute to show/hide the menubar toolbar grippy when
> in Customize mode.

You need to hide the grippy when not in customize mode as well :-) - see the previous screenshots.

> 2. Remove the inline style on the menubar as I think it shouldn't have any
> effect.
> 3. Fix typos |oncommand="Browser:Stop"| -> |command="Browser:Stop"|
> 
> > Btw, note the Home button and how it affects the toolbar.
> 
> 4. On toolbox init if a customizable toolbar has a local default{mode|iconsize}
> and doesn't have user setting, then use the local defaults instead of the
> toolbox defaults. (e.g. Personal Toolbar should be in small icon mode.)

The button is smaller now, but it's still larger than before the patch... Hmm, now when I think of it, it might be that the button is smaller than it should be by default...
> 
> >  Note also the differences in look when the dialog is opened
> 
> Anything in particular? These are (semi)-intentional:
> 
> 1. Bookmark toolbar items are replaced by a placeholder for DnD.
> 2. The navigation buttons don't show the disabled state. The toolkit
> customizeToolbar code removes observers from the buttons in customize mode and
> we rely on these to set the disabled state.

Yeah, you're right. That is, nothing in particlar ;-) Btw, interestingly, the search button label ("Search") has never disappeared when in icons mode only. But with your patches it does.

Comment 44

[Philip Chee]

Attachment: Patch 1.3b When on OSX hide the menubar toolbar-grippy

Stefan could you test this patch? Thanks.

Changes:
1. Hide OSX menubar toolbar-grippy in suite/browser/mac/platformNavigationBindings.xul
2. In Modern the (large size) Home button now respects the active/hover/disabled state.

> You need to hide the grippy when not in customize mode as well :-) - see the
> previous screenshots.

Ooops.

>>> Btw, note the Home button and how it affects the toolbar.

>> 4. On toolbox init if a customizable toolbar has a local default{mode|iconsize}
>> and doesn't have user setting, then use the local defaults instead of the
>> toolbox defaults. (e.g. Personal Toolbar should be in small icon mode.)

I've reverted this addition because I was wrong. The code I added to the toolbox init should not be needed. Looking at my patch. On startup with a fresh profile (or with a default localstore.rdf) The buttons - including the home button - on the Personal Toolbar should come up in small icon mode. I tested this on WinXP and this is working. If it doesn't work in OSX, could you use the DOM inspector to look at the Personal Toolbar. One of the attributes should be |iconsize="small"|.
 
> The button is smaller now, but it's still larger than before the patch... Hmm,
> now when I think of it, it might be that the button is smaller than it should
> be by default...

Pre-patch, in Classic there is some CSS that squeezes the small home button from 19x19 t0 16x16 to match the bookmarks button and bookmark items. Modern has a modified home button that is 22x17.

I could special case the home-button when on the Personal Toolbar but then if you drag another navigation button on to this toolbar, they won't match the home button. Since KaiRo says the slightly larger home button is acceptable, I'll leave it as it is.

> Yeah, you're right. That is, nothing in particlar ;-) Btw, interestingly, the
> search button label ("Search") has never disappeared when in icons mode only.
> But with your patches it does.

Yes I deliberately made the search button respect the button modes.

Comment 45

[Stefan [:stefanh]]

Comment on attachment 291683 [details] [diff] [review]
Patch 1.3b When on OSX hide the menubar toolbar-grippy

Yes, toolbar looks fine now :-)

Some thoughts:

1) Hitting "Restore default Set" in the dialog makes the Home button large.    But I suppose that's how it should be, I can of course choose "small icons" again.

2) There are a few padding/margin issues when you just use the text mode, but I can probably squeeze the css a bit in communicator/mac/toolbar.css once this is settled. I was going to fix pinstripe but it does looks a bit too late for that.

3) If you add a toolbar, but doesn't drop anything in it, it disappears when you click "done". Looks like a toolkit issue/feature, though - same thing happens in Minefield.

4) We might want to think of what to do with the folder icons in the PT because they're not getting replaced by a text label if you choose "text mode". Hmm, but it would be weird if they did...

5) I guess we need a small search-button? well, it is pretty small, but...

Comment 46

[Stefan [:stefanh]]

Additional note:

I'd just like to point out that customizing toolbars is a well-known mac feature that even has a HIG:

http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/index.html?http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_17_section_3.html

That is, we're currently violating the HIG (the only HIG that exists on this, I think). Of course I'm not saying that this should be addressed in this bug - Philip is really doing enough work in here. But imo this should be addressed once this bug is resolved (not necessarily by Philip, of course).

Comment 47

[Stefan [:stefanh]]

> http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/index.html?http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGWindows/chapter_17_section_3.html
> 

Sorry, tha should have been http://developer.apple.com/documentation/UserExperience/Conceptual/OSXHIGuidelines/XHIGMenus/chapter_16_section_4.html#//apple_ref/doc/uid/TP30000356-TPXREF148

Comment 48

[Philip Chee]

(In reply to comment #45)

> 1) Hitting "Restore default Set" in the dialog makes the Home button large.
> But I suppose that's how it should be, I can of course choose "small icons"
> again.

The problem is that toolkit doesn't know that SeaMonkey wants different defaults for some toolbars. I plan to file a bug in the toolkit "restore default set" code to enhance it for SeaMonkey.

> 2) There are a few padding/margin issues when you just use the text mode, but I
> can probably squeeze the css a bit in communicator/mac/toolbar.css once this is
> settled. I was going to fix pinstripe but it does looks a bit too late for
> that.

Sorry about that. I don't have a Mac to test.

> 3) If you add a toolbar, but doesn't drop anything in it, it disappears when
> you click "done". Looks like a toolkit issue/feature, though - same thing
> happens in Minefield.

Yes. Apparently, looking at the code, this is intended behaviour.

> 4) We might want to think of what to do with the folder icons in the PT because
> they're not getting replaced by a text label if you choose "text mode". Hmm,
> but it would be weird if they did...

I'm following Firefox here. I think that the logic is that the folder icons are /not/ toolbar buttons but part of a toolbar item so the button modes/size don't apply.

> 5) I guess we need a small search-button? well, it is pretty small, but...

I think what you mean is that we need a *large* search button. Also a "Go" graphic |> for the Go button when in icon mode - Firefox only has a Go icon with no text.

Comment 49

[Philip Chee]

Comment on attachment 291683 [details] [diff] [review]
Patch 1.3b When on OSX hide the menubar toolbar-grippy

Stefan has confirmed that the outstanding nits on OSX have been resolved. I've spun off Bug 406780 to move OSX to a "customize toolbar sheet". Subsidiary issues are in Bug 406778 and other follow-ups.

Comment 50

[Philip Chee]

Attachment: Patch 1.3c

Via IRC stefanh pointed out to me that:
1) The styles I have in classic/communicator/button.css all ready exist in *stripe/toolbar.css
2) button.css has been forked and cvs copied to classic/communicator/{mac|win}/button.css.

For issue #1: although the styles are in *stripe/toolbar.css it appears that suite doesn't actually reference classic/global/toolbar.css, only classic/communicator/communicator.css

For issue #2: I have moved the styles to classic/communicator/{mac|os2|win}/toolbar.css
and from /modern/communicator/button.css to /modern/communicator/toolbar.css to be consistent, since in *stripe they are in */global/toolbar.css.

Comment 51

[Stefan [:stefanh]]

Comment on attachment 292226 [details] [diff] [review]
Patch 1.3c

+
>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-text,
>+toolbar[mode="text"] .toolbarbutton-1 > stack > toolbarbutton > .toolbarbutton-text {
>+  margin: 4px;
>+}

You can remove those rules from communicator/mac/toolbar.css - they messes up the vertical orientation of some of the toolbarbutton labels. It looks OK without the rules.

Comment 52

[Robert Kaiser]

(In reply to comment #50)
> For issue #1: although the styles are in *stripe/toolbar.css it appears that
> suite doesn't actually reference classic/global/toolbar.css, only
> classic/communicator/communicator.css

AFAIK, toolbar.css is loaded via the XBL binding - though we are using our own toolbar binding for grippies, AFAIK it just does extend the toolkit toolbar binding, which is the one that loads the global toolbar CSS (from *stripe in the default theme).

Comment 53

[Philip Chee]

Attachment: Patch 1.3d Nits fixed (Customizable Toolbars for Navigator)

(In reply to comment #51)
> You can remove those rules from communicator/mac/toolbar.css - they messes up
> the vertical orientation of some of the toolbarbutton labels. It looks OK
> without the rules.

OK. Done.

Comment 54

[Philip Chee]

Comment on attachment 292316 [details] [diff] [review]
Patch 1.3d Nits fixed (Customizable Toolbars for Navigator)

All the CSS is now bit-rotted

Comment 55

[Philip Chee]

Attachment: Patch 2.0 [WIP] bit-rot fixed, css simplified

I'm waiting for Bug 411648 to land then I'll fix the bit-rot but in the meantime I'm putting this WIP patch for review.

Comment 56

[Philip Chee]

Attachment: Patch 2.1 [WIP] bit-rot fixed

I might not continue fixing bit-rot if this goes on much longer.

Comment 57

[Philip Chee]

From #seamonkey:
[22:34:16] <philor> NeilAway: speaking of which, no, I don't have any good ideas for design for a fork, but I do know where a couple of bodies are buried in the current code, lemme look for bug numbers
[22:36:00] <philor> NeilAway: avoiding bug 220701 and bug 364111 from the start would be good

Bug 220701: SeaMonkey uses "hidden" to hide toolbars and I'm persisting the hidden attribute at the moment too.
Bug 364111: Noted.

Comment 58

[Philip Chee]

Attachment: Patch 2.2 [WIP] bit-rot fixed

Updated to tip because apparently some body out there actually wants to use this patch.

Otherwise I am splitting off Bug 428216 for the non customize-toolbar bits so that as least some of the functionality can make it into SM2.0

Comment 59

[Philip Chee]

Attachment: Updated to tip

1. Sync with Bug 428216.
2. Update to tip.

Comment 60

[Philip Chee]

Attachment: Patch against comm-central tip

Now that we have abandoned CVS, this patch is against the comm-central HG repository updated to tip.

Comment 62

[Philip Chee]

http://www.hirsch.sth.ac.at/~robert/thebot-logs/%23seamonkey-20090202-120003.xml

[09:10:29]
<lleroy>
after upgrading to the latest seamonkey2.0a3pre the address bar is gone... how do I get it back? (just 'back', 'forward', 'reload' and 'stop' are left over)

Comment 63

[Philip Chee]

http://www.hirsch.sth.ac.at/~robert/thebot-logs/%23seamonkey-20090202-120003.xml

[09:10:29] <lleroy> after upgrading to the latest seamonkey2.0a3pre the address bar is gone... how do I get it back? (just 'back', 'forward', 'reload' and 'stop' are left over)

User needs to customize->reset toolbars to defaults. Setting relnote keyword.

Comment 64

[Philip Chee]

Marking as fixed as all the dependencies are fixed.

Comment 65

[Biju]

"Menu Bar" and "Site Navigation bar" still not customizable.
Bug 479004 created

Comment 68

[Robert Kaiser]

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