428216 - toolbarbutton iconsize/mode control (Split from Bug 394288)

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Philip Chee]

Since Bug 394288 has hit an impasse, I'm splitting the non-customize bits off into a separate bug as suggested by Neil.

Comment 1

[Philip Chee]

Attachment: WIP Patch v 0.1.1

WIP Patch.

1. Split offf everything from Bug 394288 that doesn't actually involve moving toolbarbuttons around.

2. Extend the implementation of buttonmode+iconsize plus UI from Navigator to the main MailNews window (may also work in the message window).

TODO:

1. In modern the dropmarkers tend to overlap the button labels when in small icon mode.

2. onViewToolbarCommand() needs a better implementation without having to hard code the associated menu item that needs to be toggled.

3. Misc CSS polish and fine tuning.

Comment 2

[Philip Chee]

Attachment: Patch v 0.1.3

Asking for review for this patch as everything seems to be working.

> the main MailNews window (may also work in the message window).

Tested the messageWindow.

> 1. In modern the dropmarkers tend to overlap the button labels when in small
> icon mode.
Fixed

> 2. onViewToolbarCommand() needs a better implementation without having to hard
> code the associated menu item that needs to be toggled.
Fixed

> 3. Misc CSS polish and fine tuning.
Done.

Comment 3

[Stefan [:stefanh]]

Comment on attachment 322603 [details] [diff] [review]
Patch v 0.1.3

Just passed by and noticed this small nit:

===================================================================
RCS file: /cvsroot/mozilla/suite/themes/classic/navigator/navigator.css,v
.
.
. +/* XXXRatty workaround bacause we don't have small icons yet */

Comment 4

[Philip Chee]

> . +/* XXXRatty workaround bacause we don't have small icons yet */
Ooops.

Comment 5

[simon anderson]

Blocks 15799? Is that right? I wonder if that should be some other bug: 15322 maybe?

Comment 6

[Philip Chee]

Oops should be Bug 157199. Sorry

Comment 7

[Philip Chee]

Attachment: Patch v 0.1.4

Changes in this patch:

1. Yet /more/ modern tweaks; e.g. the positioning of drop markers in the Forward/Backward/Print buttons are special cased so I have to replicate these rules for smallicons/icon-mode etc.

2.Added UI to toggle the "Show text beside icon" mode.

Comment 8

[Philip Chee]

Attachment: Patch v 0.1.5

