471372 - Customizable Toolbars in Navigator (Part 2)

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Philip Chee]

+++ This bug was initially created as a clone of Bug #428216 +++

Continuing on from Bug 394288 since that bug has gone on for much too long. Patch coming up.

Comment 1

[Philip Chee]

Attachment: Patch v1.0

This patch turns on customizable toolbars completely in navigator. As far as I can tell I've caught all the obvious regressions so asking for first review.

TODO:

1. Collapsed grippy mode of custom toolbars need to be persisted.

2. Currently the customize toolbars command depends on the existence of a "getNavToolbox()" function in the current scope which works for Firefox and for Thunderbird (which uses getMailToolbox()" for all its windows). I suppose we could put a "gToolbox", or "gMainToolbox", or  even just reuse "gNavToolbox" in each window with a customizable toolbar.

2a. Or we could add a parameter to goCustomizeToolbar() e.g. goCustomizeToolbar(toolbox) and move this from utilityOverlay.xul:

+    <command id="cmd_CustomizeToolbars"
+             oncommand="goCustomizeToolbar()"/>

to each specific window.

3. I need to file a toolkit bug to enhance the toolboxChanged callback to return the actual change made including the "reset to defaults" action. We need this so that we can reset our custom modes (e.g. "labelalign") which toolkit doesn't know about.

Comment 2

[Stefan [:stefanh]]

I understand that you want to do the win/nix part first, but do you plan to fix the mac part in bug 406780 once this have landed (I'm just worried that we end up shipping a broken feature on mac).

Comment 3

[Stefan [:stefanh]]

(add a "?")

Comment 4

[Philip Chee]

Depends on someone buying me a Mac, although I might be able to fake something on WinXP if someone on a Mac volunteers to test my patches.

<rant>Also I think Firefox is epic fail by forking off customizeToolbarSheet.* into browser/ instead of leaving it in toolkit/ where other applications could use it, thus forcing Thunderbird and Sunbird to do the same. If we go the same route there would then be four subtly different versions floating around in the tree.</rant>

Phil

Comment 5

[Phil Ringnalda (:philor)]

Your rant is incorrect.

Comment 6

[Philip Chee]

Oh, it Thunderbird/Sunbird building with the firefox customizeToolbarSheet* stuff now? I guess I'm out of date then. Sorry.

Comment 7

[Phil Ringnalda (:philor)]

Not that part, the assertion that Mano could have and should have landed what he considered a quick dirty hack to make things at least a little better very shortly before a Firefox release in toolkit rather than browser, and that even if he couldn't have landed it in toolkit at the time, or if it was actually a mistake to land it in browser, that he and only he, or only a Firefox developer, has the responsibility to move it to toolkit. He didn't owe me that code then, he doesn't owe you that code now. I'm free to move it to toolkit if I want, you're free to move it there if you want, he's free to not move it there if he doesn't want. If you do the work to write a patch to move it, and a browser peer refuses to accept the patch without giving any reason, then and only then do you have something to rant about, but complaining that someone didn't give you something for free two and a half years before you were even ready to have it is silly and inappropriate.

Comment 8

[Philip Chee]

Thanks for the history. I didn't realize that it was a last minute hack to get it in under a deadline.

Comment 9

[Philip Chee]

Attachment: Patch v1.1

Changes since the last patch:

1. Collapsed grippy mode of custom toolbars are persisted.
2. Remove dependency on getNavToolbox() in utilityOverlay.js. Now we pass the toolbox as a parameter to all those *customize* functions that need it.
3. Reset our custom toolbar attributes when the toolkit customize toolbar "Reset to Defaults" button is pressed. This depends on toolkit Bug 471508.

TODO:

1. If a custom toobar is collapsed using the toolbar grippy then removed via the customize toolbar window then there is a dangling non-functional collapsed grippy until that window is closed and re-opened. I think I'll just make sure all toolbars are un-collapsed while in customize mode.

Phil

Comment 10

[Philip Chee]

Attachment: Patch v1.2

> 1. If a custom toobar is collapsed using the toolbar grippy then removed via
> the customize toolbar window then there is a dangling non-functional collapsed
> grippy until that window is closed and re-opened.

I decided not to do this in this bug because this is an edge case that can only occur if you grippy-collapse an empty custom toolbar while the customize window is open. On exit the empty custom toolbar is removed leaving a dangling non functional collapsed grippy. But this disappears when you re-open the navigator window so there is no dataloss nor permanent UI blemish. If necessary I will file a follow-up polish bug on this issue.

Changes since the last patch:

Moved behavioural CSS from themes to content/communicator.css. These styles just wallpaper over bug 472302, which hasn't landed on 1.9.1 /yet/.

+/* XXXRatty: wallpaper customize toolbar bug 402573 and bug 472302.
+   Remove these two styles once the underlying bugs are fixed on 1.9.1. */
+#BrowserToolbarPalette > #urlbar-container > #nav-bar-inner > #urlbar {
+  -moz-binding: none;
+}
+
+#wrapper-urlbar-container > #urlbar-container> #nav-bar-inner > #urlbar {
+  -moz-user-input: disabled;
+}