> -.toolbarbutton-1 > .toolbarbutton-text {
> -  min-width: 50px;
> +toolbarbutton .toolbarbutton-text {
> +  min-width: 33px;
>  }

Ooops this braino caused a regression in modern.

Comment 9

[Philip Chee]

Attachment: Patch v 0.1.6

1. Removed some tab characters that somehow got into the patch.
2. some minor CSS tweaks for Modern.

Comment 10

[[e-mail]]

This patch seems to have some unrelated (though possibly worthwhile) changes?

Comment 11

[Philip Chee]

> This patch seems to have some unrelated (though possibly worthwhile) changes?

They are not unrelated. I followed your advice with Bug 394288 and split absolutely everything from that bug the didn't actually involve actually moving buttons around in the DOM. Since in the original bug I touched code handling [1] the urlbar (including the page-proxy-fav-icon) and the [2] Stop Button (fixed some bugs). These bits came along.

I would absolutely hate to split more bits off into separate bugs (it might be easier with hg) since I'm actually beginning to forget why I made some of the changes (especially from one year ago).

Comment 12

[Philip Chee]

NB: The page-proxy-favicon bits came from Firefox 2.x, Firefox 3.0 seems to have totally rewritten it and I can't understand the new code except that it seems to be connected to either Larry or the Awesomebar[TM].

Comment 13

[[e-mail]]

Comment on attachment 324728 [details] [diff] [review]
Patch v 0.1.6

>+function getMailToolbox() /* Thunderbird compatibility */
>+{
>+  getMainToolbox();
>+}
>+
>+function getMainToolbox()
>+{
>+  if (!gMailToolbox)
>+    gMailToolbox = document.getElementById("mail-toolbox");
>+  return gMailToolbox;
>+}
This looks weird - getMailToolbox calls getMainToolbox to init gMailToolbox (not that you need getMainToolbox, see below, so this should be separated.)

>+           grippytooltiptext="&mailToolbar.tooltip;"
>+           toolbarname="&showMessengerToolbarCmd.label;"
Strange, I wonder why we have these apparently duplicate entities.

>-function getNavToolbox()
>+function getNavToolbox() /* Firefox compatibility */
???

>+          if (toolbarMode != "text") {
Firefox compatibility?

>+          // XXX See bug 202978: we disable the context menu
>+          // to prevent customization while in fullscreen, which
>+          // causes menu breakage.
>+          els[i].setAttribute("saved-context",
>+                              els[i].getAttribute("context"));
>+          els[i].removeAttribute("context");
I was thinking about cancelling the popupshowing event, but maybe not.

>-  SetPageProxyState("invalid", null);
>+  SetPageProxyState("invalid");
I'd have to read the page proxy stuff *very* carefully. And separately.

>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>+           mode="icons" defaultmode="icons">
Surely the default mode is both icons and text?

>-                       observes="canGoBack" context="backMenu"
>+                       context="backMenu"
>                        tooltiptext="&backButton.tooltip;">
>+          <observes element="canGoBack" attribute="disabled"/>
Why this (and similar) changes?

>-      <hbox id="throbber-box" align="center">
>+      <toolbaritem id="throbber-box" align="center" pack="center">
Why the pack?

>-      <toolbarbutton id="home-button" class="bookmark-item"
>+      <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
Why this change? (Well, toolbarbutton-1 I can sort of understand...)

>Index: suite/browser/navigatorDD.js
These changes seem unrelated.

>       // XXXjag: <command id="cmd_stop"/> ?
>-      this.stopButton.disabled = true;
>-      this.stopMenu.setAttribute('disabled', 'true');
>-      this.stopContext.setAttribute('disabled', 'true');
>+      // XXXRatty: now using <command id="Browser:Stop"/>
>+      this.stopCommand.setAttribute("disabled", "true");
Then you can remove jag's comment! (but ideally separate out Browser:Stop)

>+    var level = null;
Unrelated change.

>-    <key id="key_stop2" keycode="VK_ESCAPE" oncommand="BrowserStop();"/>
>-    <key id="key_stop" key="." oncommand="BrowserStop();" modifiers="accel"/>
>+    <key id="key_stop2" keycode="VK_ESCAPE" command="Browser:Stop"/>
>+    <key id="key_stop" key="." command="Browser:Stop" modifiers="accel"/>
These (and similar) changes aren't right; ESC needs to work on a loaded page.

>+function onViewToolbarsPopupShowing(aEvent)
>+{
>+  var popup = aEvent.target;
>+  var i;
>+
>+  // Empty the menu
I'm not convinced that this needs to be dynamic.

>+      menuItem.setAttribute("checked", toolbar.getAttribute("hidden") != "true");
!toolbar.hidden

>+  var mode = toolbar.getAttribute("mode");
>+  var modePopup = document.getElementById("toolbarmodePopup");
>+
>+  for (i = 0; i < modePopup.childNodes.length; ++i) {
>+    var radio = modePopup.childNodes[i];
>+    if(radio.value == mode) {
>+      radio.setAttribute("checked", "true");
>+      break;
>+    }
>+  }
getElementsByAttribute("value", mode); ?

>+  mode == "text" ? smallicons.setAttribute("disabled", true) :
>+                   smallicons.removeAttribute("disabled");
Please use if/else; you're not interested in the resulting value.

>+  var custom = toolbar.getAttribute("toolbarlocalmode") ||
>+               toolbar.getAttribute("toolbarlocaliconsize") ||
>+               toolbar.getAttribute("toolbarlocallabelalign");
>+  var defmode = document.getElementById("toolbarmode-default");
>+  defmode.setAttribute("checked", !custom);
>+  defmode.setAttribute("disabled", !custom);
Wait, so these attributes are only used to detect custom settings?

>+function goSetToolbarState(aEvent)
>+{
>+  var toolbox = getMainToolbox();
>+  var toolbar = document.popupNode;
>+  while(toolbar.localName != "toolbar") toolbar = toolbar.parentNode;
var toolbox = toolbar.parentNode; (then you can eliminate getMailToolbox)
Also space before (

>+  if(mode == "smallicons") {
>+    var size = target.getAttribute("checked") == "true" ? "small" : "large";
>+    toolbox.setLocalToolbarMode(toolbar, "iconsize", size);
>+  }
>+  else if(mode == "end") {
>+    var align = target.getAttribute("checked") == "true" ? "end" : "bottom";
>+    toolbox.setLocalToolbarMode(toolbar, "labelalign", align);
>+  }
>+  else if(mode == "default") {
>+    toolbox.resetToolbarModes(toolbar);
>+  }
>+  else if(radiogroup == "mode")
>+    toolbox.setLocalToolbarMode(toolbar, "mode", mode);
>+}
switch? (Last case being default: of course)

>+function isElementVisible(aElement)
Strictly speaking this is an unrelated change.

>+  //XXXRatty: Hmm given the comment above, shouldn't this be || instead of &&?
&& is probably safer, in case the element is only invisible in one direction.

>+      <method name="updateCustomToolbarModes">
>+        <body><![CDATA[
>+          var toolbar = this.firstChild;
>+          var attrs = ["mode", "iconsize", "labelalign", "hidden"];
>+          var val;
>+          while (toolbar) {
>+            if (toolbar.localName == "toolbar" &&
>+                toolbar.getAttribute("customizable") == "true") {
>+              for (var i = 0; i < attrs.length; ++i) {
>+                var attr = attrs[i];
>+                val = toolbar.getAttribute("toolbarlocal" + attr);
>+                if (val)
>+                  toolbar.setAttribute(attr, val);
What does this achieve? Aren't we persisting all the attributes anyway?

I haven't looked at the CSS changes yet.

Comment 14

[[e-mail]]

Comment on attachment 324728 [details] [diff] [review]
Patch v 0.1.6

Where I write ??? it means that I'd like to know why the styles were added. (References to overridden styles or screen shots might help.) I've also tried to avoid repeating, so you may need to check stuff in several places.

>+toolbar[iconsize="small"] .toolbarbutton-1,
>+toolbar[iconsize="small"] .toolbarbutton-menubutton-button {
>   min-width: 0px;
> }
???

>+toolbar[mode="text"] .toolbarbutton-1,
>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-button {
>+ -moz-box-orient: horizontal
>+}
???

>+toolbar[labelalign="end"] .toolbarbutton-1,
>+toolbar[labelalign="end"] .toolbarbutton-1 > .toolbarbutton-menubutton-button,
>+toolbar[labelalign="end"] .toolbarbutton-1 > hbox > vbox {
>+  -moz-box-orient: horizontal;
>+  margin: 0;
>+}
>+
>+toolbar[labelalign="end"][mode="text"] .toolbarbutton-text {
>+  margin: 0;
>+}
???

>+toolbar[iconsize="small"] .toolbarbutton-1,
>+toolbar[iconsize="small"] .toolbarbutton-menubutton-button {
>   min-width: 0px;
> }
???

>+toolbar[mode="text"] .toolbarbutton-1,
>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-button {
>+ -moz-box-orient: horizontal
>+}
???
Nit: missing ;

>+toolbar[labelalign="end"] .toolbarbutton-1,
>+toolbar[labelalign="end"] .toolbarbutton-1 > .toolbarbutton-menubutton-button,
>+toolbar[labelalign="end"] .toolbarbutton-1 > hbox > vbox {
>+  -moz-box-orient: horizontal;
>+  margin: 0;
>+}
>+
>+toolbar[labelalign="end"][mode="text"] .toolbarbutton-text {
>+  margin: 0;
>+}
???

> #button-getmsg {
>+  list-style-image: url("chrome://messenger/skin/icons/messengericons.png");
???
(etc.)

>+/* ::::: small primary toolbar buttons ::::: */
IMHO fake small icons are worse than no small icons.

>+#home-button {
>+  list-style-image: url("chrome://communicator/skin/icons/communicatoricons.png");
>+  -moz-image-region: rect(120px 29px 149px 0);
>+  cursor: pointer;
>+}
>+
>+#home-button:hover {
>+  -moz-image-region: rect(120px 59px 149px 30px);
>+  text-decoration: underline;
>+} 
>+
>+#home-button:hover:active {
>+  -moz-image-region: rect(120px 89px 149px 60px);
>+}
>+
>+#home-button[disabled="true"] {
>+  -moz-image-region: rect(120px 119px 149px 90px) !important;
>+  cursor: default !important;
>+  text-decoration: none !important;
>+}
Please don't move style rules around, it makes the diff harder to read.

>-.toolbarbutton-1[toolbarmode="small"] {
>+toolbar[mode="icons"] toolbarbutton,
???

>+toolbar[mode="text"] #search-button > .button-box > .button-icon,
>+toolbar[mode="icons"] #search-button > .button-box > .button-text {
>+  display: none;
>+}
Hmm, the Go button only has text, right?

>+toolbarbutton.chevron {
>+  list-style-image: url("chrome://navigator/skin/icons/chevron.gif") !important;
???

> .toolbarbutton-1,
> .toolbarbutton-1[type="menu-button"] > .toolbarbutton-menubutton-stack 
>   > .toolbarbutton-menubutton-button
> {
>   -moz-box-orient: vertical;
>-  -moz-box-pack: start;
>+  -moz-box-pack: center;
???

>-.toolbarbutton-1 > .toolbarbutton-text {
>-  min-width: 50px;
>+.toolbarbutton-1 .toolbarbutton-text {
>+  min-width: 33px;
> }
???

>+toolbar[mode="text"] .toolbarbutton-1,
>+toolbar[mode="text"] .toolbarbutton-1[type="menu-button"]
>+  > .toolbarbutton-menubutton-stack  > .toolbarbutton-menubutton-button {
>+ -moz-box-orient: horizontal;
>+}
???

>+toolbar[labelalign="end"] .toolbarbutton-1,
>+toolbar[labelalign="end"] .toolbarbutton-1[type="menu-button"] > .toolbarbutton-menubutton-stack 
>+  > .toolbarbutton-menubutton-button
>+{
>+  -moz-box-orient: horizontal;
>+  font-size: 100%;
>+  color: #000;
> }
Duplicate?

>+toolbar[mode="text"] .toolbarbutton-1 .toolbarbutton-text,
>+toolbar[labelalign="end"] .toolbarbutton-1 .toolbarbutton-text {
>+  min-width: 0px;
> }
???

>+.toolbar-primary-icon[iconsize="small"] {
>+  display: none;
>+}
>+
>+.toolbar-primary-holder[iconsize="small"] {
>+  background-image: url("chrome://communicator/skin/toolbar/prtb-bg-noline.gif");
>+}
Duplicates?

>-toolbar[mode="text"] #button-getmsg > .toolbarbutton-menubutton-stack
>-  > .toolbarbutton-menubutton-dropmarker {
>-  margin: 0px 2px 0px 55px !important;
>-}
So, the dropmarkers are now centrally managed, right?

>+/* ::::: small primary toolbar buttons ::::: */
>+
>+toolbar[iconsize="small"] #button-getmsg > .toolbarbutton-menubutton-stack > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-newmsg > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-reply > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-replyall > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-forward > .toolbarbutton-menubutton-stack > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-file > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-goback > .toolbarbutton-menubutton-stack > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-goforward > .toolbarbutton-menubutton-stack > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-next > .toolbarbutton-menubutton-stack > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-junk > toolbarbutton > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-delete > .toolbarbutton-icon,
>+toolbar[iconsize="small"] #button-mark > .toolbarbutton-menubutton-stack > .toolbarbutton-menubutton-button > .toolbarbutton-icon {
>+  height: 19px;
>+  width: 28px;
>+}
Don't the icons know what size they are already?

>+/* ::::: primary toolbar buttons with labels on the right ::::: */
>+
>+toolbar[labelalign="end"][iconsize="small"] #home-button > .toolbarbutton-icon {
>+  width: 16px;
>+  height: 16px;
> }
???

>+toolbar[mode="icons"] #back-button > .toolbarbutton-menubutton-stack
>+    > .toolbarbutton-menubutton-dropmarker,
>+toolbar[mode="icons"] #forward-button > .toolbarbutton-menubutton-stack
>+    > .toolbarbutton-menubutton-dropmarker
>+{
>+  margin: 30px 0px 0px 34px;
>+}
No print button?

Comment 15

[Philip Chee]

Attachment: Patch v 0.1.7.1 (XUL/JS/XML part only)

I'm splitting off bits that should go into different bugs/patches (e.g. SetPageProxyState and the Stop button state) so this should be easier to review.

(In reply to Comment 13)
>>+function getMailToolbox() /* Thunderbird compatibility */
>>+{
>>+  getMainToolbox();
>>+}
>>+
>>+function getMainToolbox()
>>+{
>>+  if (!gMailToolbox)
>>+    gMailToolbox = document.getElementById("mail-toolbox");
>>+  return gMailToolbox;
>>+}
> This looks weird - getMailToolbox calls getMainToolbox to init gMailToolbox
> (not that you need getMainToolbox, see below, so this should be separated.)
Fixed. getMainToolbox() removed.

>>+           grippytooltiptext="&mailToolbar.tooltip;"
>>+           toolbarname="&showMessengerToolbarCmd.label;"
>Strange, I wonder why we have these apparently duplicate entities.
One is a tooltip, the other is for the View->Show/Hide menu.

>>-function getNavToolbox()
>>+function getNavToolbox() /* Firefox compatibility */
>???
Ooops. Removed comment.

>>+          if (toolbarMode != "text") {
>Firefox compatibility?

Err, basically if a (fullscreen) toolbar is in textmode, don't bother saving the state, changing the mode etc, thus on exiting fullscreen mode you don't have to restore any state.

>>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>>+           mode="icons" defaultmode="icons">
>Surely the default mode is both icons and text?

Hmm. Yes the default mode is icons+text - in classic. In Modern the buttonstate prefs for labels are ignored and labels are never shown in the nav-bar. What should we do here?

>>-      <hbox id="throbber-box" align="center">
>>+      <toolbaritem id="throbber-box" align="center" pack="center">
>Why the pack?
Good question. I've removed the pack. According to the DOM Inspector pack doesn't do anything. -moz-box-pack is always "start" what ever the pack attribute says. Also the align attribute doesn't seem to change -moz-box-align which is always "stretch" in classic and "start" in modern. Any clues?

>>-      <toolbarbutton id="home-button" class="bookmark-item"
>>+      <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>Why this change? (Well, toolbarbutton-1 I can sort of understand...)

This is leakage from the Customizable toolbars patch. I can defer it to that patch/bug if you say so.

The home button behaves like a bookmark item on the personal toolbar. When we move it to, say, the nav-bar we want it to behave like the other nav-bar buttons. So I changed the class to toolbarbutton-1, and then use a style specific to the personal toolbar to make it look like a bookmark item when on that toolbar.

The Firefox 3 developers solved this in a somewhat different fashion. They used JS to dynamically switch the class between "toolbarbutton-1" and "bookmark-item" depending on which toolbar the home button is on. I though that this was rather twee.

>>+function onViewToolbarsPopupShowing(aEvent)
>>+{
>>+  var popup = aEvent.target;
>>+  var i;
>>+
>>+  // Empty the menu
>I'm not convinced that this needs to be dynamic.

When a SeaMonkey extension developer writes a seamonkey toolbar, they overlay the toolbox and add a toolbar. They then overlay the View->Show/Hide menu and add a static menuitem (or not as the case may be, but I am assuming that they do for the purposes of this illustration) to toggle their toolbar on and off.

When a firefox extension developer writes a firefox toolbar, they overlay the toolbox and add a toolbar with a toolbarname attribute. Firefox then dynamically generates the View->Toolbars menu with a list of toolbars that can be toggled on and off.

The other use case is when we have user created custom toolbars (q.v. Customizable Toolbars) which can only be determined at run time. In this case we would have to generate a list of toolbars each time the menu is invoked as they can be created and destroyed between invocations.

>>+  var mode = toolbar.getAttribute("mode");
>>+  var modePopup = document.getElementById("toolbarmodePopup");
>>+
>>+  for (i = 0; i < modePopup.childNodes.length; ++i) {
>>+    var radio = modePopup.childNodes[i];
>>+    if(radio.value == mode) {
>>+      radio.setAttribute("checked", "true");
>>+      break;
>>+    }
>>+  }
>getElementsByAttribute("value", mode); ?

Fixed. Hmm.
+  var mode = toolbar.getAttribute("mode") || "full";
+  var modePopup = document.getElementById("toolbarmodePopup");
+  var radio = modePopup.getElementsByAttribute("value", mode);
+  if (radio)
+    radio[0].setAttribute("checked", "true");
Do I need a null check here?

>>+  mode == "text" ? smallicons.setAttribute("disabled", true) :
>>+                   smallicons.removeAttribute("disabled");
>Please use if/else; you're not interested in the resulting value.
Fixed.

>>+  var custom = toolbar.getAttribute("toolbarlocalmode") ||
>>+               toolbar.getAttribute("toolbarlocaliconsize") ||
>>+               toolbar.getAttribute("toolbarlocallabelalign");
>>+  var defmode = document.getElementById("toolbarmode-default");
>>+  defmode.setAttribute("checked", !custom);
>>+  defmode.setAttribute("disabled", !custom);
>Wait, so these attributes are only used to detect custom settings?

Well these came from the Customizable Toolbars patch. When you change the mode/size in the toolkit customize window it affects all the toolbars and overrides any local settings that my patch provides. What my patch then does when the Customize Toolbar window closes is to restore any local settings to individual toolbars (that the user may have made) by using the toolbarlocal* attributes. Should I remove the toolbarlocal* attributes and rewrite the logic for this patch?

>>+function goSetToolbarState(aEvent)
>>+{
>>+  var toolbox = getMainToolbox();
>>+  var toolbar = document.popupNode;
>>+  while(toolbar.localName != "toolbar") toolbar = toolbar.parentNode;
>var toolbox = toolbar.parentNode; (then you can eliminate getMailToolbox)
>Also space before (
Fixed.

>>+  if(mode == "smallicons") {
>>+    var size = target.getAttribute("checked") == "true" ? "small" : "large";
>>+    toolbox.setLocalToolbarMode(toolbar, "iconsize", size);
>>+  }
>>+  else if(mode == "end") {
>>+    var align = target.getAttribute("checked") == "true" ? "end" : "bottom";
>>+    toolbox.setLocalToolbarMode(toolbar, "labelalign", align);
>>+  }
>>+  else if(mode == "default") {
>>+    toolbox.resetToolbarModes(toolbar);
>>+  }
>>+  else if(radiogroup == "mode")
>>+    toolbox.setLocalToolbarMode(toolbar, "mode", mode);
>>+}
>switch? (Last case being default: of course)
Fixed.

>>+function isElementVisible(aElement)
>Strictly speaking this is an unrelated change.
Removed.

Have I missed anything else?

Comment 16

[[e-mail]]

(In reply to comment #15)
>(In reply to Comment 13)
>>>+          if (toolbarMode != "text") {
>>Firefox compatibility?
>Err, basically if a (fullscreen) toolbar is in textmode, don't bother saving
>the state, changing the mode etc, thus on exiting fullscreen mode you don't
>have to restore any state.
So Firefox leaves toolbars in text mode when it full screens?

>>>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>>>+           mode="icons" defaultmode="icons">
>>Surely the default mode is both icons and text?
>Hmm. Yes the default mode is icons+text - in classic. In Modern the buttonstate
>prefs for labels are ignored and labels are never shown in the nav-bar. What
>should we do here?
Well, the default mode has always been icons+text, but Modern currently has overrides for Navigator to display as icons except in textmode. I guess anyone wanting to keep using Modern can set that toobar to icons if they want.

>>>-      <hbox id="throbber-box" align="center">
>>>+      <toolbaritem id="throbber-box" align="center" pack="center">
>>Why the pack?
>Good question. I've removed the pack. According to the DOM Inspector pack
>doesn't do anything. -moz-box-pack is always "start" what ever the pack
>attribute says. Also the align attribute doesn't seem to change -moz-box-align
>which is always "stretch" in classic and "start" in modern. Any clues?
DOM Inspector doesn't always show the effect of XUL attributes. As it happens, align and pack attributes override the CSS, although modern might have had -moz-box-align for historical reasons.

>>>-      <toolbarbutton id="home-button" class="bookmark-item"
>>>+      <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>>Why this change? (Well, toolbarbutton-1 I can sort of understand...)
>The home button behaves like a bookmark item on the personal toolbar. When we
>move it to, say, the nav-bar we want it to behave like the other nav-bar
>buttons. So I changed the class to toolbarbutton-1, and then use a style
>specific to the personal toolbar to make it look like a bookmark item when on
>that toolbar.
chromeclass-toolbar-additional is not specific to the personal toolbar, is it?

>The Firefox 3 developers solved this in a somewhat different fashion. They used
>JS to dynamically switch the class between "toolbarbutton-1" and
>"bookmark-item" depending on which toolbar the home button is on. I though that
>this was rather twee.
:-)

>>I'm not convinced that this needs to be dynamic.
>When a SeaMonkey extension developer writes a seamonkey toolbar, they overlay
>the toolbox and add a toolbar. They then overlay the View->Show/Hide menu and
>add a static menuitem (or not as the case may be, but I am assuming that they
>do for the purposes of this illustration) to toggle their toolbar on and off.
My point is that this scheme works well for the Site Navigation Bar ;-)

>+  var mode = toolbar.getAttribute("mode") || "full";
>+  var modePopup = document.getElementById("toolbarmodePopup");
>+  var radio = modePopup.getElementsByAttribute("value", mode);
>+  if (radio)
>+    radio[0].setAttribute("checked", "true");
>Do I need a null check here?
getElementsByAttribute never returns null. radio[0] shouldn't be null either.

>>>+  var custom = toolbar.getAttribute("toolbarlocalmode") ||
>>>+               toolbar.getAttribute("toolbarlocaliconsize") ||
>>>+               toolbar.getAttribute("toolbarlocallabelalign");
>>>+  var defmode = document.getElementById("toolbarmode-default");
>>>+  defmode.setAttribute("checked", !custom);
>>>+  defmode.setAttribute("disabled", !custom);
>>Wait, so these attributes are only used to detect custom settings?
>Well these came from the Customizable Toolbars patch. When you change the
>mode/size in the toolkit customize window it affects all the toolbars and
>overrides any local settings that my patch provides.
Is it possible to use the toolkit customise window without overriding the local settings? We can then say, "use customise as a quick way to reset the settings of all the toolbars."

Comment 17

[Tony Mechelynck [:tonymec]]

(In reply to comment #16)
[...]
> Is it possible to use the toolkit customise window without overriding the local
> settings? We can then say, "use customise as a quick way to reset the settings
> of all the toolbars."
> 

This bug was created precisely because toolbar customization seems to have hit an impasse in SeaMonkey. See among others bug 157199 and bug 394288. Beware of a chicken-and-egg loop where A waits on B which waits on A.

Comment 18

[[e-mail]]

Comment on attachment 329031 [details] [diff] [review]
Patch v 0.1.7.1 (XUL/JS/XML part only)

>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>+           mode="icons" defaultmode="icons">
I think we agreed that this should be "full".

>-      <hbox id="nav-bar-inner" flex="1">
>+
>+      <hbox id="nav-bar-inner" flex="1" class="chromeclass-location">
The problem with doing this is that this gets hidden on windows without a location bar, which means that the throbber ends up on the left :-(
Also, isn't this supposed to be a toolbaritem?

>-      <toolbarbutton id="home-button" class="bookmark-item"
>+      <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
(still waiting for information on how chromeclass-toolbar-additional helps)

>+  for (i = popup.childNodes.length-1; i >= 0; --i) {
Nit: length - 1

>+  var toolbox = document.popupNode;
>+  while (toolbox.localName != "toolbox") toolbox = toolbox.parentNode;
...
>+  var toolbar = document.popupNode;
>+  while (toolbar.localName != "toolbar")
>+    toolbar = toolbar.parentNode;
Get the toolbar first, then toolbox = toolbar.parentNode;
(Like goSetToolbarState, but I overlooked it last time!)

>+  var radio = modePopup.getElementsByAttribute("value", mode);
>+  if (radio)
Nit: getElementsByAttribute never returns null.

>+  smallicons.setAttribute("checked", small);
>+  if (mode == "text")
>+    smallicons.setAttribute("disabled", true);
>+  else
>+    smallicons.removeAttribute("disabled");
Nit: could use smallicons.setAttribute("disabled", mode == "text");

>+  mode == "icons" ? labelalign.setAttribute("disabled", true) :
>+                    labelalign.removeAttribute("disabled");
You forgot to remove this.

>+  if (mode == "icons")
>+    labelalign.setAttribute("disabled", true);
>+  else
>+    labelalign.removeAttribute("disabled");
See smallicons.

>+  var custom = toolbar.getAttribute("toolbarlocalmode") ||
>+               toolbar.getAttribute("toolbarlocaliconsize") ||
>+               toolbar.getAttribute("toolbarlocallabelalign");
>+  var defmode = document.getElementById("toolbarmode-default");
>+  defmode.setAttribute("checked", !custom);
>+  defmode.setAttribute("disabled", !custom);
So what's the point of <toolbar mode="full" defaultmode="full">?

>+  while (toolbar.localName != "toolbar") toolbar = toolbar.parentNode;
Nit: split to one line per statement

>+    case "default":
>+    toolbox.resetToolbarModes(toolbar);
>+      break;
Nit: indentation wrong

>+    default:
>+      if(radiogroup == "mode")
When is radiogroup not going to be mode?

>+  <popup id="toolbar-context-menu"
>+         onpopupshowing="onViewToolbarsPopupShowing(event);">
>+
>+    <menuseparator id="toolbarmode-sep"/>
>+    <menu id="toolbarmode-context-menu"
>+          label="&customizeToolbar.toolbarmode.label;"
>+          accesskey="&customizeToolbar.toolbarmode.accesskey;">
>+      <menupopup id="toolbarmodePopup"
>+                 onpopupshowing="event.stopPropagation();"
>+                 oncommand="goSetToolbarState(event);">
If you add event.stopPropagation(); to goSetToolbarState, you can then add oncommand="onViewToolbarCommand(event);" to the popup, so you don't have to add it manually in onViewToolbarsPopupShowing.

>+      <method name="setLocalToolbarMode">
>+        <parameter name="aToolbar"/>
>+        <parameter name="aMode"/>
>+        <parameter name="aValue"/>
>+        <body><![CDATA[
>+          aToolbar.setAttribute(aMode, aValue);
>+          document.persist(aToolbar.id, aMode);
>+          var localMode = "toolbarlocal" + aMode;
>+          aToolbar.setAttribute(localMode, aValue);
>+          document.persist(aToolbar.id, localMode);
(still waiting for information on whether the customise toolbar dialog will unconditionally trash toolbar modes or only if you interact with the UI)

Comment 19

[Philip Chee]

Attachment: Patch v 0.1.7.2 (XUL/JS/XML part only)

>>>+          if (toolbarMode != "text") {
>>Firefox compatibility?
>Err, basically if a (fullscreen) toolbar is in textmode, don't bother saving
>the state, changing the mode etc, thus on exiting fullscreen mode you don't
>have to restore any state.
>So Firefox leaves toolbars in text mode when it full screens?
Yes it doesn't touch text mode toolbars going in or out of full screen.

>>>>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>>>>+           mode="icons" defaultmode="icons">
>>>Surely the default mode is both icons and text?
>Hmm. Yes the default mode is icons+text - in classic. In Modern the buttonstate
>prefs for labels are ignored and labels are never shown in the nav-bar. What
>should we do here?
>Well, the default mode has always been icons+text, but Modern currently has
>overrides for Navigator to display as icons except in textmode. I guess anyone
>wanting to keep using Modern can set that toobar to icons if they want.
Fixed: mode="full" defaultmode="full"

>>>>-      <hbox id="throbber-box" align="center">
>>>>+      <toolbaritem id="throbber-box" align="center" pack="center">
>>>Why the pack?
>>Good question. I've removed the pack. According to the DOM Inspector pack
>>doesn't do anything. -moz-box-pack is always "start" what ever the pack
>>attribute says. Also the align attribute doesn't seem to change -moz-box-align
>>which is always "stretch" in classic and "start" in modern. Any clues?
>DOM Inspector doesn't always show the effect of XUL attributes. As it happens,
>align and pack attributes override the CSS, although modern might have had
>-moz-box-align for historical reasons.
Fixed.

>>>>-      <toolbarbutton id="home-button" class="bookmark-item"
>>>>+      <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>>>Why this change? (Well, toolbarbutton-1 I can sort of understand...)
>>The home button behaves like a bookmark item on the personal toolbar. When we
>>move it to, say, the nav-bar we want it to behave like the other nav-bar
>>buttons. So I changed the class to toolbarbutton-1, and then use a style
>>specific to the personal toolbar to make it look like a bookmark item when on
>>that toolbar.
>chromeclass-toolbar-additional is not specific to the personal toolbar, is it?
No it isn't.

Removed "chromeclass-toolbar-additional". But all the navigation buttons in the nav bar have this class. I'll revisit this when the home button is movable to the nav bar.

>>>I'm not convinced that this needs to be dynamic.
>>When a SeaMonkey extension developer writes a seamonkey toolbar, they overlay
>>the toolbox and add a toolbar. They then overlay the View->Show/Hide menu and
>>add a static menuitem (or not as the case may be, but I am assuming that they
>>do for the purposes of this illustration) to toggle their toolbar on and off.
>My point is that this scheme works well for the Site Navigation Bar ;-)

I think I just totally misunderstood you. I looked at the links navigation bar code and I just don't get your point. I'll try to catch you on #irc.

>>+  if (radio)
>>+    radio[0].setAttribute("checked", "true");
>>Do I need a null check here?
>getElementsByAttribute never returns null. radio[0] shouldn't be null either.
Fixed.

>>>>+  var custom = toolbar.getAttribute("toolbarlocalmode") ||
>>>>+               toolbar.getAttribute("toolbarlocaliconsize") ||
>>>>+               toolbar.getAttribute("toolbarlocallabelalign");
>>>>+  var defmode = document.getElementById("toolbarmode-default");
>>>>+  defmode.setAttribute("checked", !custom);
>>>>+  defmode.setAttribute("disabled", !custom);
>>>Wait, so these attributes are only used to detect custom settings?

>>Well these came from the Customizable Toolbars patch. When you change the
>>mode/size in the toolkit customize window it affects all the toolbars and
>>overrides any local settings that my patch provides.

>Is it possible to use the toolkit customise window without overriding the local
>settings? We can then say, "use customise as a quick way to reset the settings
>of all the toolbars."

If you just drag buttons around, then the toolkit code doesn't change any toolbar settings. If you use the Customise window to change the icon size or button mode, the toolkit code will unconditionally update *all* toolbars. KaiRo wanted finer, per toolbar control so I went through the hoops a bit.

If you click on the reset to defaults button, every toolbar gets zapped. I'm not sure how we want SeaMonkey to react. In one version of my original megapatch, I would restore any custom local settings after the customize window closed.

>>>Wait, so these attributes are only used to detect custom settings?
Specifically to detect settings that differ from that particular toolbars
defaults.

=============================================

>>+  <toolbox id="navigator-toolbox" class="toolbox-top" deferattached="true"
>>+           mode="icons" defaultmode="icons">
>I think we agreed that this should be "full".
Right

>>-      <hbox id="nav-bar-inner" flex="1">
>>+
>>+      <hbox id="nav-bar-inner" flex="1" class="chromeclass-location">
>The problem with doing this is that this gets hidden on windows without a
>location bar, which means that the throbber ends up on the left :-(

Hmm. Firefox doesn't have this exact problem since their throbber is in the menubar. On their nav bar they have a hidden:

      <hbox id="fullscreenflex" flex="1" hidden="true" fullscreencontrol="true"/>

just before the window-controls which gets unhidden in full screen mode. Should I do something like this for our throbber but with "chromeclass-location" instead?

>Also, isn't this supposed to be a toolbaritem?
I was going to do it in the actual customize toolbar bug/patch that allows you to move these around.

>>-      <toolbarbutton id="home-button" class="bookmark-item"
>>+      <toolbarbutton id="home-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
>(still waiting for information on how chromeclass-toolbar-additional helps)
Removed since I don't understand why the navigation buttons all have these.

>>+  for (i = popup.childNodes.length-1; i >= 0; --i) {
>Nit: length - 1
Fixed.

>>+  var toolbox = document.popupNode;
>>+  while (toolbox.localName != "toolbox") toolbox = toolbox.parentNode;
>...
>>+  var toolbar = document.popupNode;
>>+  while (toolbar.localName != "toolbar")
>>+    toolbar = toolbar.parentNode;
>Get the toolbar first, then toolbox = toolbar.parentNode;
>(Like goSetToolbarState, but I overlooked it last time!)
Fixed.

>>+  var radio = modePopup.getElementsByAttribute("value", mode);
>>+  if (radio)
>Nit: getElementsByAttribute never returns null.
Fixed.

>>+  smallicons.setAttribute("checked", small);
>>+  if (mode == "text")
>>+    smallicons.setAttribute("disabled", true);
>>+  else
>>+    smallicons.removeAttribute("disabled");
>>Nit: could use smallicons.setAttribute("disabled", mode == "text");
Fixed.

>>>+  mode == "icons" ? labelalign.setAttribute("disabled", true) :
>>>+                    labelalign.removeAttribute("disabled");
>You forgot to remove this.
Fixed.

>>+  if (mode == "icons")
>>+    labelalign.setAttribute("disabled", true);
>>+  else
>>+    labelalign.removeAttribute("disabled");
>See smallicons.
Fixed.

>>+  var custom = toolbar.getAttribute("toolbarlocalmode") ||
>>+               toolbar.getAttribute("toolbarlocaliconsize") ||
>>+               toolbar.getAttribute("toolbarlocallabelalign");
>>+  var defmode = document.getElementById("toolbarmode-default");
>>+  defmode.setAttribute("checked", !custom);
>>+  defmode.setAttribute("disabled", !custom);
>So what's the point of <toolbar mode="full" defaultmode="full">?

Originally when the "reset everything" button in the customi(s|z)e toolbar window was pressed, the code only looked at the defaults on the TOOLBOX. KaiRo didn't like it so I submitted a patch to toolkit to check for
the default{mode|size} attributes on the toolbar first. This explains the defaultmode="full", the mode="full" isn't necessary but I added it in to make it explicit. I'll remove it if you think that this is being overly obsessive.

>>+  while (toolbar.localName != "toolbar") toolbar = toolbar.parentNode;
>Nit: split to one line per statement
Fixed.

>>+    case "default":
>>+    toolbox.resetToolbarModes(toolbar);
>>+      break;
>Nit: indentation wrong
Fixed.

>>+    default:
>>+      if(radiogroup == "mode")
>When is radiogroup not going to be mode?
Fixed.

>>+  <popup id="toolbar-context-menu"
>>+         onpopupshowing="onViewToolbarsPopupShowing(event);">
>>+
>>+    <menuseparator id="toolbarmode-sep"/>
>>+    <menu id="toolbarmode-context-menu"
>>+          label="&customizeToolbar.toolbarmode.label;"
>>+          accesskey="&customizeToolbar.toolbarmode.accesskey;">
>>+      <menupopup id="toolbarmodePopup"
>>+                 onpopupshowing="event.stopPropagation();"
>>+                 oncommand="goSetToolbarState(event);">
>If you add event.stopPropagation(); to goSetToolbarState, you can then add
>oncommand="onViewToolbarCommand(event);" to the popup, so you don't have to add
>it manually in onViewToolbarsPopupShowing.
Fixed.

>>+      <method name="setLocalToolbarMode">
>>+        <parameter name="aToolbar"/>
>>+        <parameter name="aMode"/>
>>+        <parameter name="aValue"/>
>>+        <body><![CDATA[
>>+          aToolbar.setAttribute(aMode, aValue);
>>+          document.persist(aToolbar.id, aMode);
>>+          var localMode = "toolbarlocal" + aMode;
>>+          aToolbar.setAttribute(localMode, aValue);
>>+          document.persist(aToolbar.id, localMode);
>(still waiting for information on whether the customise toolbar dialog will
>unconditionally trash toolbar modes or only if you interact with the UI)
See above.

Comment 20

[Philip Chee]

Attachment: toolbar[iconsize="small"] .toolbarbutton-1

[mac/button.css]
>>+toolbar[iconsize="small"] .toolbarbutton-1,
>>+toolbar[iconsize="small"] .toolbarbutton-menubutton-button {
>>   min-width: 0px;
>> }
>???

The standard style sets the min-width to 47px; e.g.:
.toolbarbutton-1,
.toolbarbutton-1 > .toolbarbutton-menubutton-button {
  min-width: 47px;
  -moz-box-orient: vertical;
}

Setting min-width to 0px reduces the "lots of wasted empty space" look (bottom half of screenshot)

Comment 21

[Philip Chee]

Attachment: toolbar[mode="text"] .toolbarbutton-1

>>+toolbar[mode="text"] .toolbarbutton-1,
>>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-button {
>>+ -moz-box-orient: horizontal
>>+}
>???

Before (top half) and After (bottom half). With -moz-box-orient: horizontal, the labels are vertically centred and aligned with the drop markers.

Comment 22

[Philip Chee]

Attachment: toolbar[labelalign="end"] .toolbarbutton-1

[mac/button.css]
>>+toolbar[labelalign="end"] .toolbarbutton-1,
>>+toolbar[labelalign="end"] .toolbarbutton-1 > .toolbarbutton-menubutton-button,
>>+toolbar[labelalign="end"] .toolbarbutton-1 > hbox > vbox {
>>+  -moz-box-orient: horizontal;
>>+  margin: 0;
>>+}

-moz-box-orient: horizontal puts the label after the icon instead of below. I've removed the margin: 0 as it was leakage from the original megapatch.

>>+toolbar[labelalign="end"][mode="text"] .toolbarbutton-text {
>>+  margin: 0;
>>+}
>???
Removed. Not needed in this patch.

Comment 23

[Philip Chee]

Attachment: Patch v 0.1.7.C1 (Classic changes only)

[win/button.css]
See comments for mac/button.css

>>+toolbar[iconsize="small"] .toolbarbutton-1,
>>+toolbar[iconsize="small"] .toolbarbutton-menubutton-button {
>>   min-width: 0px;
>> }
>???
>
>>+toolbar[mode="text"] .toolbarbutton-1,
>>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-button {
>>+ -moz-box-orient: horizontal
>>+}
>???
>Nit: missing ;
Fixed.

>>+toolbar[labelalign="end"] .toolbarbutton-1,
>>+toolbar[labelalign="end"] .toolbarbutton-1 > .toolbarbutton-menubutton-button,
>>+toolbar[labelalign="end"] .toolbarbutton-1 > hbox > vbox {
>>+  -moz-box-orient: horizontal;
>>+  margin: 0;
>>+}
>>+
>>+toolbar[labelalign="end"][mode="text"] .toolbarbutton-text {
>>+  margin: 0;
>>+}
>???

>> #button-getmsg {
>>+  list-style-image: url("chrome://messenger/skin/icons/messengericons.png");
>???
>(etc.)
In customizable toolbars, we overlay customizeToolbars.xul with navigator.css, messenger.css etc. In the original bug you pointed out that just inheriting the list-style-image from .toolbarbutton-1 won't work because every window with customizable toolbars would want to overlay their own css. I could defer this to the next bug if you think this isn't necessary at this stage.

>>+/* ::::: small primary toolbar buttons ::::: */
>IMHO fake small icons are worse than no small icons.

I put this in just to test that things are working. In classic they don't look too bad. They look really really bad in Modern though. Can I leave these in for Classic until MReimer gets around to generating real small icons?

>Please don't move style rules around, it makes the diff harder to read.
Fixed.

>>-.toolbarbutton-1[toolbarmode="small"] {
>>+toolbar[mode="icons"] toolbarbutton,
>???
When I was testing the original megapatch with toolbar buttons from various extensions, I noticed that some of them were not using the toolbarbutton-1 class. Not necessary here. In fact I've deleted this and moved it to button.css

>>+toolbar[mode="text"] #search-button > .button-box > .button-icon,
>>+toolbar[mode="icons"] #search-button > .button-box > .button-text {
>>+  display: none;
>>+}
>Hmm, the Go button only has text, right?
Right.

>>+toolbarbutton.chevron {
>>+  list-style-image: url("chrome://navigator/skin/icons/chevron.gif") !important;
>???
After putting the home-button styles back to where they were, the diff is clearer:

-#bookmarks-chevron {
+toolbarbutton.chevron {
   list-style-image: url("chrome://navigator/skin/icons/chevron.gif") !important;
 } 

I copied this from the other browser. It appears that they use toolbarbutton.chevron
because their places toolbar is a XBL widget.

Comment 24

[Philip Chee]

Attachment: Patch v 0.1.7.2.1 (XUL/JS/XML part only) Fix typos.

Hmm. I fixed some typos but they seem to have gotten lost when I moved to HG.

See Comment #19 for details.

Comment 25

[Philip Chee]

>>>+toolbarbutton.chevron {
>>>+  list-style-image: url("chrome://navigator/skin/icons/chevron.gif") !important;
>>???
>After putting the home-button styles back to where they were, the diff is
>clearer:
>
>-#bookmarks-chevron {
>+toolbarbutton.chevron {
>   list-style-image: url("chrome://navigator/skin/icons/chevron.gif") !important;
> }

Note: Classic uses "#bookmarks-chevron" but Modern uses "toolbarbutton.chevron"

Comment 26

[Philip Chee]

Attachment: .toolbarbutton-1 ... -moz-box-pack: center;

>> .toolbarbutton-1,
>> .toolbarbutton-1[type="menu-button"] > .toolbarbutton-menubutton-stack 
>>   > .toolbarbutton-menubutton-button
>> {
>>   -moz-box-orient: vertical;
>>-  -moz-box-pack: start;
>>+  -moz-box-pack: center;
>???

It isn't obvious in this patch, but once you get third party extension toolbar buttons in the UI things look awful with |-moz-box-pack: start;| Most extension toolbar buttons seem to implicitly assume that the icon will be vertically centred in the button. This screen-shot is from the complete mega-patch with customizable toolbars.

Comment 27

[Philip Chee]

Attachment: .toolbarbutton-1 ... min-width: 33px;

>>-.toolbarbutton-1 > .toolbarbutton-text {
>>-  min-width: 50px;
>>+.toolbarbutton-1 .toolbarbutton-text {
>>+  min-width: 33px;
>> }
>???
I felt that in small icon modes there is too much wasted space. Also see previous comment about extension toolbar buttons.

Comment 28

[Philip Chee]

Attachment: toolbar[mode="text"] ... -moz-box-orient: horizontal;

>>+toolbar[mode="text"] .toolbarbutton-1,
>>+toolbar[mode="text"] .toolbarbutton-1[type="menu-button"]
>>+  > .toolbarbutton-menubutton-stack  > .toolbarbutton-menubutton-button {
>>+ -moz-box-orient: horizontal;
>>+}
>???
See before (top half) and after (bottom half) applying this style. Without this, the Reload and Stop buttons are vertically misaligned compared to the Backwards and Forwards buttons.

Comment 29

[Philip Chee]

Attachment: toolbar[mode="text"] ... .toolbarbutton-text min-width: 0px;

>>+toolbar[mode="text"] .toolbarbutton-1 .toolbarbutton-text,
>>+toolbar[labelalign="end"] .toolbarbutton-1 .toolbarbutton-text {
>>+  min-width: 0px;
>> }
>???
In these modes the default min-width leaves significant horizontal space between the label and the drop markers, and with the button icon

Comment 30

[Philip Chee]

Attachment: toolbar[labelalign="end"] ... .toolbarbutton-text min-width: 0px;

>>+toolbar[mode="text"] .toolbarbutton-1 .toolbarbutton-text,
>>+toolbar[labelalign="end"] .toolbarbutton-1 .toolbarbutton-text {
>>+  min-width: 0px;
>> }
>???
In these modes the default min-width leaves significant horizontal space between the label and the drop markers, and with the button icon.

Comment 31

[Philip Chee]

Attachment: Patch v 0.1.8.C (Modern diffs)

Addressing the rest of the comments on the modern changes:

>>+toolbar[labelalign="end"] .toolbarbutton-1,
>>+toolbar[labelalign="end"] .toolbarbutton-1[type="menu-button"] > .toolbarbutton-menubutton-stack 
>>+  > .toolbarbutton-menubutton-button
>>+{
>>+  -moz-box-orient: horizontal;
>>+  font-size: 100%;
>>+  color: #000;
>> }
>Duplicate?

Fixed ( removed font-size and colour styles).

>>+.toolbar-primary-icon[iconsize="small"] {
>>+  display: none;
>>+}
>>+
>>+.toolbar-primary-holder[iconsize="small"] {
>>+  background-image: url("chrome://communicator/skin/toolbar/prtb-bg-noline.gif");
>>+}
>Duplicates?

Moved to toolbar.css and combined with existing styles there.

>>-toolbar[mode="text"] #button-getmsg > .toolbarbutton-menubutton-stack
>>-  > .toolbarbutton-menubutton-dropmarker {
>>-  margin: 0px 2px 0px 55px !important;
>>-}
>So, the dropmarkers are now centrally managed, right?
Only in text and label-on-the-end modes. Special casing each  primary button wasn't viable. This way the drop markers automatically work for current and future (e.g. extension toolbar buttons) buttons.

>>+/* ::::: small primary toolbar buttons ::::: */
[.....]
.toolbarbutton-icon {
>>+  height: 19px;
>>+  width: 28px;
>>+}
>Don't the icons know what size they are already?
I was just testing small icon mode by squeezing the large buttons down via css (they look really awful!), I'll remove them later when I'm done testing.

>>+toolbar[labelalign="end"][iconsize="small"] #home-button > .toolbarbutton-icon {
>>+  width: 16px;
>>+  height: 16px;
>> }
>???

In XPFE/Modern: The home graphic is 22x17px.

On trunk we are using the graphics in chrome://communicator/skin/icons/common-small.png which are 19x19px. I'm squeezing it down to 16x16 to prevent it from expanding the bookmarks toolbar vertically and also to better match the look of modern back on branch.

>>+toolbar[mode="icons"] #back-button > .toolbarbutton-menubutton-stack
>>+    > .toolbarbutton-menubutton-dropmarker,
>>+toolbar[mode="icons"] #forward-button > .toolbarbutton-menubutton-stack
>>+    > .toolbarbutton-menubutton-dropmarker
>>+{
>>+  margin: 30px 0px 0px 34px;
>>+}
>No print button?

The more general styles work reasonably well in this particular case so I didn't see the need to special-case the print button here.

Comment 32

[Philip Chee]

Attachment: Patch v 0.1.8 Complete

The latest complete WIP patch.

Comment 33

[Philip Chee]

Attachment: Patch v 0.1.9 Complete

Updated patch to tip (The previous patch still applies with a bit of fuzz).

Comment 34

[[e-mail]]

Comment on attachment 335499 [details] [diff] [review]
Patch v 0.1.9 Complete

Nearly there, I think... some nits on the non-CSS bits (CSS to follow):

>- 
>-      <hbox id="nav-bar-inner" flex="1">
>+
>+      <hbox id="nav-bar-inner" flex="1" class="chromeclass-location">
Still don't want this.

>       <hbox id="window-controls" hidden="true" fullscreencontrol="true">
>         <toolbarbutton id="minimize-button" class="toolbarbutton-1"
These didn't display properly in Classic when the toolbar is in text mode. It might be better to remove the class="toolbarbutton-1" as then the text/icon settings won't affect the full screen controls, which would presumably simplify the CSS for both Classic and Modern themes.

>+      <method name="setLocalToolbarMode">
I don't think we need this, we can compare the current mode against the default mode (that resetToolbarModes uses) instead.

>+  for (i = popup.childNodes.length - 1; i >= 0; --i) {
>+    var deadItem = popup.childNodes[i];
>+    if (deadItem.hasAttribute("toolbarid"))
>+      popup.removeChild(deadItem);
>+  }
Possibly use popup.getElementsByAttribute("toolbarid", "*") ?

>+      toolbox.resetToolbarModes(toolbar);
Given that this is the only caller, is it worth having the separate method?

>+<!ENTITY customizeToolbar.toolbarmode.label       "Settings for this toolbar ">
Nit: trailing space in the string.

Comment 35

[[e-mail]]

(In reply to comment #34)
> (From update of attachment 335499 [details] [diff] [review])
>>       <hbox id="window-controls" hidden="true" fullscreencontrol="true">
>>         <toolbarbutton id="minimize-button" class="toolbarbutton-1"
>These didn't display properly in Classic when the toolbar is in text mode. It
>might be better to remove the class="toolbarbutton-1" as then the text/icon
>settings won't affect the full screen controls, which would presumably simplify
>the CSS for both Classic and Modern themes.
Hmm, I wonder why removing the class worked, as it would seem that the rules for hiding the text don't check for that class.

Comment 36

[[e-mail]]

Comment on attachment 335499 [details] [diff] [review]
Patch v 0.1.9 Complete

>-  min-width: 0px;
>+  min-width: 0;
Unnecessary to change these.

>+ -moz-box-orient: horizontal
Still missing a ;

> .toolbarbutton-1 {
>   list-style-image: url("chrome://messenger/skin/icons/messengericons.png");
> }
> 
> #button-getmsg {
>+  list-style-image: url("chrome://messenger/skin/icons/messengericons.png");
Might as well remove the .toolbarbutton-1 rule.
(And in navigator too, but I couldn't find that one in your diff.)

+/* ::::: small primary toolbar buttons ::::: */
+/* XXXRatty workaround because we don't have small icons yet */
This is for demonstration purposes only, right?

+#nav-bar[inFullscreen="true"] .toolbar-primary-grippy /* .toolbar-grippy ??*/ {
Modern's primary toolbar does use toolbar-primary-grippy, but I don't think Classic does.

>+toolbar[iconsize="small"][labelalign="end"] #home-button > .toolbarbutton-icon {
>+  width: 16px;
>+  height: 16px;
>+}
>+
>+/* ::::: primary toolbar buttons with labels on the right ::::: */
>+
>+toolbar[labelalign="end"][iconsize="small"] #home-button > .toolbarbutton-icon {
>+  width: 16px;
>+  height: 16px;
Duplicate. (I think it's OK to leave one of these styles in.)

> #home-button {
>+  list-style-image: url("chrome://communicator/skin/icons/communicatoricons.png");
>+  -moz-image-region: rect(120px 29px 149px 0);
>+  cursor: pointer;
> }
> 
> #home-button:hover {
>+  -moz-image-region: rect(120px 59px 149px 30px);
>+  text-decoration: underline;
>+} 
Hmm, the cursor and pointer won't look so good if the button is moved.

Comment 37

[[e-mail]]

Comment on attachment 335499 [details] [diff] [review]
Patch v 0.1.9 Complete

>+toolbar[mode="text"] .toolbarbutton-1,
>+toolbar[labelalign="end"] .toolbarbutton-1 {
>+  color: #000;
I think this should be inherit;

>+  padding: 0px 2px !important;
This is to match bookmark items?

>+toolbar[iconsize=small] .toolbarbutton-menubutton-dropmarker {
Missing "s around "small"

>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-stack,
>+toolbar[labelalign="end"]:not([mode="icons"]) .toolbarbutton-1 > .toolbarbutton-menubutton-stack {
>+  display: -moz-box;
>+  -moz-box-orient: horizontal;
I don't think you can change the orient of a stack.
What style changes the display that you need to change it back?

>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-stack
>+  > .toolbarbutton-menubutton-dropmarker,
>+toolbar[labelalign="end"]:not([mode="icons"]) .toolbarbutton-1 > .toolbarbutton-menubutton-stack
>+  > .toolbarbutton-menubutton-dropmarker {
>+  margin: 0px 2px !important;
Doesn't this style belongs in button.css with the other dropmarker styles?

>+ /* XXXRatty: copied min-width: 0px from navigator.css,
>+    has no effect in large icon mode. But makes small icon mode
>+    look better. Centralize/move this style to button.css??
>+ */
> .toolbarbutton-1 {
>   list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>+  min-width: 0px !important;
> }
The default button width without a min-width style would be 44px;
maybe you could adjust the padding or margin to avoid a min-width.

>+/* ::::: small primary toolbar buttons ::::: */
I guess this is harder to remove because we do actually have some (but not all) small button images.

> .toolbarbutton-1 > stack > .toolbarbutton-menubutton-button 
>   > .toolbarbutton-icon,
> .toolbarbutton-1 > .toolbarbutton-icon {
>+  display: inline;
>+  /* Ratty: !important causes problems with fullscreen controls */
> }
> 
> toolbar[mode="text"] .toolbarbutton-1 .toolbarbutton-icon {
>+  display: none;
>+  /* Ratty: !important causes problems with fullscreen controls */
> }
Why do we need to keep these styles at all?

>+toolbar[iconsize="small"] .toolbarbutton-1 {
>+  margin-top: 0;
???

>+toolbar[mode="full"][iconsize="small"] .toolbarbutton-1 {
>+  margin-top: 3px !important;
???

>-#search-button[toolbarmode="small"] > .button-box > .button-text {
Where did this go?

The diff in this area was very difficult to read, it's hard to tell that you're not (I hope!) really changing anything.

> #back-button > .toolbarbutton-menubutton-stack
>     > .toolbarbutton-menubutton-dropmarker,
> #forward-button > .toolbarbutton-menubutton-stack
>     > .toolbarbutton-menubutton-dropmarker
> {
>-  margin: 30px 0px 0px 34px;
>+  margin: 20px 0px 0px 34px;
What's the default margin here (I couldn't find it)?

Comment 38

[Philip Chee]

Attachment: Screen-shot navigator in print-preview mode

IanN asked if I tested that the toolbars patch has been tested in print preview mode. Here is a screen-shot. It looks OK as far as I can tell (except that the content looks rather naff).

Comment 39

[Philip Chee]

Attachment: Toolbars patch in print preview mode.

IanN asked me to test the patch in print preview mode.

Comment 40

[Ian Neal]

Comment on attachment 336033 [details]
Toolbars patch in print preview mode.

That looks fine, I assume it is okay from mailnews too?

Comment 41

[Philip Chee]

> That looks fine, I assume it is okay from mailnews too?
It's the same, right down to the extra fat scroll bars, but they are also like this without this patch applied.

Comment 42

[Philip Chee]

Attachment: Patch v 2.0 Complete

>>+      <hbox id="nav-bar-inner" flex="1" class="chromeclass-location">
> Still don't want this.
Fixed.

>>       <hbox id="window-controls" hidden="true" fullscreencontrol="true">
>>         <toolbarbutton id="minimize-button" class="toolbarbutton-1"
> These didn't display properly in Classic when the toolbar is in text mode. It
> might be better to remove the class="toolbarbutton-1" as then the text/icon
Removed.

> settings won't affect the full screen controls, which would presumably simplify
> the CSS for both Classic and Modern themes.
Managed to remove only one stlye each in classic and modern.

>>+      <method name="setLocalToolbarMode">
> I don't think we need this, we can compare the current mode against the default
> mode (that resetToolbarModes uses) instead.
Fixed.

>>+  for (i = popup.childNodes.length - 1; i >= 0; --i) {
>>+    var deadItem = popup.childNodes[i];
>>+    if (deadItem.hasAttribute("toolbarid"))
>>+      popup.removeChild(deadItem);
>>+  }
> Possibly use popup.getElementsByAttribute("toolbarid", "*") ?
Fixed.

>>+      toolbox.resetToolbarModes(toolbar);
> Given that this is the only caller, is it worth having the separate method?
Fixed.

>>+<!ENTITY customizeToolbar.toolbarmode.label       "Settings for this toolbar ">
> Nit: trailing space in the string.
Fixed.

******* CLASSIC *******

>>-  min-width: 0px;
>>+  min-width: 0;
> Unnecessary to change these.
Fixed.

>>+ -moz-box-orient: horizontal
> Still missing a ;
Fixed.

>> .toolbarbutton-1 {
>>   list-style-image: url("chrome://messenger/skin/icons/messengericons.png");
>> }
>> 
>> #button-getmsg {
>>+  list-style-image: url("chrome://messenger/skin/icons/messengericons.png");
> Might as well remove the .toolbarbutton-1 rule.
> (And in navigator too, but I couldn't find that one in your diff.)

I'd rather leave it in for the customize toolbar bit in the next bug. The reason is a common mistake for new extension authors is to forget to skin their toolbar button or make a mistake in their css. In the other browser this results in a very large button containing all the icons in the palette. This is a useful reminder that something is broken. Without this rule the toolbar button is invisible leading to two problems: noticing where on a toolbar and which toolbar it was dropped on; and it's almost impossible to drag it back to the palette even if you know where it is.

>+/* ::::: small primary toolbar buttons ::::: */
>+/* XXXRatty workaround because we don't have small icons yet */
> This is for demonstration purposes only, right?
Yes. I've changed the comment to:
/* XXXRatty: Do proper CSS when Bug 428227 lands classic small icons for navigator and messenger */ 

>>+toolbar[labelalign="end"][iconsize="small"] #home-button > .toolbarbutton-icon {
>>+  width: 16px;
>>+  height: 16px;
> Duplicate. (I think it's OK to leave one of these styles in.)
Fixed.
Also I fogot to add that in XPFE the bookmark-item class overrides any other style and forces the home icon to 16 by 16. :P

>> #home-button:hover {
>>+  -moz-image-region: rect(120px 59px 149px 30px);
>>+  text-decoration: underline;
>>+} 
> Hmm, the cursor and pointer won't look so good if the button is moved.
I've separated the pointer styles and made them specific to the personal toolbar.

******* MODERN *******

>>+toolbar[mode="text"] .toolbarbutton-1,
>>+toolbar[labelalign="end"] .toolbarbutton-1 {
>>+  color: #000;
> I think this should be inherit;
Fixed.

>>+  padding: 0px 2px !important;
> This is to match bookmark items?
Yes.

>>+toolbar[iconsize=small] .toolbarbutton-menubutton-dropmarker {
> Missing "s around "small"
Fixed.

>>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-stack,
>>+toolbar[labelalign="end"]:not([mode="icons"]) .toolbarbutton-1 > .toolbarbutton-menubutton-stack {
>>+  display: -moz-box;
>>+  -moz-box-orient: horizontal;
> I don't think you can change the orient of a stack.
Of course not, but then I'm not changing the orientation of a stack, I'm changing it on a -moz-box (after changing the stack to a box). WFM.

> What style changes the display that you need to change it back?
Err. The css that applies the toolbarbutton-1 binding that creates the stack??

>>+toolbar[mode="text"] .toolbarbutton-1 > .toolbarbutton-menubutton-stack
>>+  > .toolbarbutton-menubutton-dropmarker,
>>+toolbar[labelalign="end"]:not([mode="icons"]) .toolbarbutton-1 > .toolbarbutton-menubutton-stack
>>+  > .toolbarbutton-menubutton-dropmarker {
>>+  margin: 0px 2px !important;
> Doesn't this style belongs in button.css with the other dropmarker styles?
Moved and consolidated.

>>+ /* XXXRatty: copied min-width: 0px from navigator.css,
>>+    has no effect in large icon mode. But makes small icon mode
>>+    look better. Centralize/move this style to button.css??
>>+ */
>> .toolbarbutton-1 {
>>   list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>>+  min-width: 0px !important;
>> }
> The default button width without a min-width style would be 44px;
> maybe you could adjust the padding or margin to avoid a min-width.
Hmm. I think that the problem is that the labels are of varying widths. Using padding and/or margins would mean special casing almost every button, and then what about buttons from extensions?

>>+/* ::::: small primary toolbar buttons ::::: */
> I guess this is harder to remove because we do actually have some (but not all)
> small button images.
I've left them in but commented out with this comment:
/* XXXRatty Uncomment these for testing only

>> .toolbarbutton-1 > stack > .toolbarbutton-menubutton-button 
>>   > .toolbarbutton-icon,
>> .toolbarbutton-1 > .toolbarbutton-icon {
>>+  display: inline;
>>+  /* Ratty: !important causes problems with fullscreen controls */
>> }
>> 
>> toolbar[mode="text"] .toolbarbutton-1 .toolbarbutton-icon {
>>+  display: none;
>>+  /* Ratty: !important causes problems with fullscreen controls */
>> }
> Why do we need to keep these styles at all?
Removed.

>>+toolbar[iconsize="small"] .toolbarbutton-1 {
>>+  margin-top: 0;
> ???

>>+toolbar[mode="full"][iconsize="small"] .toolbarbutton-1 {
>>+  margin-top: 3px !important;
> ???
Well I fiddled with the top margin until they looked roughly vertically
centred (And the toolbarbutton-1 class is now vertically centred instead of start).

>>-#search-button[toolbarmode="small"] > .button-box > .button-text {
> Where did this go?
Consolidated, somewhere else I have this style:

toolbar[mode="text"] #nav-bar-inner > #search-button > .button-box
  > .button-icon,
toolbar[mode="icons"] #nav-bar-inner > #search-button > .button-box
  > .button-text {
  display: none;
}

> The diff in this area was very difficult to read, it's hard to tell that you're
> not (I hope!) really changing anything.

I have reorganized the changes to put several of the additons at the end of the file. This should simplify the diffs. I'll move them back to a more logical location in a clean up bug/patch.

>> #back-button > .toolbarbutton-menubutton-stack
>>     > .toolbarbutton-menubutton-dropmarker,
>> #forward-button > .toolbarbutton-menubutton-stack
>>     > .toolbarbutton-menubutton-dropmarker
>> {
>>-  margin: 30px 0px 0px 34px;
>>+  margin: 20px 0px 0px 34px;
> What's the default margin here (I couldn't find it)?

I think somewhere in global/toolbarbutton.css there is this:

.toolbarbutton-menubutton-dropmarker {
...etc...
  margin: 20px 5px 15px 40px;
...etc...
}

Comment 43

[[e-mail]]

> I'd rather leave it in for the customize toolbar bit in the next bug.
Maybe we should have a placeholder image? (I'm not sure what it would be for Classic but for Modern it could be a blank button.)

>Of course not, but then I'm not changing the orientation of a stack, I'm
>changing it on a -moz-box (after changing the stack to a box). WFM.
I don't like this idea. Please either change the binding to one without a stack or tweak the margins so that the text and dropmarker don't overlap.

> .toolbarbutton-1 {
>   list-style-image: url("chrome://messenger/skin/icons/btn1.gif");
>+  min-width: 0px !important;
> }
Still not sure what the point of this is.

>Well I fiddled with the top margin until they looked roughly vertically centred
Fiddling with margins is not the way to centre elements.

Comment 44

[[e-mail]]

I started reviewing this when my PC locked up and I had to power cycle it, and I can't be sure I remembered all the points that I was trying to make :-(

(From update of attachment 341394 [details] [diff] [review])
Sorry, I don't like those _define[GS]etter__ changes. Please undo them.

>+  labelalign.setAttribute("disabled", mode == "icons");
This should be mode != full

>+toolbar[iconsize="small"] #home-button {
Shouldn't there be a > in there? (you didn't remember it in all cases)

> #print-button > .toolbarbutton-menubutton-stack
>     > .toolbarbutton-menubutton-dropmarker
> {
>   margin: 20px 0px 0px 40px;
> }
Hmm, how is it that we don't need to override the margin in the message window?

Comment 45

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

(In reply to comment #44)
> (From update of attachment 341394 [details] [diff] [review])
> Sorry, I don't like those _define[GS]etter__ changes. Please undo them.

Why don't you like them, out of curiousity?

Comment 46

[Philip Chee]

Attachment: Patch v 2.1

>> I'd rather leave it in for the customize toolbar bit in the next bug.
> Maybe we should have a placeholder image? (I'm not sure what it would be for
> Classic but for Modern it could be a blank button.)

I thought about it then I read <http://blog.mozilla.com/faaborg/2008/10/24/firefox-themes-the-contention-between-visual-hierarchy-and-toolbar-customization/> specifically: <http://people.mozilla.com/~faaborg/files/20081021-visualHierarchyAndCustomization/squintTest.png>

For Classic we how have small icons so I've updated the css to use those. For Modern, I am keeping the large icons when in small mode for those buttons (in messenger) where we don't have a suitable graphic.

>> Of course not, but then I'm not changing the orientation of a stack, I'm
>> changing it on a -moz-box (after changing the stack to a box). WFM.
> I don't like this idea. Please either change the binding to one without a stack
> or tweak the margins so that the text and dropmarker don't overlap.

I've switched to using the original un-derived toolkit binding. I had to tweak a few styles that assume a <stack> in between.

>>+  min-width: 0px !important;
>> }
> Still not sure what the point of this is.
Removed.

>> Well I fiddled with the top margin until they looked roughly vertically centred
> Fiddling with margins is not the way to centre elements.
OK. Switched to using padding-top/padding-bottom.

> (From update of attachment 341394 [details] [diff] [review])
> Sorry, I don't like those _define[GS]etter__ changes. Please undo them.
Undone.

>>+  labelalign.setAttribute("disabled", mode == "icons");
> This should be mode != full
Fixed.

>>+toolbar[iconsize="small"] #home-button {
> Shouldn't there be a > in there? (you didn't remember it in all cases)
Fixed.

>> #print-button > .toolbarbutton-menubutton-stack
>>     > .toolbarbutton-menubutton-dropmarker
>> {
>>   margin: 20px 0px 0px 40px;
>> }
>Hmm, how is it that we don't need to override the margin in the message window?
It's a *different* icon/button. The form factor in messenger is different.

Comment 47

[Philip Chee]

Attachment: Small Icon mode. Left: no padding; Right: top and bottom 1px.

Screenshot as requested over #irc.

>>> Well I fiddled with the top margin until they looked roughly vertically centred
>> Fiddling with margins is not the way to centre elements.
> OK. Switched to using padding-top/padding-bottom.

The problem is that the normal sized navigation buttons have "white space" or padding built in to the graphics.
<http://mxr.mozilla.org/comm-central/source/suite/themes/modern/communicator/icons/common.png>
<http://mxr.mozilla.org/comm-central/source/suite/themes/modern/navigator/icons/browser.png>

But the small icon set doesn't have any extra white space around the "buttons":
<http://mxr.mozilla.org/comm-central/source/suite/themes/modern/communicator/icons/common-small.png>
<http://mxr.mozilla.org/comm-central/source/suite/themes/modern/navigator/icons/browser-small.png>

A 1px padding-top prevents the navigation buttons from touching the top border of the toolbar and a 1px padding-bottom stops the bottom of the "p" in "Stop" from touching the bottom border.

Comment 48

[[e-mail]]

Comment on attachment 345878 [details] [diff] [review]
Patch v 2.1

>+toolbar[iconsize="small"][mode="full"]:not([labelalign="end"]) .toolbarbutton-1 {
>+  padding-top: 1px !important;
>+  padding-bottom: 1px !important;
> }
Nit: I understand the case for padding small icons, but is it necessary to overcomplicate the rule with the mode/labelalign tests?

Comment 49

[Philip Chee]

> Nit: I understand the case for padding small icons, but is it necessary to
> overcomplicate the rule with the mode/labelalign tests?

You are right, The following works:

toolbar[iconsize="small"] .toolbarbutton-1 {

Comment 50

[Philip Chee]

Attachment: Patch v 2.2

1. Fix bit-rot
2. Fix CSS nit in Comment 48 and Comment 49

Changed the rule from:
toolbar[iconsize="small"][mode="full"]:not([labelalign="end"]) .toolbarbutton-1 {...
to
toolbar[iconsize="small"] .toolbarbutton-1 {...

Comment 51

[Stefan [:stefanh]]

Comment on attachment 347498 [details] [diff] [review]
Patch v 2.2

I think the overall look is good (icons in the toolbar in different modes etc). 

I'm having some problems with the context menu, though. The choices here are a mixture of things you can do with both toolbars and the "context" toolbar (the sub-menu). That doesn't seem logical to me. It's not that anyone won't understand it, it's just that the "context" for the popup seems to be "the toolbar I right-clicked on" and "all toolbars".

Since I expect that here will be a "Customize..." option available in the future: What should the customize toolbar dialog contain? Personally, I'd expect it to contain all customizable options (adding/moving items and picking different icon sizes etc). In that case, how would this work with the "Customize this toolbar..." options?

Comment 52

[Robert Kaiser]

(In reply to comment #51)
> Since I expect that here will be a "Customize..." option available in the
> future

Could we please leave that discussion to bug 394288 and get this part in first?

Comment 53

[Stefan [:stefanh]]

(In reply to comment #52)
> (In reply to comment #51)
> > Since I expect that here will be a "Customize..." option available in the
> > future
> 
> Could we please leave that discussion to bug 394288 and get this part in first?

For the moment I find it hard to combine icon size/mode options for a single toolbar with options for icon size/mode for all toolbars in a customize toolbar dialog...

Comment 54

[Stefan [:stefanh]]

Attachment: Screenshot of small icons

Btw, is there some problem with the small icons? They seem to be a bit distorted.

Comment 55

[Philip Chee]

> Btw, is there some problem with the small icons? They seem to be a bit
> distorted.

I think this is Bug 462645 where I messed up and lost the alpha transparency for all the small icons in classic (navigator/communicator/messenger) when I resampled the originals down.

Comment 56

[Philip Chee]

>> Since I expect that here will be a "Customize..." option available in the
>> future

> Could we please leave that discussion to bug 394288 and get this part in first?

Yes pretty please. There is lots of time to fine tune the UI. This bug has been dragging on for too long. It's time to get the functionality in.

Comment 57

[Stefan [:stefanh]]

(In reply to comment #56)
> >> Since I expect that here will be a "Customize..." option available in the
> >> future
> 
> > Could we please leave that discussion to bug 394288 and get this part in first?

I was just asking, it wasn't any attempt to start a discussion. I simply find it hard to combine this with the customize toolbar feature. As for the popup, I would have skipped the hide/unhide toolbar stuff - that would make the sub-menu unneccecary and it would also make the context clear (only the toolbar you right-clicked on).

> Yes pretty please. There is lots of time to fine tune the UI. This bug has been
> dragging on for too long. It's time to get the functionality in.

As you requested ui-review from me, my points/questions are all related to the ui. If this is just about getting stuff in, you shouldn't need ui-review.

Comment 58

[Philip Chee]

> As you requested ui-review from me, my points/questions are all related to the
> ui. If this is just about getting stuff in, you shouldn't need ui-review.

Very sorry. I was thinking more about how the various button modes/sizes look on OSX. I am splitting off the question of the context menu design into Bug 464653

Comment 59

[Stefan [:stefanh]]

(In reply to comment #58)
> > As you requested ui-review from me, my points/questions are all related to the
> > ui. If this is just about getting stuff in, you shouldn't need ui-review.
> 
> Very sorry. I was thinking more about how the various button modes/sizes look
> on OSX.

Nothing to be sorry for :-)

Anyway, see comment #51 re the modes/sizes looks. Only tested on "Classic", though - but I find it hard to believe that Modern looks different on mac.

Comment 60

[[e-mail]]

Comment on attachment 347498 [details] [diff] [review]
Patch v 2.2

>+  var custommode = (toolbar.getAttribute("mode") || "full") !=
>+                   (toolbar.getAttribute("defaultmode") ||
>+                    toolbox.getAttribute("mode") ||
>+                    "full");
What is this trying to achieve? Are we expecting people to set defaultmode without setting mode? I'm just wondering whether it would be better to write
var custommode = toolbar.hasAttribute("defaultmode") &&
                 toolbar.getAttribute("defaultmode") !=
                 toolbar.getAttribute("mode);

>+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#menu-button");
You can't see it here but a ^M character crept in to the patch.
(There are a few in other places too.)

> #reload-button {
>   margin-right: 3px; /* balance out extra dropmarker space  */
>+}
Hmm, I don't think this will be useful with customisation ;-)

Comment 61

[[e-mail]]

(In reply to comment #60)
> (From update of attachment 347498 [details] [diff] [review])
> >+  var custommode = (toolbar.getAttribute("mode") || "full") !=
> >+                   (toolbar.getAttribute("defaultmode") ||
> >+                    toolbox.getAttribute("mode") ||
> >+                    "full");
> What is this trying to achieve?
I see now, there's a toolbox mode too.

Comment 62

[[e-mail]]

Comment on attachment 347498 [details] [diff] [review]
Patch v 2.2

>+toolbar[iconsize="small"] .toolbarbutton-1 {
>+  padding-top: 1px !important;
>+  padding-bottom: 1px !important;
> }
I think a 1px padding all round is better. Actually, can you do this as a transparent border? Also, this probably belongs in button.css

r+sr=me with all my latest nits fixed.

Comment 63

[Philip Chee]

Attachment: Patch v 2.3

>>+  -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#menu-button");
>You can't see it here but a ^M character crept in to the patch.
>(There are a few in other places too.)
Removed four occurrences of \x0D 

>> #reload-button {
>>   margin-right: 3px; /* balance out extra dropmarker space  */
>>+}
>Hmm, I don't think this will be useful with customisation ;-)
Removed.

>>+toolbar[iconsize="small"] .toolbarbutton-1 {
>>+  padding-top: 1px !important;
>>+  padding-bottom: 1px !important;
>> }
>I think a 1px padding all round is better. Actually, can you do this as a
>transparent border? Also, this probably belongs in button.css

Moved to button.css and converted to border. I tried an all round border but I didn't like the extra white space to the left and to the right of each button and makes them look too spaced out especially in the Messenger toolbar which has more than a few buttons. So I used the following border styles instead:

+  border-top: 1px solid transparent !important;
+  border-bottom: 1px solid transparent !important;

Comment 64

[Philip Chee]

Attachment: [for checkin] Patch v 2.3.1

OK stefanh (over IRC) pointed out that without the left/right 1px: border the forward and backward buttons are too close together, changing back to:

+toolbar[iconsize="small"] .toolbarbutton-1 {
+  border: 1px solid transparent !important;
+}

carrying forward r+/sr+

Comment 65

[Karsten Düsterloh]

Landed attachment 348164 [details] [diff] [review] on Hg trunk as <http://hg.mozilla.org/comm-central/rev/66e6db37bcc5>.

Comment 66

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #55)
> > Btw, is there some problem with the small icons? They seem to be a bit
> > distorted.
> 
> I think this is Bug 462645 where I messed up and lost the alpha transparency
> for all the small icons in classic (navigator/communicator/messenger) when I
> resampled the originals down.

Is there an equivalent bug for navigatoricons-small.png? They look distorted, too (current self-compiled nightly, WinXP, Classic theme).

Also at least for me the toolbars do not change when activating Print Preview from a browser window (i.e. the browser toolbars stay while the content area switches to Print Preview).

Other than that: Great work!

Comment 67

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #66)
> (In reply to comment #55)
> > > Btw, is there some problem with the small icons? They seem to be a bit
> > > distorted.
> > 
> > I think this is Bug 462645 where I messed up and lost the alpha transparency
> > for all the small icons in classic (navigator/communicator/messenger) when I
> > resampled the originals down.
> 
> Is there an equivalent bug for navigatoricons-small.png? They look distorted,
> too (current self-compiled nightly, WinXP, Classic theme).

Forget that, found bug 464936 under Depends on...

Comment 68

[Philip Chee]

> Also at least for me the toolbars do not change when activating Print Preview
> from a browser window (i.e. the browser toolbars stay while the content area
> switches to Print Preview).

And neither does full screen F11. This was working in an earlier patch and I forgot to test for this in the latest patch. I'll file a bug (Sigh)

Comment 69

[Philip Chee]

Filed Bug 465020 for the full screen and print preview problem

Comment 70

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081122 SeaMonkey/2.0a2pre - Build ID: 20081122000456

"Verified" on Linux trunk.

I just noticed that right-clicking a toolbar gave a menu with checkboxes to display the current window's toolbars and "Settings for this toolbar" -- icons and/or text, "small" icons or not. (I mustn't have been paying attention lately, that I didn't notice it earlier.)

I tried it with KaiRo's "Early Blue" theme and with "SeaMonkey Modern". Strangely enough, in Modern MailNews (but not Browser), selecting "small icons" gives icons most of which are the same size ("Stop" and throbber are noticeably smaller though), but wider on average. (EarlyBlue MailNews "small" icons are actually smaller, as well as Modern Browser icons.) One icon (from the Remove Duplicate Messages extension) displays only a featureless grey background when "small" but I guess this is the extension's problem, not SeaMonkey's.

Comment 71

[Philip Chee]

> Strangely enough, in Modern MailNews (but not Browser), selecting "small icons"
> gives icons most of which are the same size ("Stop" and throbber are noticeably
> smaller though),

We don't have the necessary small icons for most of the buttons in the messenger window (and  standalone message window). In an earlier patch I used CSS to squeeze the graphics down but Neil didn't like the result (they looked really really grotty).

cc Eyal Rozenberg the author of <http://removedupes.mozdev.org/>

> One icon (from the Remove
> Duplicate Messages extension) displays only a featureless grey background when
> "small" but I guess this is the extension's problem, not SeaMonkey's.

Well since remove dups works in Thunderbird as well there should be small icon support somewhere in that extension. Over to you Eyal.

Comment 72

[Robert Kaiser]

Modern and EarlyBlue don't have full support of small icons yet, so the problems you see there are natural.