Comment 11

[Philip Chee]

Ooops. You also need to apply attachment (id=356515) from bug 471508 if you want to test that the navigator toolbars respond correctly to the "reset to defaults" button in the customize toolbars window.

Comment 12

[Robert Kaiser]

(In reply to comment #10)
> If necessary I will
> file a follow-up polish bug on this issue.

Please do that, but it sounds reasonable to leave this out of the patch here, it's already large enough without that.

Comment 13

[Philip Chee]

Attachment: Patch v1.3

> +/* XXXRatty: wallpaper customize toolbar bug 402573 and bug 472302.
> +   Remove these two styles once the underlying bugs are fixed on 1.9.1. */

The fix for Bug 472302 has landed on mozilla-1.9.1 so removing workarounds.

Comment 14

[[e-mail]]

Comment on attachment 356924 [details] [diff] [review]
Patch v1.3

>+function BrowserToolboxCustomizeDone(aToolboxChanged)
Why do we lose the page proxy state?

>+             defaultset="menubar-items"
Are you supposed to include static items in the default set?

>+      <toolbaritem id="urlbar-container" align="center" flex="1" persist="width"
>+                   class="chromeclass-location"
>+                   title="&locationBar.title;">
>+        <hbox id="nav-bar-inner" flex="1">
Why not just <toolbaritem id="nav-bar-inner" flex="1" title="&locationBar.title;">?

>+                     template="bookmarksMenuTemplate"
>                      oncommand="BookmarksMenu.loadBookmark(event, this.database);"
>                      onclick="BookmarksMenu.loadBookmarkMiddleClick(event, this.database)" 
>-                     template="bookmarksMenuTemplate"
Moved unnecessarily.

>+        <stack id="bookmarks-stack" flex="1" style="min-width:0px; width:0px;"
I'm not convinced that this stack is necessary.

>+    <command id="cmd_CustomizeToolbars" oncommand="goCustomizeToolbar(getNavToolbox())"/>
Nit: missing semicolon.

>-    this.stopButton      = document.getElementById("stop-button");
>-    this.stopMenu        = document.getElementById("menuitem-stop");
>+    this.stopCommand     = document.getElementById("Browser:Stop");
Why this (and related) change?

>+      <property name="palette">
<field name="palette">this.getElementsByTagName("toolbarpalette").item(0)</field>

>+  // Re-enable parts of the UI we disabled during the dialog
>+  var menubar = document.getElementById(menubarID);
>+  for (let i = 0; i < menubar.childNodes.length; ++i)
>+    menubar.childNodes[i].setAttribute("disabled", false);
What if the menu was already disabled before customising?

>+    persistCustomToolbar(toolbars[i]);
What can the customise toolbar dialog forget to persist?

>+function isElementVisible(aElement)
>+{
>+  if (!aElement)
>+    return false;
Don't need to check this any more.

>-toolbar[iconsize="small"] > #back-button {
>+toolbar[iconsize="small"] #back-button {
Why these changes?

>+toolbarpaletteitem[dragover="left"] {
Where is this used?

Comment 15

[[e-mail]]

(In reply to comment #14)
>(From update of attachment 356924 [details] [diff] [review])
>>+  // Re-enable parts of the UI we disabled during the dialog
>>+  var menubar = document.getElementById(menubarID);
>>+  for (let i = 0; i < menubar.childNodes.length; ++i)
>>+    menubar.childNodes[i].setAttribute("disabled", false);
>What if the menu was already disabled before customising?
FYI Message Compose does this.

Comment 16

[[e-mail]]

Comment on attachment 356924 [details] [diff] [review]
Patch v1.3

> function ShowAndSelectContentsOfURLBar()
> {
>-  var navBar = document.getElementById("nav-bar");
>-  
>-  // If it's hidden, show it.
>-  if (navBar.getAttribute("hidden") == "true")
>-    goToggleToolbar('nav-bar','cmd_viewnavbar');
This stuff never really worked properly (grippies, popups etc.) Just launch the dialog if the urlbar is hidden.

>+  // Update the urlbar
>+  var value = gBrowser.userTypedValue;
>+  var valid = false;
>+  var uri = getWebNavigation().currentURI;
>+
>+  if (!value) {
This is wrong, because the empty string may have been typed.

>+  min-height: 19px;
I think 22px works better.

>+menubar > .toolbar-box > .toolbar-holder,
>+toolbar[type="menubar"] > .toolbar-box > .toolbar-holder  {
>   border-top: 1px solid #EBF4FF;
>   border-right: 1px solid #86929E;
>   border-bottom: 1px solid #B9BFC7;
>   border-left: 1px solid #EEF4FC;
> }  
>+
>+toolbar > toolbaritem > menubar > .toolbar-box > .toolbar-holder {
>+  border: 0px none;
>+}
The menubar theming doesn't look quite right, even in Modern.
(It looks obviously wrong in Classic.)

>+.toolbarpaletteitem-box[type="spacer"],
>+.toolbarpaletteitem-box[type="spring"] {
>+  border: 1px solid #808080;
>+  background-color: #FFF !important;
I was thinking it looked garish. Maybe a light colour or some opacity?

>+toolbarpaletteitem[place="toolbar"] {
>+  margin-left: -2px;
>+  margin-right: -2px;
>+  border-left: 2px solid transparent;
>+  border-right: 2px solid transparent;
[What does this achieve? I guess it's just copied from Classic.]

>+.toolbarpaletteitem-box[type="spacer"][place="toolbar"],
>+.toolbarpaletteitem-box[type="spring"][place="toolbar"] {
>+  margin: 2px 2px 2px 0px;
[And this.]

>+toolbar[mode="text"] #urlbar-container > #nav-bar-inner,
>+toolbar[iconsize="small"] #urlbar-container > #nav-bar-inner {
>+  margin: 0 !important;
>+  border: none !important;
>+}
Why only one > ? (But might be obsoleted by fixing a previous comment.)

>+#wrapper-personal-bookmarks .toolbarpaletteitem-box {
>+  width: 16px;
>+  height: 16px;
What does this do?

>+.bookmarks-toolbar-customize > .toolbarbutton-text {
>+  text-align: left;
What's this overriding?

>+.bookmarks-toolbar-customize > .toolbarbutton-icon {
>+  background: url("chrome://communicator/skin/bookmarks/bookmark-folder-open.gif") no-repeat !important;
Why a background rather than simply inheriting it as a list-style-image?

Comment 17

[Philip Chee]

Attachment: Patch v1.5

>(From update of attachment 356924 [details] [diff] [review])
>>+function BrowserToolboxCustomizeDone(aToolboxChanged)
>Why do we lose the page proxy state?

I've removed all the calls to SetPageProxyState() so now we shouldn't.

>>+             defaultset="menubar-items"
>Are you supposed to include static items in the default set?

Yes. Or rather the resetToolbars code in toolkit expects a non-null defaultset and will skip this toolbar if empty or missing. The result is custom buttons still stuck on the menu toolbar after resetting.

>>+      <toolbaritem id="urlbar-container" align="center" flex="1" persist="width"
>>+                   class="chromeclass-location"
>>+                   title="&locationBar.title;">
>>+        <hbox id="nav-bar-inner" flex="1">
>Why not just <toolbaritem id="nav-bar-inner" flex="1"
>title="&locationBar.title;">?

For compatibility with Firefox extensions that look for "urlbar-container".

>>+                     template="bookmarksMenuTemplate"
>>                      oncommand="BookmarksMenu.loadBookmark(event, this.database);"
>>                      onclick="BookmarksMenu.loadBookmarkMiddleClick(event, this.database)" 
>>-                     template="bookmarksMenuTemplate"
>Moved unnecessarily.

Fixed.

>>+        <stack id="bookmarks-stack" flex="1" style="min-width:0px; width:0px;"
>I'm not convinced that this stack is necessary.

Removed. I had to move things around slightly but I've also removed some styles that I copied from Firefox that only existed to workaround problems caused by the presence of the stack (see later)

>>+    <command id="cmd_CustomizeToolbars" oncommand="goCustomizeToolbar(getNavToolbox())"/>
>Nit: missing semicolon.

Fixed.

>>-    this.stopButton      = document.getElementById("stop-button");
>>-    this.stopMenu        = document.getElementById("menuitem-stop");
>>+    this.stopCommand     = document.getElementById("Browser:Stop");
>Why this (and related) change?

(I've simplified the XUL slightly since so here for reference:)
<toolbarbutton id="stop-button"
               class="toolbarbutton-1 chromeclass-toolbar-additional"
               label="&stopButton.label;"
               observes="Browser:Stop"
               tooltiptext="&stopButton.tooltip;">

<command id="Browser:Stop"    oncommand="BrowserStop();"    disabled="true"/>

<menuitem label="&stopCmd.label;"
          accesskey="&stopCmd.accesskey;"
          id="menuitem-stop"
          observes="Browser:Stop"
          key="key_stop">

OK. Problem #1 (this also exists in 1.1 and probably goes back to Mozilla Suite). On opening the navigator window the stop button is active and doesn't start to synchronize until you start navigating somewhere. To fix this I made the stop button observe a static element with an initial state of disabled="true" and then I pointed the sync code in nsBrowserStatusHandler.js at this instead of the button. I used "Browser:Stop" for compatibility with Firefox. In the stop menu item I use an observes instead of a command attribute because I noticed a UI discrepancy that can occur under certain circumstances. For example:

1. Click in a link to start a page loading.
2. Open the View menu.
3. The Stop menu item is active.
4. The page completes loading.
5. The Stop menu item is still active until you reopen the View menu.

I use an observes element in the stop button for another reason as well. The toolkit customize code removes and adds back command attributes everytime toolbar items are wrapped/unwrapped. This causes bug 309953 which is wallpapered in Firefox 3.0 with some javascript by bug 287105.

>>+      <property name="palette">
><field
>name="palette">this.getElementsByTagName("toolbarpalette").item(0)</field>

Fixed.

>>+  // Re-enable parts of the UI we disabled during the dialog
>>+  var menubar = document.getElementById(menubarID);
>>+  for (let i = 0; i < menubar.childNodes.length; ++i)
>>+    menubar.childNodes[i].setAttribute("disabled", false);
>What if the menu was already disabled before customising?
>FYI Message Compose does this.

Fixed.

>>+    persistCustomToolbar(toolbars[i]);
>What can the customise toolbar dialog forget to persist?

For custom, dynamically created toolbars, the toolkit customize code doesn't actually persist anything, even those attributes that it knows about (size and mode).

>>+function isElementVisible(aElement)
>>+{
>>+  if (!aElement)
>>+    return false;
>Don't need to check this any more.

Fixed.

>>-toolbar[iconsize="small"] > #back-button {
>>+toolbar[iconsize="small"] #back-button {
>Why these changes?

Behavioural compatibility with Firefox where when the toolbar items are wrapped in customize mode, the icon size and mode don't change. With a direct child selector in SeaMonkey if, say, you have your toolbar in small/icon mode, opening the customize window causes the buttons to change to large/icon+text due to an intermediate wrapper element around all the buttons.

>>+toolbar[mode="text"] #urlbar-container > #nav-bar-inner,
>>+toolbar[iconsize="small"] #urlbar-container > #nav-bar-inner {
>>+  margin: 0 !important;
>>+  border: none !important;
>>+}
>Why only one > ? (But might be obsoleted by fixing a previous comment.)

Same reason.

>>+toolbarpaletteitem[dragover="left"] {
>Where is this used?

The last item on a toolbar; and for RTL windows, all the items except the last item.

>>-  // If it's hidden, show it.
>>-  if (navBar.getAttribute("hidden") == "true")
>>-    goToggleToolbar('nav-bar','cmd_viewnavbar');
>This stuff never really worked properly (grippies, popups etc.) Just launch the
>dialog if the urlbar is hidden.

Fixed.

>>+  // Update the urlbar
>>+  var value = gBrowser.userTypedValue;
>>+  var valid = false;
>>+  var uri = getWebNavigation().currentURI;
>>+
>>+  if (!value) {
>This is wrong, because the empty string may have been typed.

Fixed.

>>+  min-height: 19px;
>I think 22px works better.

Fixed.

>>+menubar > .toolbar-box > .toolbar-holder,
>>+toolbar[type="menubar"] > .toolbar-box > .toolbar-holder  {
>>   border-top: 1px solid #EBF4FF;
>>   border-right: 1px solid #86929E;
>>   border-bottom: 1px solid #B9BFC7;
>>   border-left: 1px solid #EEF4FC;
>> }  
>>+
>>+toolbar > toolbaritem > menubar > .toolbar-box > .toolbar-holder {
>>+  border: 0px none;
>>+}
>The menubar theming doesn't look quite right, even in Modern.

Sorry, it looks fine over here. Anything specific? screenshots would be appreciated.

>(It looks obviously wrong in Classic.)

I didn't change anything here. The theming in Classic comes from toolkit.

>>+.toolbarpaletteitem-box[type="spacer"],
>>+.toolbarpaletteitem-box[type="spring"] {
>>+  border: 1px solid #808080;
>>+  background-color: #FFF !important;
>I was thinking it looked garish. Maybe a light colour or some opacity?

Fixed using a background colour of #F0F0F0. Opacity doesn't work well, because the flexible spring has a graphic.
I forgot to un-bitrot my patch and change spring.gif => spring.png in Modern. Fixed.

>>+toolbarpaletteitem[place="toolbar"] {
>>+  margin-left: -2px;
>>+  margin-right: -2px;
>>+  border-left: 2px solid transparent;
>>+  border-right: 2px solid transparent;
>[What does this achieve? I guess it's just copied from Classic.]

Removed. Yes I copied these from Classic.

>>+.toolbarpaletteitem-box[type="spacer"][place="toolbar"],
>>+.toolbarpaletteitem-box[type="spring"][place="toolbar"] {
>>+  margin: 2px 2px 2px 0px;
>[And this.]

Removed.

Following three styles are removed. After removing the stack these are not needed.

>>+#wrapper-personal-bookmarks .toolbarpaletteitem-box {
>>+  width: 16px;
>>+  height: 16px;
>What does this do?
>
>>+.bookmarks-toolbar-customize > .toolbarbutton-text {
>>+  text-align: left;
>What's this overriding?
>
>>+.bookmarks-toolbar-customize > .toolbarbutton-icon {
>>+  background: url("chrome://communicator/skin/bookmarks/bookmark-folder-open.gif") no-repeat !important;
>Why a background rather than simply inheriting it as a list-style-image?

Fini.

Comment 18

[Philip Chee]

>>>+    persistCustomToolbar(toolbars[i]);
>>What can the customise toolbar dialog forget to persist?

>For custom, dynamically created toolbars, the toolkit customize code doesn't
>actually persist anything, even those attributes that it knows about (size and
>mode).

See Bug 220701 (UNFIXED) for similar issues in Firefox.

Comment 19

[[e-mail]]

(In reply to comment #17)
>Patch v1.5
No v1.4?

>>>+      <toolbaritem id="urlbar-container" align="center" flex="1" persist="width"
>>>+                   class="chromeclass-location"
>>>+                   title="&locationBar.title;">
>>>+        <hbox id="nav-bar-inner" flex="1">
>>Why not just <toolbaritem id="nav-bar-inner" flex="1"
>>title="&locationBar.title;">?
>For compatibility with Firefox extensions that look for "urlbar-container".
Yes, and they'll expect the urlbar to be contained in the urlbar-container. So we should still remove the hbox, we just need to decide whether the toolbaritem's id should be urlbar-container or nav-bar-inner.

> I had to move things around slightly but I've also removed some styles
> that I copied from Firefox that only existed to workaround problems caused by
> the presence of the stack (see later)
Now that's what I like to hear!

> OK. Problem #1 (this also exists in 1.1 and probably goes back to Mozilla
> Suite). On opening the navigator window the stop button is active and doesn't
> start to synchronize until you start navigating somewhere.
Since when was this bug called "Stop button enabled in new blank window"? It really deserves a separate bug where we can decide whether to use a broadcaster or simply add disabled="true" on the appropriate elements.

>>>-toolbar[iconsize="small"] > #back-button {
>>>+toolbar[iconsize="small"] #back-button {
>>Why these changes?
>Behavioural compatibility with Firefox where when the toolbar items are wrapped
>in customize mode, the icon size and mode don't change. With a direct child
>selector in SeaMonkey if, say, you have your toolbar in small/icon mode,
>opening the customize window causes the buttons to change to large/icon+text
>due to an intermediate wrapper element around all the buttons.
That sucks. But then, we already knew that the customise toolbar code sucks.

Since we know what the items will be wrapped in, I guess toolbar[iconsize="small"] > toolbarpaletteitem > #back-button would also work.

>>>+toolbarpaletteitem[dragover="left"] {
>>Where is this used?
>The last item on a toolbar; and for RTL windows, all the items except the last item.
Sorry, that was a silly question.

Comment 20

[Stefan [:stefanh]]

Comment on attachment 357928 [details] [diff] [review]
Patch v1.5

There is one mac-specific issue with this patch: On mac the menubar is hidden, but since you now have the menubar wrapped in a toolbar, there's a 9px high "empty toolbar" above the .nav-bar on mac .

Comment 21

[Philip Chee]

> That sucks. But then, we already knew that the customise toolbar code sucks.
> Since we know what the items will be wrapped in, I guess
> toolbar[iconsize="small"] > toolbarpaletteitem > #back-button would also work.

Does this mean I have to do this for every toolbar button and item? This sounds equally sucky

> There is one mac-specific issue with this patch: On mac the menubar is hidden,
> but since you now have the menubar wrapped in a toolbar, there's a 9px high
> "empty toolbar" above the .nav-bar on mac .

Sorry I forgot about Bug 394288 Comment ##44 where I fixed this in suite/browser/mac/platformNavigationBindings.xul

Other TODO stuff before I forget:

1. menubar themeing in both classic and modern. This is Windows XP Classic Theme specific. You won't see this in the default XP Theme.
2. s/place=palette/place="palette"/g
3. Fix DOS line endings.
4. <Neil@irc> ok, so the "extra" label comes from the <toolbaritem id="personal-bookmarks" title="&bookmarksToolbarItem.label;" You could instead put in some CSS so that when the button is in the palette its orient is vertical so the text appears below the icon

Comment 22

[[e-mail]]

(In reply to comment #21)
> > There is one mac-specific issue with this patch: On mac the menubar is hidden,
> > but since you now have the menubar wrapped in a toolbar, there's a 9px high
> > "empty toolbar" above the .nav-bar on mac .
> Sorry I forgot about Bug 394288 Comment ##44 where I fixed this in
> suite/browser/mac/platformNavigationBindings.xul
You're going to need a solution that works in all customisable windows.

Comment 23

[Philip Chee]

> You're going to need a solution that works in all customisable windows.
I realized this just after hitting "Commit" :(

Comment 24

[Philip Chee]

Possibilities:
#1. Put the following in content/communicator.css
%ifdef XP_MACOSX
toolbar[type="menubar"] {
  display: none;
}
%endif

#2. Add (or import) the styles from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/help/content/platformClasses.css

and then add a class "noMac" to our toolbar menubars.

In either case some file or other will have to end up pre-processed.

Comment 25

[Philip Chee]

>> That sucks. But then, we already knew that the customise toolbar code sucks.
>> Since we know what the items will be wrapped in, I guess
>> toolbar[iconsize="small"] > toolbarpaletteitem > #back-button would also work.

> Does this mean I have to do this for every toolbar button and item? This sounds
> equally sucky

And then we would have to do this for mailnews and every other window that we want to have customizable toolbars.

Comment 26

[[e-mail]]

(In reply to comment #25)
>>> That sucks. But then, we already knew that the customise toolbar code sucks.
>>> Since we know what the items will be wrapped in, I guess
>>> toolbar[iconsize="small"] > toolbarpaletteitem > #back-button would also work.
>> Does this mean I have to do this for every toolbar button and item? This sounds
>> equally sucky
> And then we would have to do this for mailnews and every other window that we
> want to have customizable toolbars.
As distinct from removing the ">" for mailnews and every other window?

Comment 27

[Stefan [:stefanh]]

(In reply to comment #24)
> Possibilities:
> #1. Put the following in content/communicator.css
> %ifdef XP_MACOSX
> toolbar[type="menubar"] {
>   display: none;
> }
> %endif
> 
> #2. Add (or import) the styles from
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/help/content/platformClasses.css
> 
> and then add a class "noMac" to our toolbar menubars.
> 

If I had to pick either #1 or #2, I'd go for #1. Being dependent on toolkit help file css doesn't seems like a good idea. fwiw, the toolbar is taken care of in http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#230. Is there any way to have the grippy on the menubar instead? Just a thought.

Comment 28

[Philip Chee]

Attachment: Patch v1.6

Changes since the last patch.

1. Renamed "urlbar-container" to "nav-bar-inner" and removed the redundant hbox.

2. Reverted all changes to the stop button and stop menu.

3. Added extra rules to classic and modern so the toolbar buttons don't change mode/size when wrapped by the customize toolbar code.

4. Removed the extra label on the personal bookmarks item and put in some CSS so that when the button is in the palette its orient is vertical and the label appears below the icon.

5. Fixed the menubar theme problem in classic. On code inspection only winstripe adds a bottom 1px border so I put a style in communicator/win/toolbar.css to override this.

6. Fixed the menubar grippy theme problem in modern by adding a rule for toolbar[type="menubar"] to the menubar style.

7. s/place=palette/place="palette"/g

8. Fix DOS ^M line endings.

Comment 29

[[e-mail]]

Comment on attachment 358398 [details] [diff] [review]
Patch v1.6

To clear up any misunderstanding: to avoid the broken Mac "menubar", I meant to suggest that the menubar changes could be backed out for now.

>-        <label control="urlbar" hidden="true" value="&locationBar.title;"/>
...
>+        <label control="urlbar" hidden="true" value="&locationBar.title;"/>
/me wishes diff was a little cleverer...

>-toolbar[iconsize="small"] .toolbarbutton-1 {
>+toolbar[iconsize="small"] .toolbarbutton-1,
>+toolbar[iconsize="small"] > toolbarpaletteitem > .toolbarbutton-1 {
You've got a bunch of these where you could have prepended the new rule to simplify this edit.

>-#PersonalToolbar[iconsize="small"] > #home-button > .toolbarbutton-icon {
>+#PersonalToolbar[iconsize="small"] #home-button > .toolbarbutton-icon {
Sorry, but it looks like you didn't catch them all...

>+toolbarpaletteitem[place] .bookmarks-toolbar-customize {
>+  display: -moz-box !important;
Does this need to be important? Also, why not a > in this group of rules?

>+toolbarpaletteitem[place="toolbar"] .bookmarks-toolbar-overflow-items,
>+toolbarpaletteitem[place="toolbar"] .bookmarks-toolbar-items {
>+  visibility: hidden;
I'd prefer collapse if it works.

Comment 30

[Philip Chee]

Attachment: Patch v1.7

> (From update of attachment 358398 [details] [diff] [review])
> To clear up any misunderstanding: to avoid the broken Mac "menubar", I meant to
> suggest that the menubar changes could be backed out for now.

Fixed.

>> -toolbar[iconsize="small"] .toolbarbutton-1 {
>> +toolbar[iconsize="small"] .toolbarbutton-1,
>> +toolbar[iconsize="small"] > toolbarpaletteitem > .toolbarbutton-1 {
> You've got a bunch of these where you could have prepended the new rule to
> simplify this edit.

Fixed.
 
>> -#PersonalToolbar[iconsize="small"] > #home-button > .toolbarbutton-icon {
>> +#PersonalToolbar[iconsize="small"] #home-button > .toolbarbutton-icon {
> Sorry, but it looks like you didn't catch them all...

Fixed.
 
>> +toolbarpaletteitem[place] .bookmarks-toolbar-customize {
>> +  display: -moz-box !important;
> Does this need to be important?

No. Removed.

>  Also, why not a > in this group of rules?

Fixed.

>> +toolbarpaletteitem[place="toolbar"] .bookmarks-toolbar-overflow-items,
>> +toolbarpaletteitem[place="toolbar"] .bookmarks-toolbar-items {
>> +  visibility: hidden;
> I'd prefer collapse if it works.

Fixed.

Comment 31

[[e-mail]]

Comment on attachment 358618 [details] [diff] [review]
Patch v1.7

>diff --git a/suite/themes/modern/navigator/navigator.css b/suite/themes/modern/navigator/navigator.css

>-toolbar[iconsize="small"] .toolbarbutton-1 {
>+toolbar[iconsize="small"] > .toolbarbutton-1,
>+toolbar[iconsize="small"] > toolbarpaletteitem > .toolbarbutton-1 {
...
>-toolbar[iconsize="small"] #print-button[disabled="true"] {
>+toolbar[iconsize="small"] > #print-button[disabled="true"],
>+toolbar[iconsize="small"] > toolbarpaletteitem > 

>+toolbar[iconsize="small"] > toolbarpaletteitem > #home-button,
> toolbar[iconsize="small"] > #home-button {
Thanks for putting in the >s on all the primary toolbar buttons but for consistency I'd like the rules swapped around to match the other buttons.

Comment 32

[[e-mail]]

Pushed changeset d4ff0342ca4e to comm-central.