Closed Bug 484968 Opened 15 years ago Closed 14 years ago

Make SeaMonkey tab bar scrollable to cope with tab overflow

Categories

(SeaMonkey :: Tabbed Browser, enhancement)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(seamonkey2.1 wanted)

Status:
RESOLVED FIXED
Milestone:
seamonkey2.1b2
Tracking Flags:
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: simon, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 14 obsolete files)

Screenshot of the arrowscrollbox, allTabs pulldown and lwtheme support.
285.31 KB, image/png
Details
With new imagery
45.03 KB, patch
philip.chee
: feedback+
Details | Diff | Splinter Review
Patch v1.6a Port theme changes to Mac Classic r=IanN
46.31 KB, patch
philip.chee
: review+
neil
: superreview+
Details | Diff | Splinter Review
Mac tweaks
2.17 KB, patch
stefanh
: review+
Details | Diff | Splinter Review
Patch Bv1.0 Minor code cleanup. [checked-in Comment 92]
3.04 KB, patch
neil
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090324 SeaMonkey/2.0b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090324 SeaMonkey/2.0b1pre

In SeaMonkey it is possible to open more tabs than the browser can display. The tabs are open but invisible and the only was to view them is by closing some of the tabs that you can see. The Firefox solution to this is to make the tab bar scrollable. 
There are a couple of bugs about this already, bug 155325 and bug 221684, but they are very old, very confusing, quite unfocused and they've been off the radar for years. They do have some patches, though, including some by Neil Parkway and Chris Thomas. I think the Firefox bug that fixed this was bug 318168, although it looks like that also introduced those ugly red close buttons. DO NOT port those. Firefox doesn't scrunch the tabs up nice and small like SeaMonkey does either. 
This feature is going to be more important in future as it is proposed that there will soon be not only browser tabs but mail tabs as well, and perhaps one day chatzilla composer and lightning all in their own tabs.
This is an attempt to get what I think is a key feature back on the radar before SM 2.0 stops taking new features.

Reproducible: Always

Steps to Reproduce:
Open about 30 or 40 tabs at once or as many as it takes to overflow the bar. Eventually tabs will begin to disappear.
Actual Results:  
Tabs vanish

Expected Results:  
A mechanism for accessing invisible tabs should exist (other than deleting some of the tabs that are open.
Version: unspecified → Trunk
Flags: wanted-seamonkey2?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Enhancement, so can't go into 2.0 any more after the feature freeze - would be nice to get some better tab overflow handling in a next release though.
Severity: normal → enhancement
Flags: wanted-seamonkey2.0? → wanted-seamonkey2.0-
Flags: wanted-seamonkey2.1?
Don't we already have the tab bar overflow code in tabmail? (Of course it still would need someone to transplant it to the browser.)
See also a different workaround (spread overflowing tabs onto additional rows) as part of my SeaMonkey userChrome.css, http://users.skynet.be/antoine.mechelynck/other/userChrome-seamonkey.css
I don't think we should have bug 155325 and this bug here both open about the same issue. Please dupe one to the other, I don't care which one.
status-seamonkey2.1: --- → wanted
Flags: wanted-seamonkey2.1?
Keywords: helpwanted
Blocks: 471388
Blocks: SMtabAPI
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attached patch WIP Patch v0.1 (obsolete) — — Splinter Review 
Done:
 1. Tabs are now scrollable.
 2. Updated themes to style the arrowscroll-box.
ToDo:
 1. AllTabs popup.
 2. Bug 543206 [Tab opening animation] Not strictly part of scrollable tabs but the Firefox code is so entwined that separating the bits that handle this from the AllTabs popup makes me go cross-eyed.

> +  <binding id="tabbrowser-arrowscrollbox"
> +           extends="chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll">
> 
> +      <method name="_canScrollToElement">
> +        <parameter name="tab"/>
> +        <body><![CDATA[
> +          return !tab.pinned && !tab.hidden;

This is a NOOP since we don't have pinned tabs at the moment but it's easier to port this now.

> --- a/suite/themes/classic/mac/navigator/tabbrowser.css
> +/* ::::: Tab scrollbox arrow ::::: */

Copied from stefanh's work in tabmail.xml

> +  list-style-image: url("chrome://messenger/skin/icons/tab-arrow-right.png");

I wonder if we can put this in some shared location under common|communicator.

> diff --git a/suite/themes/classic/navigator/icons/tab-arrow-left.png b/suite/themes/classic/navigator/icons/tab-arrow-left.png
> new file mode 100644

Copied from Winstripe.

> +++ b/suite/themes/classic/navigator/tabbrowser.css
> +/* ::::: Tab scrollbox arrow ::::: */

Copied from Winstripe. Plus some styles from tabbrowser tabs to make the arrowscrollboxes match the tabs.

> +  border-radius: 4px 2px 0 0;

My own addition because I think a slight radius looks better than something completely square.

> +.tabbrowser-arrowscrollbox > .scrollbutton-down[notifybgtab] {
> +  background-color: Highlight;
> +  -moz-transition: none;

No idea what this does, Just copying from *stripe.

> diff --git a/suite/themes/modern/navigator/icons/tabscroll.png b/suite/themes/modern/navigator/icons/tabscroll.png
> new file mode 100644

Copied from Past Modern.

> +++ b/suite/themes/modern/navigator/tabbrowser.css

Adapted from Past Modern.
Attachment #488491 - Flags: feedback?(iann_bugzilla)
Comment on attachment 488491 [details] [diff] [review]
WIP Patch v0.1

Stefanh: I've just copied the arrowscrollbox styles that you did for tabmail. Not sure how it applies to the tabbrowser scrollbox so asking for feedback.
Attachment #488491 - Flags: feedback?(stefanh)
Comment on attachment 488491 [details] [diff] [review]
WIP Patch v0.1

># HG changeset patch
># User Philip Chee <philip.chee@gmail.com>
># Date 1288973816 -28800
># Node ID 18c8510fe2ae0c96baf6c7b1aea57b2767913304
># Parent  2161775b088dcb7814d63e15ab63b72914c44ee5
>Bug 484968 Make SeaMonkey tab bar scrollable to cope with tab overflow
>
>diff --git a/suite/browser/navigator.css b/suite/browser/navigator.css
>--- a/suite/browser/navigator.css
>+++ b/suite/browser/navigator.css
>@@ -9,16 +9,20 @@
> tabbrowser {
>   -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser");
> }
> 
> .tabbrowser-tabs {
>   -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser-tabs");
> }
> 
>+.tabbrowser-arrowscrollbox {
>+  -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser-arrowscrollbox");
>+}
>+
> .tabs-closebutton-box > .tabs-closebutton {
>   -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton");
> }
> 
> /* ::::: urlbar autocomplete ::::: */
> 
> #urlbar {
>   -moz-binding: url("chrome://navigator/content/urlbarBindings.xml#urlbar");
>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
>--- a/suite/browser/tabbrowser.xml
>+++ b/suite/browser/tabbrowser.xml
>@@ -110,17 +110,20 @@
>                     xbl:inherits="onnewtab,onnewtabclick"
>                     onclosetab="var node = this.parentNode;
>                                 while (node.localName != 'tabbrowser')
>                                   node = node.parentNode;
>                                 node.removeCurrentTab();">
>             <xul:tab selected="true" validate="never"
>                      onerror="this.parentNode.parentNode.parentNode.parentNode.addToMissedIconCache(this.getAttribute('image'));
>                               this.removeAttribute('image');"
>-                     maxwidth="250" width="0" minwidth="30" flex="100"
>+                     maxwidth="250"
>+                     minwidth="30"
>+                     width="0"
>+                     flex="100"
>                      class="tabbrowser-tab" label="&untitledTab;" crop="end"/>
>           </xul:tabs>
>         </xul:hbox>
>         <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer">
>           <xul:notificationbox class="browser-notificationbox" xbl:inherits="popupnotification">
>             <xul:browser flex="1" type="content-primary" xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autocompletepopup"/>
>           </xul:notificationbox>
>         </xul:tabpanels>
>@@ -1642,16 +1645,17 @@
>                             this.mStringBundle.getString("tabs.untitled");
> 
>             var l = this.tabs.length;
>             switch (l) {
>               case 1:
>                 // add a new blank tab to replace the one we're about to close
>                 // (this ensures that the remaining tab is as good as new)
>                 this.addTab("about:blank");
>+                this.tabContainer._fillTrailingGap();
>                 l++;
>                 // fall through
>               case 2:
>                 if (this.mPrefs.getBoolPref("browser.tabs.autoHide"))
>                   this.mStrip.collapsed = true;
>             }
> 
>             var index = this.getTabIndex(aTab);
>@@ -2652,20 +2656,20 @@
>               !this.mPrefs.getBoolPref("browser.tabs.forceHide") &&
>               window.toolbar.visible)
>             this.mStrip.collapsed = false;
> 
>           var t = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "tab");
>           t.setAttribute("label", this.mStringBundle.getString("tabs.untitled"));
>           t.setAttribute("crop", "end");
>           t.className = "tabbrowser-tab";
>-          t.setAttribute("maxwidth", 250);
>-          t.setAttribute("minwidth", 30);
>-          t.setAttribute("width", 0);
>-          t.setAttribute("flex", 100);
>+          t.maxWidth = this.tabContainer.mTabMaxWidth;
>+          t.minWidth = this.tabContainer.mTabMinWidth;
>+          t.width = 0;
>+          t.setAttribute("flex", "100");
>           t.setAttribute("validate", "never");
>           t.setAttribute("onerror", "this.parentNode.parentNode.parentNode.parentNode.addToMissedIconCache(this.getAttribute('image')); this.removeAttribute('image');");
>           this.referenceTab = t;
> 
>           var os = Components.classes["@mozilla.org/observer-service;1"]
>                              .getService(Components.interfaces.nsIObserverService);
>           os.addObserver(this, "browser:purge-session-history", false);
>         ]]>
>@@ -2746,43 +2750,220 @@
>       <handler event="keypress" keycode="VK_RIGHT" modifiers="accel" action="if (event.target == this) { this.moveTabRight(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_UP" modifiers="accel" action="if (event.target == this) { this.moveTabBackward(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_DOWN" modifiers="accel" action="if (event.target == this) { this.moveTabForward(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_HOME" modifiers="accel" action="if (event.target == this) { this.moveTabToStart(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_END" modifiers="accel" action="if (event.target == this) { this.moveTabToEnd(); event.preventDefault(); }"/>
>     </handlers>
>   </binding>
> 
>+  <binding id="tabbrowser-arrowscrollbox"
>+           extends="chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll">
>+    <implementation>
>+      <!-- Override scrollbox.xml method, since our scrollbox's children are
>+           inherited from the binding parent -->
>+      <method name="_getScrollableElements">
>+        <body><![CDATA[
>+          return Array.filter(document.getBindingParent(this).childNodes,
>+                              this._canScrollToElement, this);
>+        ]]></body>
>+      </method>
>+      <method name="_canScrollToElement">
>+        <parameter name="tab"/>
>+        <body><![CDATA[
>+          return !tab.pinned && !tab.hidden;
>+        ]]></body>
>+      </method>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="underflow"><![CDATA[
>+         if (event.detail == 0)
>+           return; // Ignore vertical events
>+
>+         var tabs = document.getBindingParent(this);
>+         tabs.removeAttribute("overflow");
>+      ]]></handler>
>+      <handler event="overflow"><![CDATA[
>+         if (event.detail == 0)
>+           return; // Ignore vertical events
>+
>+         var tabs = document.getBindingParent(this);
>+         tabs.setAttribute("overflow", "true");
>+         this.ensureElementIsVisible(tabs.selectedItem, false);
>+      ]]></handler>
>+    </handlers>
>+  </binding>
>+
>   <binding id="tabbrowser-tabs"
>            extends="chrome://global/content/bindings/tabbox.xml#tabs">
>     <resources>
>       <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>     </resources>
> 
>     <content>
>       <xul:stack flex="1" class="tabs-stack">
>         <xul:vbox>
>           <xul:spacer flex="1"/>
>           <xul:hbox class="tabs-bottom" align="center"/>
>         </xul:vbox>
>         <xul:vbox>
>           <xul:hbox>
>             <xul:stack>
>               <xul:spacer class="tabs-left"/>
>-              <xul:toolbarbutton class="tabs-newbutton" xbl:inherits="oncommand=onnewtab,onclick=onnewtabclick,tooltiptext=tooltiptextnew"/>
>+              <xul:toolbarbutton class="tabs-newbutton"
>+                                 anonid="tabstrip-newbutton"
>+                                 xbl:inherits="oncommand=onnewtab,onclick=onnewtabclick,tooltiptext=tooltiptextnew"/>
>             </xul:stack>
>-            <xul:hbox flex="1" style="min-width: 1px; overflow: -moz-hidden-unscrollable;">
>-              <children/>
>+            <xul:arrowscrollbox anonid="arrowscrollbox"
>+                                class="tabbrowser-arrowscrollbox"
>+                                flex="1"
>+                                xbl:inherits="smoothscroll"
>+                                orient="horizontal"
>+                                style="min-width: 1px;">
>+              <children includes="tab"/>
>               <xul:spacer class="tabs-right" flex="1"/>
>-            </xul:hbox>
>+            </xul:arrowscrollbox>
>+            <children/>
>             <xul:stack>
>               <xul:spacer class="tabs-right"/>
>               <xul:hbox class="tabs-closebutton-box" align="center" pack="end">
>-                <xul:toolbarbutton class="tabs-closebutton close-button" xbl:inherits="disabled=disableclose,oncommand=onclosetab,tooltiptext=tooltiptextclose"/>
>+                <xul:toolbarbutton class="tabs-closebutton close-button"
>+                                   align="center"
>+                                   pack="end"
>+                                   anonid="tabstrip-closebutton"
>+                                   xbl:inherits="disabled=disableclose,oncommand=onclosetab,tooltiptext=tooltiptextclose"/>
>               </xul:hbox>
>             </xul:stack>
>           </xul:hbox>
>           <xul:spacer class="tabs-bottom-spacer"/>
>         </xul:vbox>
>       </xul:stack>
>     </content>
>+
>+    <implementation implements="nsITimerCallback, nsIDOMEventListener">
>+      <constructor>
>+        <![CDATA[
>+          this.mTabMinWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMinWidth");
>+          this.mTabMaxWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMaxWidth");
>+          this.mTabClipWidth = this.mPrefs.getIntPref ("browser.tabs.tabClipWidth");
>+          this.firstChild.minWidth = this.mTabMinWidth;
>+          this.firstChild.maxWidth = this.mTabMaxWidth;
>+          window.addEventListener("resize", this, false);
>+        ]]>
>+      </constructor>
>+
>+      <destructor>
>+        <![CDATA[
>+        ]]>
>+      </destructor>
>+
>+      <field name="mPrefs" readonly="true">
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefBranch2);
>+      </field>
>+
>+      <field name="mTabstripWidth">0</field>
>+
>+      <field name="mTabstrip">
>+        document.getAnonymousElementByAttribute(this, "anonid", "arrowscrollbox");
>+      </field>
>+
>+      <field name="mTabMinWidth">100</field>
>+      <field name="mTabMaxWidth">250</field>
>+      <field name="mTabClipWidth">140</field>
>+
>+      <method name="_handleTabSelect">
>+        <body>
>+          <![CDATA[
>+            this.mTabstrip.ensureElementIsVisible(this.selectedItem);
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="_fillTrailingGap">
>+        <body>
>+          <![CDATA[
>+          try {
>+            // if we're at the right side (and not the logical end,
>+            // which is why this works for both LTR and RTL)
>+            // of the tabstrip, we need to ensure that we stay
>+            // completely scrolled to the right side
>+            var tabStrip = this.mTabstrip;
>+            if (tabStrip.scrollPosition + tabStrip.scrollClientSize >
>+                tabStrip.scrollSize)
>+              tabStrip.scrollByPixels(-1);
>+          } catch (e) {}
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="handleEvent">
>+        <parameter name="aEvent"/>
>+        <body>
>+          <![CDATA[
>+            switch (aEvent.type)
>+            {
>+              case "resize":
>+                var width = this.mTabstrip.boxObject.width;
>+                if (width != this.mTabstripWidth)
>+                {
>+                  this._fillTrailingGap();
>+                  this._handleTabSelect();
>+                  this.mTabstripWidth = width;
>+                }
>+                break;
>+            }
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <field name="mAllTabsPopup">
>+        document.getAnonymousElementByAttribute(this, "anonid", "alltabs-popup");
>+      </field>
>+
>+      <field name="_animateElement">
>+        this.mTabstrip._scrollButtonDown;
>+      </field>
>+
>+      <method name="_notifyBackgroundTab">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+          var scrollRect = this.mTabstrip.scrollClientRect;
>+          var tab = aTab.getBoundingClientRect();
>+
>+          // Is the new tab already completely visible?
>+          if (scrollRect.left <= tab.left && tab.right <= scrollRect.right)
>+            return;
>+
>+          if (this.mTabstrip.smoothScroll) {
>+            let selected = this.selectedItem.getBoundingClientRect();
>+
>+            // Can we make both the new tab and the selected tab completely visible?
>+            if (!selected ||
>+                Math.max(tab.right - selected.left, selected.right - tab.left) <=
>+                  scrollRect.width) {
>+              this.mTabstrip.ensureElementIsVisible(aTab);
>+              return;
>+            }
>+
>+            this.mTabstrip._smoothScrollByPixels(this.mTabstrip._isRTLScrollbox ?
>+                                                 selected.right - scrollRect.right :
>+                                                 selected.left - scrollRect.left);
>+          }
>+
>+          if (!this._animateElement.hasAttribute("notifybgtab")) {
>+            this._animateElement.setAttribute("notifybgtab", "true");
>+            setTimeout(function (ele) {
>+              ele.removeAttribute("notifybgtab");
>+            }, 150, this._animateElement);
>+          }
>+          ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="TabSelect" action="this._handleTabSelect();"/>
>+    </handlers>
>   </binding>
> </bindings>
>diff --git a/suite/themes/classic/mac/navigator/tabbrowser.css b/suite/themes/classic/mac/navigator/tabbrowser.css
>--- a/suite/themes/classic/mac/navigator/tabbrowser.css
>+++ b/suite/themes/classic/mac/navigator/tabbrowser.css
>@@ -112,16 +112,59 @@ tabpanels {
> .tabbrowser-tab[beforeselected="true"]:-moz-locale-dir(ltr) {
>   -moz-border-right-colors: transparent transparent;
> }
> 
> .tabbrowser-tab[beforeselected="true"]:-moz-locale-dir(rtl) {
>   -moz-border-left-colors: transparent transparent;
> }
> 
>+/* ::::: Tab scrollbox arrow ::::: */
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up,
>+.tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  border: 0;
>+  padding: 0 4px;
>+  margin: 0;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up {
>+  border-right: 2px solid;
>+  -moz-border-right-colors: rgba(0, 0, 0, 0.19) transparent;
>+  list-style-image: url("chrome://messenger/skin/icons/tab-arrow-left.png");
>+  -moz-image-region: rect(0px, 7px, 11px, 0px);
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up[disabled="true"] {
>+  -moz-image-region: rect(0px, 14px, 11px, 7px);
>+  -moz-border-right-colors: transparent transparent;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  border-left: 2px solid;
>+  -moz-border-left-colors: rgba(0 ,0 ,0 , 0.19) transparent;
>+  list-style-image: url("chrome://messenger/skin/icons/tab-arrow-right.png");
>+  -moz-image-region: rect(0px, 7px, 11px, 0px);
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down[disabled="true"] {
>+  -moz-image-region: rect(0px, 14px, 11px, 7px);
>+  -moz-border-left-colors: transparent transparent;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:hover:not([disabled="true"]),
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:hover:not([disabled="true"]) {
>+  background-color: rgba(0, 0, 0, 0.1);
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:hover:active:not([disabled="true"]),
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:hover:active:not([disabled="true"]) {
>+  background-color: rgba(0, 0, 0 , 0.2);
>+}
>+
> /* ::::: close button ::::: */
> 
> .tabs-closebutton {
>   margin: 3px;
>   padding: 1px;
>   list-style-image: url("chrome://global/skin/icons/closetab.png");
> }
> 
>diff --git a/suite/themes/classic/navigator/icons/tab-arrow-left.png b/suite/themes/classic/navigator/icons/tab-arrow-left.png
>new file mode 100644
>index 0000000000000000000000000000000000000000..0ef2b1ae61e2d4396ac70fab94dab6a7b49ef1aa
>GIT binary patch
>literal 460
>zc%17D@N?(olHy`uVBq!ia0vp^azHG|!3-q-?s(%0q*es@gn;Nf&)*Y?UVixc;{6vj
>z4NbNBS!zvl)LQ0g8(ZjG*t*Z(9vB{DYGr5Izua=-3ad#gZJgblJbnB^A_C@b4vL5g
>zn!h<@`QFflTO$)vVw2KilhYH@vy!s%Iop<T3yBMKt`O{7CnPN=CZ{MibC1Nt4RUJQ
>zDhm$im{{qX+Zb5c8(G;cc78Vx=vJ+gAirP+Miw@9E>SUY2}xNkZDTjz8DOw<>2fgG
>zi~uvk13v?Gt@Lzp4ABT)d-1r?Aq4@p1Sglv3IZl&y&I?BIQ@J7Obf{f8-utzx3}AG
>z-pt@sufZ?eWUKzSLCyC~%83?_j-xui7v1=-$$0ASfn^qcl?{q(xqMrg)%=X*e$HFA
>z#>?4d#SXp7I|fspTLx7Ad9_i(uRkw#!K`E9{c~E=EaGEswPj83$e(9vyxMionk8v>
>zZ``vk5S}UzWJUOEY>ePh{I@4**0v4J`}ZA*%nv-p{r#BF&r3i@GkCiCxvX<aXaWEg
>CQq4;M
>
>diff --git a/suite/themes/classic/navigator/tabbrowser.css b/suite/themes/classic/navigator/tabbrowser.css
>--- a/suite/themes/classic/navigator/tabbrowser.css
>+++ b/suite/themes/classic/navigator/tabbrowser.css
>@@ -43,16 +43,65 @@ tabpanels {
> }
> 
> .tabbrowser-tab[selected="true"] {
>   margin-bottom: 0px;
>   padding-top: 2px; /* compensates the top margin of background tabs */
>   padding-bottom: 3px; /* compensates the bottom margin of background tabs */
> }
> 
>+/* ::::: Tab scrollbox arrow ::::: */
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up,
>+.tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  list-style-image: url("chrome://navigator/skin/icons/tab-arrow-left.png");
>+  -moz-image-region: rect(0, 15px, 17px, 0);
>+  margin: 2px 0 1px;
>+  padding-top: 0;
>+  padding-bottom: 0;
>+  border-top: 2px solid;
>+  border-right: 2px solid;
>+  border-left: 2px solid;
>+  border-bottom: 1px solid ThreeDHighlight;
>+  -moz-border-top-colors: ThreeDHighlight ThreeDLightShadow;
>+  -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>+  -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>+  border-radius: 4px 2px 0 0;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl),
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr) {
>+  -moz-border-right-colors: ThreeDHighlight ThreeDShadow;
>+  border-radius: 2px 4px 0 0;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up[disabled="true"],
>+.tabbrowser-arrowscrollbox > .scrollbutton-down[disabled="true"] {
>+  opacity: .4;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):hover:active,
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):hover:active {
>+  -moz-image-region: rect(0, 30px, 17px, 15px);
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:-moz-locale-dir(rtl) > .toolbarbutton-icon,
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:-moz-locale-dir(ltr) > .toolbarbutton-icon {
>+  -moz-transform: scaleX(-1);
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  -moz-transition: 1s background-color ease-out;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down[notifybgtab] {
>+  background-color: Highlight;
>+  -moz-transition: none;
>+}
>+
> /* ::::: close button ::::: */
> 
> .tabs-closebutton {
>   margin: 3px;
>   list-style-image: url("chrome://communicator/skin/icons/close-button.gif");
> }
> 
> .tabs-closebutton > .toolbarbutton-icon {
>diff --git a/suite/themes/modern/navigator/icons/tabscroll.png b/suite/themes/modern/navigator/icons/tabscroll.png
>new file mode 100644
>index 0000000000000000000000000000000000000000..d5053d04734bec710a4b154c6106b16a8c7f56d2
>GIT binary patch
>literal 151
>zc%17D@N?(olHy`uVBq!ia0vp^B0$W^!VDx2ycetnQv3lvA+G-!7|I$at=_UvKv-ht
>z%$Y~GCe{F@7)yfuf*Bm1-ADs+%sgEjLn>}1CnSg@1PG-h1SKRS2#6&VF(tCGxg|4-
>wswX<DiL<gLCM$}o8#Fd|E)hG)(z27`yB(M7aoYtafF>|_y85}Sb4q9e08qmz=Kufz
>
>diff --git a/suite/themes/modern/navigator/tabbrowser.css b/suite/themes/modern/navigator/tabbrowser.css
>--- a/suite/themes/modern/navigator/tabbrowser.css
>+++ b/suite/themes/modern/navigator/tabbrowser.css
>@@ -63,16 +63,84 @@ tab[busy] {
> }
> 
> .tab-icon {
>   -moz-margin-end: 3px;
>   width: 16px;
>   height: 16px;
> }
> 
>+/* ::::: Tab scrollbox arrow ::::: */
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up,
>+.tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  list-style-image: url("chrome://navigator/skin/icons/tabscroll.png");
>+  padding: 0 3px;
>+  margin: 0px;
>+  border-bottom: 3px solid;
>+  -moz-border-bottom-colors: #000000 transparent; /*#98A7B5;*/
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up {
>+  -moz-image-region: rect(0px, 5px, 9px, 0px);
>+  border-right: 2px solid;
>+  -moz-border-right-colors: #000000 transparent;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  -moz-image-region: rect(0px, 15px, 9px, 10px);
>+  border-left: 2px solid;
>+  -moz-border-left-colors: #000000 transparent;
>+  -moz-transition: 1s background-color ease-out;
>+}
>+
>+/* Bug 525724 */
>+.tabbrowser-arrowscrollbox > .scrollbutton-down[notifybgtab],
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:hover:not([disabled="true"]),
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:hover:active:not([disabled="true"]),
>+.tabbrowser-arrowscrollbox > .scrollbutton-down[disabled="true"] {
>+  -moz-transition: none;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down[notifybgtab] {
>+  background-color: #8C9AA8;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:hover:not([disabled="true"]) {
>+  -moz-border-right-colors: #000000 #86929E;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:hover:not([disabled="true"]) {
>+  -moz-border-left-colors: #000000 #EEF0F3;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up:hover:active:not([disabled="true"]) {
>+  background-color: #8C9AA8;
>+  border-top-color: #454C55;
>+  -moz-border-right-colors: #000000 #7D848D;
>+  border-bottom-color: #7D848D;
>+  border-left-color: #454C55;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:hover:active:not([disabled="true"]) {
>+  background-color: #8C9AA8;
>+  border-top-color: #454C55;
>+  border-right-color: #7D848D;
>+  border-bottom-color: #7D848D;
>+  -moz-border-left-colors: #000000 #454C55;
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-up[disabled="true"] {
>+  -moz-image-region: rect(0px, 10px, 9px, 5px);
>+}
>+
>+.tabbrowser-arrowscrollbox > .scrollbutton-down[disabled="true"] {
>+  -moz-image-region: rect(0px, 20px, 9px, 15px);
>+}
>+
> /* ::::: close button ::::: */
> 
> .tabs-closebutton {
>   margin: 0px 4px;
>   padding: 3px 2px;
>   border: none;
>   list-style-image: url("chrome://global/skin/icons/close.gif");
> }
Attachment #488491 - Flags: feedback?(mnyromyr)
Grr. Needing to clear the cache every time I edit an attachment is a pain.

> Created attachment 488491 [details] [diff] [review]
> WIP Patch v0.1

Mnyromyr, you worked on the tabmail implementation so perhaps you can comment on the arrowscrollbox code here.
Comment on attachment 488491 [details] [diff] [review]
WIP Patch v0.1

Bah forgot the jar.mn changes.
Attachment #488491 - Flags: feedback?(stefanh)
Attachment #488491 - Flags: feedback?(mnyromyr)
Attachment #488491 - Flags: feedback?(iann_bugzilla)
Attached patch WIP Patch v0.2 now with jar.mn (obsolete) — — Splinter Review 
WIP Patch v0.1

Done:
 1. Tabs are now scrollable.
 2. Updated themes to style the arrowscroll-box.
ToDo:
 1. AllTabs popup.
 2. Bug 543206 [Tab opening animation]
Attachment #488491 - Attachment is obsolete: true
Attachment #488513 - Flags: review?(iann_bugzilla)
Attachment #488513 - Flags: feedback?(jh)
Comment on attachment 488513 [details] [diff] [review]
WIP Patch v0.2 now with jar.mn

stefanh -> copied your Mac arrowscrollbox styles from tabmail but I don't know how well they fit in tabbrowser.

mnyromyr -> you did the tabmail implementation so could you have a look at the tabbrowser changes.
Attachment #488513 - Flags: ui-review?(stefanh)
Attachment #488513 - Flags: feedback?(mnyromyr)
Attachment #488513 - Flags: feedback?(jh) → feedback?(aqualon)
(In reply to comment #11) 
> stefanh -> copied your Mac arrowscrollbox styles from tabmail but I don't know
> how well they fit in tabbrowser.

I *think* they should fit. Just a heads-up that it will take some time before I can look at this since I'm currently away. I should be able to look at this in the middle of next week, though.
Will you be making sure the minimum size of a tab is nice and small, as it presently is in the SM browser, and not enormous like it is in Friedfox and SM's own tabmail?
This is controlled by a pref browser.tabs.tabMinWidth the default is 100 but you can set it as small as you like (but probably anything less than 32px is useless).
Comment on attachment 488513 [details] [diff] [review]
WIP Patch v0.2 now with jar.mn

Mac styling looks good (same as tabmail). One thing I've noticed is that this implementation behaves different that tabmail's when you select the left/rightmost (visible) tab: In your implementation the box scrolls so the whole tab is visible, but in tabmail it doesn't (your implementation is an improvement imo).
Attachment #488513 - Flags: ui-review?(stefanh) → ui-review+
> In your implementation the box scrolls so the
> whole tab is visible, but in tabmail it doesn't (your implementation is an
> improvement imo).

I am copying some of the newer code from Firefox. If all goes well I'll look into unforking the code or at least backporting the improvements to tabmail.

Our tabmail implementation is based on the Thunderbird tabmail and both are somewhat frozen in time. Unfortunately I can't copy all the latest Firefox improvements because there are just too many unresolved dependent bugs which we haven't ported yet.
As a short remark before I forget about it, the patch contains some CR line endings in suite/browser/navigator.css
I think you meant tabbrowser.xml couldn't find any DOS newlines in navigator.css.
(In reply to comment #18)
> I think you meant tabbrowser.xml couldn't find any DOS newlines in
> navigator.css.
Yes, scrolled too far in the file, sry.
Taboverflow complete patch including the allTabs menubutton.

Mnyromyr: The scrolldown arrow box animation has been simplified and follows Firefox 4.0. You might want to take these changes to tabmail.

> -                     maxwidth="250" width="0" minwidth="30" flex="100"
> +                     width="0" flex="100"
[....]
> -          t.setAttribute("maxwidth", 250);
> -          t.setAttribute("minwidth", 30);
> -          t.setAttribute("width", 0);
> -          t.setAttribute("flex", 100);
> +          t.style.maxWidth = this.tabContainer.mTabMaxWidth + "px";
> +          t.style.minWidth = this.tabContainer.mTabMinWidth + "px";
> +          t.width = 0;
> +          t.setAttribute("flex", "100");

Now controlled with CSS like in Firefox 3.6. Presumably to allow third party themes to style.

> +.scrollbutton-up:-moz-lwtheme-brighttext,
> +.scrollbutton-down:-moz-lwtheme-brighttext,
> +.tabs-alltabs-button:-moz-lwtheme-brighttext {
> +  background-color: rgba(0,0,0,.5);
> +}
> +
> +.scrollbutton-up:-moz-lwtheme-darktext,
> +.scrollbutton-down:-moz-lwtheme-darktext,
> +.tabs-alltabs-button:-moz-lwtheme-darktext {
> +  background-color: rgba(255,255,255,.5);
> +}

Added lwTheme support to Classic. Did not add lwtheme support to Modern because it didn't seem to work.
Attachment #488513 - Attachment is obsolete: true
Attachment #490468 - Flags: review?(iann_bugzilla)
Attachment #490468 - Flags: feedback?(mnyromyr)
Attachment #488513 - Flags: review?(iann_bugzilla)
Attachment #488513 - Flags: feedback?(mnyromyr)
Attachment #488513 - Flags: feedback?(aqualon)
Added Bug 612305 tabbrowser.xml is only interested in resize events for the chrome window, should ignore those for content windows.

> Taboverflow complete patch including the allTabs menubutton.
> 
> Mnyromyr: The scrolldown arrow box animation has been simplified and follows Firefox 4.0. You might want to take these changes to tabmail.
> 
> > -                     maxwidth="250" width="0" minwidth="30" flex="100"
> > +                     width="0" flex="100"
> [....]
> > -          t.setAttribute("maxwidth", 250);
> > -          t.setAttribute("minwidth", 30);
> > -          t.setAttribute("width", 0);
> > -          t.setAttribute("flex", 100);
> > +          t.style.maxWidth = this.tabContainer.mTabMaxWidth + "px";
> > +          t.style.minWidth = this.tabContainer.mTabMinWidth + "px";
> > +          t.width = 0;
> > +          t.setAttribute("flex", "100");
> 
> Now controlled with CSS like in Firefox 3.6. Presumably to allow third party themes to style.
> 
> > +.scrollbutton-up:-moz-lwtheme-brighttext,
> > +.scrollbutton-down:-moz-lwtheme-brighttext,
> > +.tabs-alltabs-button:-moz-lwtheme-brighttext {
> > +  background-color: rgba(0,0,0,.5);
> > +}
> > +
> > +.scrollbutton-up:-moz-lwtheme-darktext,
> > +.scrollbutton-down:-moz-lwtheme-darktext,
> > +.tabs-alltabs-button:-moz-lwtheme-darktext {
> > +  background-color: rgba(255,255,255,.5);
> > +}
> 
> Added lwTheme support to Classic. Did not add lwtheme support to Modern because it didn't seem to work.
>
Attachment #490468 - Attachment is obsolete: true
Attachment #491436 - Flags: review?(iann_bugzilla)
Attachment #491436 - Flags: feedback?(mnyromyr)
Attachment #490468 - Flags: review?(iann_bugzilla)
Attachment #490468 - Flags: feedback?(mnyromyr)
> Added Bug 612305 tabbrowser.xml is only interested in resize events for the
> chrome window, should ignore those for content windows.

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=490468&action=interdiff&newid=491436&headers=1
Comment on attachment 491436 [details] [diff] [review]
Patch v1.0a Tabs scrolling complete with AllTabs menubutton

From testing:
1) Drag and drop seems to get confused about where it is dropping to when you have enough tabs from scrolling to start happening.
2) Drag and drop does not scroll the tabs when you try and drag past the end of the visible tabs.
r-
Attachment #491436 - Flags: review?(iann_bugzilla) → review-
> From testing:
> 1) Drag and drop seems to get confused about where it is dropping to when you
> have enough tabs from scrolling to start happening.
What do you mean exactly by "get confused"?

> 2) Drag and drop does not scroll the tabs when you try and drag past the end of
> the visible tabs.
Will look into this.

> r-
Yes, but what about the actual code?
Blocks: taboverflow
No longer blocks: 471388
No longer depends on: taboverflow
Keywords: helpwanted
(In reply to comment #28)
> > From testing:
> > 1) Drag and drop seems to get confused about where it is dropping to when you
> > have enough tabs from scrolling to start happening.
> What do you mean exactly by "get confused"?
Say you have Tabs A through to Q and you try and drop tab P between tabs K and L, it appears between tabs G and H. Best way to see the confusion is to open enough tabs to get the scrolling activated and then try to drag and drop.
> 
> > 2) Drag and drop does not scroll the tabs when you try and drag past the end of
> > the visible tabs.
> Will look into this.
> 
> > r-
> Yes, but what about the actual code?
I've has a brief look at the code but until the issues found in testing are fixed I'd not want to do a full review of it.
Comment on attachment 491436 [details] [diff] [review]
Patch v1.0a Tabs scrolling complete with AllTabs menubutton

Nothing serious, mostly stylistic nitpicking. ;-)

>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
>+          <![CDATA[
>+            return Array.filter(document.getBindingParent(this).childNodes,
>+                                this._canScrollToElement, this);

Either all parameters on one line or one line for each.

>+      <method name="_canScrollToElement">
>+        <parameter name="tab"/>
>+        <body>
>+          <![CDATA[
>+            return !tab.pinned && !tab.hidden;

Arguments should have the "a" prefix, i.e. aTab here.

>+      <handler event="overflow">
>+        <![CDATA[
>+           if (event.detail == 0)
>+             return; // Ignore vertical events
>+
>+           var tabs = document.getBindingParent(this);
>+           tabs.setAttribute("overflow", "true");

Probably should set a boolean here for clarity.

>+    <implementation implements="nsITimerCallback, nsIDOMEventListener">
>+      <constructor>
>+        <![CDATA[
>+          this.mTabMinWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMinWidth");
>+          this.mTabMaxWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMaxWidth");

No space before the (, please.

>+      <method name="_fillTrailingGap">
>+        <body>
>+          <![CDATA[
>+          try {
>+            /* if we're at the right side (and not the logical end,
>+             * which is why this works for both LTR and RTL)
>+             * of the tabstrip, we need to ensure that we stay
>+             * completely scrolled to the right side
>+             */
>+            var tabStrip = this.mTabstrip;
>+            if (tabStrip.scrollPosition + tabStrip.scrollClientSize >
>+                tabStrip.scrollSize)

Not wrapping here would probably be more readable.

>+      <method name="_notifyBackgroundTab">
...
>+              /* Can we make both the new tab and the selected tab completely
>+                 visible? */
>+              if (!selected ||
>+                  Math.max(tab.right - selected.left, selected.right - tab.left) <=
>+                    scrollRect.width) {

Same here for the last line, maybe.

> +            if (!this._animateElement.hasAttribute("notifybgtab")) {
> +              this._animateElement.setAttribute("notifybgtab", "true");
> +              setTimeout(function (ele) {

No space before the (, please.

>+      <method name="_handleNewTab">
>+        <parameter name="tab"/>

Parameter should be named aTab.

>+/* XXXRatty: Something goes here but I don't have a Mac.

I would have called out for Stefan anyway. ;-)

f=me with these fixed/reasoned.
Attachment #491436 - Flags: feedback?(mnyromyr) → feedback+
(In reply to comment #30)

> 
> >+/* XXXRatty: Something goes here but I don't have a Mac.
> 
> I would have called out for Stefan anyway. ;-)

Yeah, this differs from the previous patch I looked at (more things added, possibly from pinstripe?), so I don't know how it looks/what it does
(In reply to comment #30)
> Comment on attachment 491436 [details] [diff] [review]
> Patch v1.0a Tabs scrolling complete with AllTabs menubutton
> >+    <implementation implements="nsITimerCallback, nsIDOMEventListener">
> >+      <constructor>
> >+        <![CDATA[
> >+          this.mTabMinWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMinWidth");
> >+          this.mTabMaxWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMaxWidth");
> 
> No space before the (, please.


And while you're at it, one space before the = is enough ;-)
(In reply to comment #32)
> > >+          this.mTabMinWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMinWidth");
> > >+          this.mTabMaxWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMaxWidth");
> > 
> > No space before the (, please.
> 
> And while you're at it, one space before the = is enough ;-)

Well, those align nicely with the = in the lines below the snippet...
> Ian Neal      2010-11-22 14:51:09 PST

> (In reply to comment #28)
>>> From testing:
>>> 1) Drag and drop seems to get confused about where it is dropping to when you
>>> have enough tabs from scrolling to start happening.
>> What do you mean exactly by "get confused"?
> Say you have Tabs A through to Q and you try and drop tab P between tabs K and
> L, it appears between tabs G and H. Best way to see the confusion is to open
> enough tabs to get the scrolling activated and then try to drag and drop.
Fixed.

>>> 2) Drag and drop does not scroll the tabs when you try and drag past the end of
>>> the visible tabs.
>> Will look into this.
Fixed. Dragging over the scroll buttons will cause the tabs to scroll.


> Karsten Düsterloh      2010-11-22 14:51:33 PST

>>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
>>+          <![CDATA[
>>+            return Array.filter(document.getBindingParent(this).childNodes,
>>+                                this._canScrollToElement, this);

> Either all parameters on one line or one line for each.
Fixed.

>>+      <method name="_canScrollToElement">
>>+        <parameter name="tab"/>
>>+        <body>
>>+          <![CDATA[
>>+            return !tab.pinned && !tab.hidden;

> Arguments should have the "a" prefix, i.e. aTab here.
Fixed.

>>+      <handler event="overflow">
>>+        <![CDATA[
>>+           if (event.detail == 0)
>>+             return; // Ignore vertical events
>>+
>>+           var tabs = document.getBindingParent(this);
>>+           tabs.setAttribute("overflow", "true");

> Probably should set a boolean here for clarity.
Fixed.

>>+    <implementation implements="nsITimerCallback, nsIDOMEventListener">
>>+      <constructor>
>>+        <![CDATA[
>>+          this.mTabMinWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMinWidth");
>>+          this.mTabMaxWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMaxWidth");

> No space before the (, please.
Fixed.

>>+      <method name="_fillTrailingGap">
>>+        <body>
>>+          <![CDATA[
>>+          try {
>>+            /* if we're at the right side (and not the logical end,
>>+             * which is why this works for both LTR and RTL)
>>+             * of the tabstrip, we need to ensure that we stay
>>+             * completely scrolled to the right side
>>+             */
>>+            var tabStrip = this.mTabstrip;
>>+            if (tabStrip.scrollPosition + tabStrip.scrollClientSize >
>>+                tabStrip.scrollSize)

> Not wrapping here would probably be more readable.
Fixed.

>>+      <method name="_notifyBackgroundTab">
> ...
>>+              /* Can we make both the new tab and the selected tab completely
>>+                 visible? */
>>+              if (!selected ||
>>+                  Math.max(tab.right - selected.left, selected.right - tab.left) <=
>>+                    scrollRect.width) {

> Same here for the last line, maybe.
Fixed.

>> +            if (!this._animateElement.hasAttribute("notifybgtab")) {
>> +              this._animateElement.setAttribute("notifybgtab", "true");
>> +              setTimeout(function (ele) {

> No space before the (, please.
Fixed.

>>+      <method name="_handleNewTab">
>>+        <parameter name="tab"/>

> Parameter should be named aTab.
Fixed.

>>+/* XXXRatty: Something goes here but I don't have a Mac.

> I would have called out for Stefan anyway. ;-)
Copied some styles from Pinstripe. I have no idea how appropriate these are.

> Robert Kaiser 2010-11-23 04:44:21 PST

> (In reply to comment #30)
>> Comment on attachment 491436 [details] [diff] [review] [details]
>> Patch v1.0a Tabs scrolling complete with AllTabs menubutton
>>>+    <implementation implements="nsITimerCallback, nsIDOMEventListener">
>>>+      <constructor>
>>>+        <![CDATA[
>>>+          this.mTabMinWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMinWidth");
>>>+          this.mTabMaxWidth  = this.mPrefs.getIntPref ("browser.tabs.tabMaxWidth");
>> 
>> No space before the (, please.
Fixed.

> And while you're at it, one space before the = is enough ;-)
Fixed.
Attachment #491436 - Attachment is obsolete: true
Attachment #493521 - Flags: review?(iann_bugzilla)
Comment on attachment 493521 [details] [diff] [review]
Patch v1.1 Fixed Drag'n'Drop and tab scrolling.

Stefan. Please review the Mac UI. Thanks.
Attachment #493521 - Flags: ui-review?(stefanh)
Comment on attachment 493521 [details] [diff] [review]
Patch v1.1 Fixed Drag'n'Drop and tab scrolling.

+.scrollbutton-down {
+  border-left: 2px solid;
+  -moz-border-left-colors: rgba(0 ,0 ,0 , 0.19) transparent;
+  list-style-image: url("chrome://messenger/skin/icons/tab-arrow-right.png");
+  -moz-image-region: rect(0px, 7px, 11px, 0px);
+  -moz-transition: 1s background-color ease-out;
+}

+.scrollbutton-down[notifybgtab] {
+  background-color: Highlight;
+  -moz-transition: none;}
+
 
This transition doesn't work as expected since there's background-color set on hover/hover:active. What happens here is that you get the transition everytime you hover or press the button (the desired effect is only visible when you set 'browser.tabs.insertRelatedAfterCurrent' and when new tabs are overflowed). What you can do here is to copy the gnomestripe rules, e.g:

'-moz-transition: 1s box-shadow ease-out;' on the .scrollbutton-down and then do
'box-shadow: inset 0 0 5px 5px Highlight;
 -moz-transation: none;' on the .scrollbutton-down[notifybgtab]

It's not 100% perfect, but it will do until we find better styling.

+.tabs-alltabs-box {
+  margin: 0;
+}
+
+.tabs-alltabs-button {
+  list-style-image: url("chrome://navigator/skin/icons/alltabs-box-bkgnd-icon.png");
+  -moz-image-region: rect(0, 22px, 20px, 0);
+  -moz-border-start: 2px solid;
+  -moz-border-end: none;
+  -moz-border-left-colors: rgba(0,0,0,0.25) rgba(255,255,255,0.15);
+  -moz-border-right-colors: rgba(0,0,0,0.25) rgba(255,255,255,0.15);
+  margin: 0;
+  padding: 0;
+}
+
+.tabs-alltabs-button:hover {
+  -moz-image-region: rect(0, 44px, 20px, 22px);
+  background-color: rgba(0,0,0,0.10);
+}
+
+.tabs-alltabs-button[open="true"],
+  -moz-image-region: rect(0, 66px, 20px, 44px);
+}
+
+.tabs-alltabs-button:hover:active {
+  -moz-image-region: rect(0, 66px, 20px, 44px);
+  background-color: rgba(0,0,0,0.20);
+}
+
+.tabs-alltabs-button > .toolbarbutton-icon {
+  padding: 0;
+  -moz-margin-end: 2px;
+}
+
+.tabs-alltabs-button > .toolbarbutton-menu-dropmarker,
+.tabs-alltabs-button > .toolbarbutton-text {
+  display: none;
+}

We don't use the alltabs-box-bkgnd-icon.png. in tabmail, so I think we can live without it. It doesn't fit to 100% either. So, you can remove all those style rules above (and of course not package the .png file)

+.alltabs-item[selected="true"] {
+  font-weight: bold;
+}

Don't want this either.


There's an issue on Mac when you press the scrollbuttons: the "Close Tab" context menu will appear after a sec or so. This is because we have 'ui.click_hold_context_menus' set to true on mac. This also happened in tabmail, but you fixed it in bug 518203. We need to do something similar here as well, otherwise mac users will always get a context menu when the click and hold the scrollbutton.
Attachment #493521 - Flags: ui-review?(stefanh) → ui-review-
Attachment #493521 - Attachment description: Patch v1.2 Fixed Drag'n'Drop and tab scrolling. → Patch v1.1 Fixed Drag'n'Drop and tab scrolling.
Attached patch Patch v1.2 Fix Mac UI issues. (obsolete) — — Splinter Review 
> Stefan      2010-11-28 11:16:30 PST
> 
> This transition doesn't work as expected since there's background-color set on
> hover/hover:active. What happens here is that you get the transition everytime
> you hover or press the button (the desired effect is only visible when you set
> 'browser.tabs.insertRelatedAfterCurrent' and when new tabs are overflowed).
> What you can do here is to copy the gnomestripe rules, e.g:
> 
> '-moz-transition: 1s box-shadow ease-out;' on the .scrollbutton-down and then
> do
> 'box-shadow: inset 0 0 5px 5px Highlight;
>  -moz-transation: none;' on the .scrollbutton-down[notifybgtab]
> 
> It's not 100% perfect, but it will do until we find better styling.
Fixed.

> +.tabs-alltabs-box {
> 
> We don't use the alltabs-box-bkgnd-icon.png. in tabmail, so I think we can live
> without it. It doesn't fit to 100% either. So, you can remove all those style
> rules above (and of course not package the .png file)
Fixed.

> +.alltabs-item[selected="true"] {
> +  font-weight: bold;
> +}
> 
> Don't want this either.
Fixed.

> There's an issue on Mac when you press the scrollbuttons: the "Close Tab"
> context menu will appear after a sec or so. This is because we have
> 'ui.click_hold_context_menus' set to true on mac. This also happened in
> tabmail, but you fixed it in bug 518203. We need to do something similar here
> as well, otherwise mac users will always get a context menu when the click and
> hold the scrollbutton.
Fixed (I think).
Attachment #493521 - Attachment is obsolete: true
Attachment #497068 - Flags: ui-review?(stefanh)
Attachment #497068 - Flags: review?(neil)
Attachment #493521 - Flags: review?(iann_bugzilla)
Comment on attachment 497068 [details] [diff] [review]
Patch v1.2 Fix Mac UI issues.

># HG changeset patch
># User Philip Chee <philip.chee@gmail.com>
># Date 1292085680 -28800
># Node ID 84733be412a22ec028cddf58f2af6fab4bdd2ee5
># Parent  26c975e35f4c1cd90b610030ccf2902eecede2d0
>Bug 484968 Make SeaMonkey tab bar scrollable to cope with tab overflow.
>
>diff --git a/suite/browser/navigator.css b/suite/browser/navigator.css
>--- a/suite/browser/navigator.css
>+++ b/suite/browser/navigator.css
>@@ -9,16 +9,24 @@
> tabbrowser {
>   -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser");
> }
> 
> .tabbrowser-tabs {
>   -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser-tabs");
> }
> 
>+.tabbrowser-arrowscrollbox {
>+  -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser-arrowscrollbox");
>+}
>+
>+.tabs-alltabs-popup {
>+  -moz-binding: url("chrome://navigator/content/tabbrowser.xml#tabbrowser-alltabs-popup");
>+}
>+
> .tabs-closebutton-box > .tabs-closebutton {
>   -moz-binding: url("chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton");
> }
> 
> /* ::::: urlbar autocomplete ::::: */
> 
> #urlbar {
>   -moz-binding: url("chrome://navigator/content/urlbarBindings.xml#urlbar");
>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
>--- a/suite/browser/tabbrowser.xml
>+++ b/suite/browser/tabbrowser.xml
>@@ -67,17 +67,17 @@
>                   ondragover="nsDragAndDrop.dragOver(event, this.parentNode.parentNode); event.stopPropagation();"
>                   ondragdrop="nsDragAndDrop.drop(event, this.parentNode.parentNode); event.stopPropagation();"
>                   ondragexit="nsDragAndDrop.dragExit(event, this.parentNode.parentNode); event.stopPropagation();">
>           <xul:tooltip onpopupshowing="event.stopPropagation(); return this.parentNode.parentNode.parentNode.doPreview(this);"
>                        onpopuphiding="this.parentNode.parentNode.parentNode.resetPreview(this);" orient="vertical">
>             <xul:label class="tooltip-label" crop="right"/>
>             <xul:label class="tooltip-label" hidden="true"><html:canvas class="tab-tooltip-canvas"/></xul:label>
>           </xul:tooltip>
>-          <xul:menupopup anonid="tabContextMenu" onpopupshowing="this.parentNode.parentNode.parentNode.updatePopupMenu(this);">
>+          <xul:menupopup anonid="tabContextMenu" onpopupshowing="return document.getBindingParent(this).updatePopupMenu(this);">
>             <xul:menuitem label="&closeTab.label;" accesskey="&closeTab.accesskey;"
>                           tbattr="tabbrowser-multiple"
>                           oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
>                                      tabbrowser.removeTab(tabbrowser.mContextTab);"/>
>             <xul:menuitem label="&closeOtherTabs.label;" accesskey="&closeOtherTabs.accesskey;"
>                           tbattr="tabbrowser-multiple"
>                           oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
>                                      tabbrowser.removeAllTabsBut(tabbrowser.mContextTab);"/>
>@@ -100,27 +100,28 @@
>                           oncommand="var tabbrowser = this.parentNode.parentNode.parentNode.parentNode;
>                                      tabbrowser.reloadAllTabs(tabbrowser.mContextTab);"/>
>           </xul:menupopup>
> 
>           <xul:tabs class="tabbrowser-tabs" closebutton="true" flex="1"
>                     anonid="tabcontainer"
>                     tooltiptextnew="&newTabButton.tooltip;"
>                     tooltiptextclose="&closeTabButton.tooltip;"
>+                    tooltiptextalltabs="&listAllTabs.label;"
>                     setfocus="false"
>                     onclick="this.parentNode.parentNode.parentNode.onTabClick(event);"
>                     xbl:inherits="onnewtab,onnewtabclick"
>                     onclosetab="var node = this.parentNode;
>                                 while (node.localName != 'tabbrowser')
>                                   node = node.parentNode;
>                                 node.removeCurrentTab();">
>             <xul:tab selected="true" validate="never"
>                      onerror="this.parentNode.parentNode.parentNode.parentNode.addToMissedIconCache(this.getAttribute('image'));
>                               this.removeAttribute('image');"
>-                     maxwidth="250" width="0" minwidth="30" flex="100"
>+                     width="0" flex="100"
>                      class="tabbrowser-tab" label="&untitledTab;" crop="end"/>
>           </xul:tabs>
>         </xul:hbox>
>         <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer">
>           <xul:notificationbox class="browser-notificationbox" xbl:inherits="popupnotification">
>             <xul:browser flex="1" type="content-primary" xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autocompletepopup"/>
>           </xul:notificationbox>
>         </xul:tabpanels>
>@@ -923,16 +924,19 @@
>         </body>
>       </method>
> 
>       <method name="updatePopupMenu">
>         <parameter name="aPopupMenu"/>
>         <body>
>           <![CDATA[
>             this.mContextTab = document.popupNode;
>+            if (this.mPrefs.getBoolPref("ui.click_hold_context_menus") &&
>+                this.mContextTab.localName != "tab")
>+              return false;
>             var disabled = this.tabs.length == 1;
>             var menuItems = aPopupMenu.getElementsByAttribute("tbattr", "tabbrowser-multiple");
>             for (var i = 0; i < menuItems.length; i++)
>               menuItems[i].setAttribute("disabled", disabled);
> 
>             var undoItem = document.getAnonymousElementByAttribute(this, "tbattr", "tabbrowser-undoclosetab");
>             undoItem.setAttribute("disabled", this.mSessionStore.getClosedTabCount(window) == 0);
>             undoItem.hidden = this.mPrefs.getIntPref("browser.tabs.max_tabs_undo") <= 0 && this.mPrefs.getIntPref("browser.sessionstore.max_tabs_undo") <= 0;
>@@ -1464,16 +1468,17 @@
>             // than the current tab.
>             if ((aRelatedToCurrent || aReferrerURI) &&
>                 this.mPrefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {
>               var lastRelatedIndex = this.mLastRelatedIndex ||
>                                      this.tabContainer.selectedIndex;
>               this.moveTabTo(t, ++lastRelatedIndex);
>               this.mLastRelatedIndex = lastRelatedIndex;
>             }
>+            this.tabContainer._handleNewTab(t);
> 
>             return t;
>           ]]>
>         </body>
>       </method>
> 
>       <method name="removeAllTabsBut">
>         <parameter name="aTab"/>
>@@ -1642,16 +1647,17 @@
>                             this.mStringBundle.getString("tabs.untitled");
> 
>             var l = this.tabs.length;
>             switch (l) {
>               case 1:
>                 // add a new blank tab to replace the one we're about to close
>                 // (this ensures that the remaining tab is as good as new)
>                 this.addTab("about:blank");
>+                this.tabContainer._fillTrailingGap();
>                 l++;
>                 // fall through
>               case 2:
>                 if (this.mPrefs.getBoolPref("browser.tabs.autoHide"))
>                   this.mStrip.collapsed = true;
>             }
> 
>             var index = this.getTabIndex(aTab);
>@@ -1993,73 +1999,107 @@
>       </method>
> 
>       <method name="canDrop">
>         <parameter name="aEvent"/>
>         <parameter name="aDragSession"/>
>         <body>
>           <![CDATA[
>             if (aDragSession.sourceNode &&
>-                aDragSession.sourceNode.parentNode == this.tabContainer) {
>-              var newIndex = this.getDropIndex(aEvent);
>-              var tabIndex = this.getTabIndex(aDragSession.sourceNode);
>-              if (newIndex == tabIndex || newIndex == tabIndex + 1)
>-                return false;
>-            }
>+                aDragSession.sourceNode.parentNode == this.tabContainer &&
>+                (aEvent.screenX >= aDragSession.sourceNode.boxObject.screenX &&
>+                 aEvent.screenX <= (aDragSession.sourceNode.boxObject.screenX +
>+                                    aDragSession.sourceNode.boxObject.width)))
>+              return false;
>             return true;
>           ]]>
>         </body>
>       </method>
> 
>       <method name="onDragOver">
>         <parameter name="aEvent"/>
>         <parameter name="aFlavour"/>
>         <parameter name="aDragSession"/>
>         <body>
>           <![CDATA[
>             var ib = document.getAnonymousElementByAttribute(this, "class", "tab-drop-indicator-bar");
> 
>-            if (!aDragSession.canDrop) {
>+            /* autoscroll the tab strip if we drag over the scroll
>+             * buttons, even if we aren't dragging a tab
>+             */
>+            var pixelsToScroll = 0;
>+            var tabStrip = this.mTabContainer.mTabstrip;
>+            if (this.mTabContainer.getAttribute("overflow") == "true") {
>+              var targetAnonid = aEvent.originalTarget.getAttribute("anonid");
>+              switch (targetAnonid) {
>+                case "scrollbutton-up":
>+                  pixelsToScroll = tabStrip.scrollIncrement * -1;
>+                  break;
>+                case "scrollbutton-down":
>+                case "alltabs-button":
>+                  pixelsToScroll = tabStrip.scrollIncrement;
>+                  break;
>+              }
>+              if (pixelsToScroll)
>+                tabStrip.scrollByPixels((ltr ? -1 : 1) * pixelsToScroll);
>+            }
>+            else if (!aDragSession.canDrop) {
>               ib.collapsed = true;
>               return;
>             }
> 
>             var ind = document.getAnonymousElementByAttribute(this, "class", "tab-drop-indicator");
> 
>             var newIndexOn = aDragSession.sourceNode &&
>                              aDragSession.sourceNode.parentNode == this.tabContainer ?
>                              -1 : this.getDropOnIndex(aEvent);
> 
>             var ltr = window.getComputedStyle(this, null).direction == "ltr";
>-            var arrowX, tabBoxObject;
>-            if (newIndexOn != -1) {
>-              tabBoxObject = this.tabs[newIndexOn].boxObject;
>-              arrowX = tabBoxObject.x + tabBoxObject.width / 2;
>+            var scrollRect = tabStrip.scrollClientRect;
>+            var rect = this.getBoundingClientRect();
>+            var minMargin = scrollRect.left - rect.left;
>+            var maxMargin = Math.min(minMargin + scrollRect.width,
>+                                     ib.getBoundingClientRect().right);
>+                                     //ib.getBoundingClientRect().right - ind.clientWidth);
>+            if (!ltr)
>+              [minMargin, maxMargin] = [this.clientWidth - maxMargin,
>+                                        this.clientWidth - minMargin];
>+
>+            var newMargin;
>+             if (pixelsToScroll) {
>+              // if we are scrolling, put the drop indicator at the edge
>+              // so that it doesn't jump while scrolling
>+              newMargin = (pixelsToScroll > 0) ? maxMargin : minMargin;
>+            }
>+            else if (newIndexOn != -1) {
>+              let tabRect = this.tabs[newIndexOn].getBoundingClientRect();
>+              newMargin = ltr? tabRect.left - rect.left:
>+                               rect.right - tabRect.right;
>+              newMargin += (tabRect.width) / 2;
>             }
>             else {
>               var newIndexBetween = this.getDropIndex(aEvent);
>               if (newIndexBetween == this.tabs.length) {
>-                tabBoxObject = this.tabs[this.tabs.length - 1].boxObject;
>-                arrowX = tabBoxObject.x;
>-                if (ltr) // for LTR "after" is on the right-hand side of the tab
>-                  arrowX += tabBoxObject.width;
>+                let tabRect = this.tabs[newIndexBetween - 1].getBoundingClientRect();
>+                newMargin = ltr? tabRect.right - rect.left:
>+                                 rect.right - tabRect.left;
>               }
>               else {
>-                tabBoxObject = this.tabs[newIndexBetween].boxObject;
>-                arrowX = tabBoxObject.x;
>-                if (!ltr) // for RTL "before" is on the right-hand side of the tab
>-                  arrowX += tabBoxObject.width;
>+                let tabRect = this.tabs[newIndexBetween].getBoundingClientRect();
>+                newMargin = ltr? tabRect.left - rect.left:
>+                                 rect.right - tabRect.right;
>               }
>+              // ensure we never place the drop indicator beyond our limits
>+              if (newMargin < minMargin)
>+                newMargin = minMargin;
>+              else if (newMargin > maxMargin)
>+                newMargin = maxMargin;
>             }
> 
>-            if (ltr)
>-              ind.style.marginLeft = (arrowX - this.boxObject.x) + "px";
>-            else
>-              ind.style.marginRight = (this.boxObject.x + this.boxObject.width - arrowX) + "px";
>-
>+            ind.style.MozMarginStart = newMargin + 'px';
>             ib.collapsed = false;
>           ]]>
>         </body>
>       </method>
> 
>       <method name="onDrop">
>         <parameter name="aEvent"/>
>         <parameter name="aXferData"/>
>@@ -2085,16 +2125,18 @@
>               if (!url || !url.length || url.indexOf(" ", 0) != -1 ||
>                   /^\s*(javascript|data):/.test(url))
>                 return;
> 
>               // Perform a security check before loading the URI
>               nsDragAndDrop.dragDropSecurityCheck(aEvent, aDragSession, url);
> 
>               var bgLoad = this.mPrefs.getBoolPref("browser.tabs.loadInBackground");
>+              if (aEvent.shiftKey)
>+                bgLoad = !bgLoad;
> 
>               var tab = null;
>               tabIndex = this.getDropOnIndex(aEvent);
>               if (tabIndex != -1) {
>                 // Load in an existing tab
>                 tab = this.tabs[tabIndex];
>                 tab.linkedBrowser.loadURI(getShortcutOrURI(url));
>                 if (this.mCurrentTab != tab && !bgLoad)
>@@ -2168,55 +2210,56 @@
> 
>             this.mCurrentTab._selected = false;
> 
>             if (aDestIndex >= aSrcIndex)
>               ++aDestIndex;
>             var tab = this.tabContainer.insertBefore(this.tabs[aSrcIndex], this.tabs.item(aDestIndex));
> 
>             this.mCurrentTab._selected = true;
>+            this.tabContainer.mTabstrip.ensureElementIsVisible(this.mCurrentTab, false);
> 
>             var evt = document.createEvent("UIEvents");
>             evt.initUIEvent("TabMove", true, false, window, aSrcIndex);
>             tab.dispatchEvent(evt);
> 
>             return tab;
>           ]]>
>         </body>
>       </method>
> 
>       <method name="getDropIndex">
>         <parameter name="aEvent"/>
>         <body>
>-          <![CDATA[     
>+          <![CDATA[
>             for (var i = 0; i < this.tabs.length; ++i) {
>-              var coord = this.tabs[i].boxObject.x +
>+              var coord = this.tabs[i].boxObject.screenX +
>                           this.tabs[i].boxObject.width / 2;
>               if (window.getComputedStyle(this, null).direction == "ltr") {
>-                if (aEvent.clientX < coord) 
>+                if (aEvent.screenX < coord)
>                   return i;
>               } else {
>-                if (aEvent.clientX > coord) 
>+                if (aEvent.screenX > coord)
>                   return i;
>               }
>             }
> 
>             return this.tabs.length;
>           ]]>
>         </body>
>       </method>
> 
>       <method name="getDropOnIndex">
>         <parameter name="aEvent"/>
>         <body>
>-          <![CDATA[     
>+          <![CDATA[
>             for (var i = 0; i < this.tabs.length; ++i) {
>               var tabBoxObject = this.tabs[i].boxObject;
>-              if (aEvent.clientX > tabBoxObject.x + tabBoxObject.width * .25 &&
>-                  aEvent.clientX < tabBoxObject.x + tabBoxObject.width * .75)
>+              if (aEvent.clientX > tabBoxObject.screenX + tabBoxObject.width * .25 &&
>+                  aEvent.clientX < tabBoxObject.screenX + tabBoxObject.width * .75)
>                 return i;
>             }
> 
>             return -1;
>           ]]>
>         </body>
>       </method>
> 
>@@ -2652,20 +2695,20 @@
>               !this.mPrefs.getBoolPref("browser.tabs.forceHide") &&
>               window.toolbar.visible)
>             this.mStrip.collapsed = false;
> 
>           var t = document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "tab");
>           t.setAttribute("label", this.mStringBundle.getString("tabs.untitled"));
>           t.setAttribute("crop", "end");
>           t.className = "tabbrowser-tab";
>-          t.setAttribute("maxwidth", 250);
>-          t.setAttribute("minwidth", 30);
>-          t.setAttribute("width", 0);
>-          t.setAttribute("flex", 100);
>+          t.style.maxWidth = this.tabContainer.mTabMaxWidth + "px";
>+          t.style.minWidth = this.tabContainer.mTabMinWidth + "px";
>+          t.width = 0;
>+          t.setAttribute("flex", "100");
>           t.setAttribute("validate", "never");
>           t.setAttribute("onerror", "this.parentNode.parentNode.parentNode.parentNode.addToMissedIconCache(this.getAttribute('image')); this.removeAttribute('image');");
>           this.referenceTab = t;
> 
>           var os = Components.classes["@mozilla.org/observer-service;1"]
>                              .getService(Components.interfaces.nsIObserverService);
>           os.addObserver(this, "browser:purge-session-history", false);
>         ]]>
>@@ -2746,43 +2789,432 @@
>       <handler event="keypress" keycode="VK_RIGHT" modifiers="accel" action="if (event.target == this) { this.moveTabRight(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_UP" modifiers="accel" action="if (event.target == this) { this.moveTabBackward(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_DOWN" modifiers="accel" action="if (event.target == this) { this.moveTabForward(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_HOME" modifiers="accel" action="if (event.target == this) { this.moveTabToStart(); event.preventDefault(); }"/>
>       <handler event="keypress" keycode="VK_END" modifiers="accel" action="if (event.target == this) { this.moveTabToEnd(); event.preventDefault(); }"/>
>     </handlers>
>   </binding>
> 
>+  <binding id="tabbrowser-arrowscrollbox"
>+           extends="chrome://global/content/bindings/scrollbox.xml#arrowscrollbox-clicktoscroll">
>+    <resources>
>+      <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>+    </resources>
>+
>+    <implementation>
>+      <!-- Override scrollbox.xml method, since our scrollbox's children are
>+           inherited from the binding parent -->
>+      <method name="_getScrollableElements">
>+        <body>
>+          <![CDATA[
>+            return Array.filter(document.getBindingParent(this).childNodes,
>+                                this._canScrollToElement,
>+                                this);
>+          ]]>
>+        </body>
>+      </method>
>+      <method name="_canScrollToElement">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+            return !aTab.pinned && !aTab.hidden;
>+          ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="underflow">
>+        <![CDATA[
>+           if (event.detail == 0)
>+             return; // Ignore vertical events
>+
>+           var tabs = document.getBindingParent(this);
>+           tabs.removeAttribute("overflow");
>+        ]]>
>+      </handler>
>+      <handler event="overflow">
>+        <![CDATA[
>+           if (event.detail == 0)
>+             return; // Ignore vertical events
>+
>+           var tabs = document.getBindingParent(this);
>+           tabs.setAttribute("overflow", true);
>+           this.ensureElementIsVisible(tabs.selectedItem, false);
>+        ]]>
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>   <binding id="tabbrowser-tabs"
>            extends="chrome://global/content/bindings/tabbox.xml#tabs">
>     <resources>
>       <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>     </resources>
> 
>     <content>
>       <xul:stack flex="1" class="tabs-stack">
>         <xul:vbox>
>           <xul:spacer flex="1"/>
>           <xul:hbox class="tabs-bottom" align="center"/>
>         </xul:vbox>
>         <xul:vbox>
>           <xul:hbox>
>             <xul:stack>
>               <xul:spacer class="tabs-left"/>
>-              <xul:toolbarbutton class="tabs-newbutton" xbl:inherits="oncommand=onnewtab,onclick=onnewtabclick,tooltiptext=tooltiptextnew"/>
>+              <xul:toolbarbutton class="tabs-newbutton"
>+                                 anonid="tabstrip-newbutton"
>+                                 xbl:inherits="oncommand=onnewtab,onclick=onnewtabclick,tooltiptext=tooltiptextnew"/>
>             </xul:stack>
>-            <xul:hbox flex="1" style="min-width: 1px; overflow: -moz-hidden-unscrollable;">
>-              <children/>
>+            <xul:arrowscrollbox anonid="arrowscrollbox"
>+                                class="tabbrowser-arrowscrollbox"
>+                                flex="1"
>+                                xbl:inherits="smoothscroll"
>+                                orient="horizontal"
>+                                style="min-width: 1px;">
>+              <children includes="tab"/>
>               <xul:spacer class="tabs-right" flex="1"/>
>-            </xul:hbox>
>-            <xul:stack>
>+            </xul:arrowscrollbox>
>+            <children/>
>+            <xul:stack align="center" pack="end">
>               <xul:spacer class="tabs-right"/>
>-              <xul:hbox class="tabs-closebutton-box" align="center" pack="end">
>-                <xul:toolbarbutton class="tabs-closebutton close-button" xbl:inherits="disabled=disableclose,oncommand=onclosetab,tooltiptext=tooltiptextclose"/>
>+              <xul:hbox class="tabs-closebutton-box" align="stretch" pack="end">
>+                <xul:toolbarbutton class="tabs-alltabs-button"
>+                                   anonid="alltabs-button"
>+                                   type="menu"
>+                                   xbl:inherits="tooltiptext=tooltiptextalltabs">
>+                  <xul:menupopup class="tabs-alltabs-popup"
>+                                 anonid="alltabs-popup"
>+                                 position="after_end"/>
>+                </xul:toolbarbutton>
>+                <xul:toolbarbutton class="tabs-closebutton close-button"
>+                                   anonid="tabstrip-closebutton"
>+                                   xbl:inherits="disabled=disableclose,oncommand=onclosetab,tooltiptext=tooltiptextclose"/>
>               </xul:hbox>
>             </xul:stack>
>           </xul:hbox>
>           <xul:spacer class="tabs-bottom-spacer"/>
>         </xul:vbox>
>       </xul:stack>
>     </content>
>+
>+    <implementation implements="nsIDOMEventListener">
>+      <constructor>
>+        <![CDATA[
>+          this.mTabMinWidth = this.mPrefs.getIntPref("browser.tabs.tabMinWidth");
>+          this.mTabMaxWidth = this.mPrefs.getIntPref("browser.tabs.tabMaxWidth");
>+          var tab = this.firstChild;
>+          tab.style.minWidth = this.mTabMinWidth + "px";
>+          tab.style.maxWidth = this.mTabMaxWidth + "px";
>+          window.addEventListener("resize", this, false);
>+        ]]>
>+      </constructor>
>+
>+      <field name="mPrefs" readonly="true">
>+        Components.classes["@mozilla.org/preferences-service;1"]
>+                  .getService(Components.interfaces.nsIPrefBranch2);
>+      </field>
>+
>+      <field name="mTabstripWidth">0</field>
>+
>+      <field name="mTabstrip">
>+        document.getAnonymousElementByAttribute(this, "anonid", "arrowscrollbox");
>+      </field>
>+
>+      <field name="mTabMinWidth">100</field>
>+      <field name="mTabMaxWidth">250</field>
>+
>+      <method name="_handleTabSelect">
>+        <body>
>+          <![CDATA[
>+            this.mTabstrip.ensureElementIsVisible(this.selectedItem);
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="_fillTrailingGap">
>+        <body>
>+          <![CDATA[
>+          try {
>+            /* if we're at the right side (and not the logical end,
>+             * which is why this works for both LTR and RTL)
>+             * of the tabstrip, we need to ensure that we stay
>+             * completely scrolled to the right side
>+             */
>+            var tabStrip = this.mTabstrip;
>+            if (tabStrip.scrollPosition + tabStrip.scrollClientSize > tabStrip.scrollSize)
>+              tabStrip.scrollByPixels(-1);
>+          } catch (e) {}
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="handleEvent">
>+        <parameter name="aEvent"/>
>+        <body>
>+          <![CDATA[
>+            switch (aEvent.type)
>+            {
>+              case "resize":
>+                if (aEvent.target != window)
>+                  break;
>+                var width = this.mTabstrip.boxObject.width;
>+                if (width != this.mTabstripWidth)
>+                {
>+                  this._fillTrailingGap();
>+                  this._handleTabSelect();
>+                  this.mTabstripWidth = width;
>+                }
>+                break;
>+            }
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <field name="mAllTabsPopup">
>+        document.getAnonymousElementByAttribute(this, "anonid", "alltabs-popup");
>+      </field>
>+
>+      <field name="_animateElement">
>+        this.mTabstrip._scrollButtonDown;
>+      </field>
>+
>+      <method name="_notifyBackgroundTab">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+            var scrollRect = this.mTabstrip.scrollClientRect;
>+            var tab = aTab.getBoundingClientRect();
>+
>+            // Is the new tab already completely visible?
>+            if (scrollRect.left <= tab.left && tab.right <= scrollRect.right)
>+              return;
>+
>+            if (this.mTabstrip.smoothScroll) {
>+              let selected = this.selectedItem.getBoundingClientRect();
>+
>+              /* Can we make both the new tab and the selected tab completely
>+                 visible? */
>+              if (!selected ||
>+                  Math.max(tab.right - selected.left, selected.right - tab.left) <= scrollRect.width) {
>+                this.mTabstrip.ensureElementIsVisible(aTab);
>+                return;
>+              }
>+
>+              this.mTabstrip._smoothScrollByPixels(this.mTabstrip._isRTLScrollbox ?
>+                                                   selected.right - scrollRect.right :
>+                                                   selected.left - scrollRect.left);
>+            }
>+
>+            if (!this._animateElement.hasAttribute("notifybgtab")) {
>+              this._animateElement.setAttribute("notifybgtab", "true");
>+              setTimeout(function(ele) {
>+                ele.removeAttribute("notifybgtab");
>+              }, 150, this._animateElement);
>+            }
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="_handleNewTab">
>+        <parameter name="aTab"/>
>+        <body>
>+          <![CDATA[
>+            if (aTab.parentNode != this)
>+              return;
>+
>+            if (aTab.getAttribute("selected") == "true") {
>+              this._fillTrailingGap();
>+              this._handleTabSelect();
>+            } else {
>+              this._notifyBackgroundTab(aTab);
>+            }
>+
>+            /* XXXmano: this is a temporary workaround for bug 345399
>+             * We need to manually update the scroll buttons disabled state
>+             * if a tab was inserted to the overflow area or removed from it
>+             * without any scrolling and when the tabbar has already
>+             * overflowed.
>+             */
>+            this.mTabstrip._updateScrollButtonsDisabledState();
>+          ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="TabSelect" action="this._handleTabSelect();"/>
>+
>+      <handler event="transitionend">
>+        <![CDATA[
>+          if (event.propertyName == "max-width")
>+            this._handleNewTab(event.target);
>+        ]]>
>+      </handler>
>+    </handlers>
>+  </binding>
>+
>+  <binding id="tabbrowser-alltabs-popup"
>+           extends="chrome://global/content/bindings/popup.xml#popup">
>+    <implementation implements="nsIDOMEventListener">
>+      <method name="_tabOnTabClose">
>+        <parameter name="aEvent"/>
>+        <body>
>+          <![CDATA[
>+            let menuItem = aEvent.target.mCorrespondingMenuitem;
>+            if (menuItem)
>+              this.removeChild(menuItem);
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="handleEvent">
>+        <parameter name="aEvent"/>
>+        <body>
>+          <![CDATA[
>+            if (!aEvent.isTrusted)
>+              return;
>+
>+            switch (aEvent.type)
>+            {
>+              case "TabClose":
>+                this._tabOnTabClose(aEvent);
>+                break;
>+              case "TabOpen":
>+                this._createTabMenuItem(aEvent.originalTarget);
>+                break;
>+              case "scroll":
>+                this._updateTabsVisibilityStatus();
>+                break;
>+            }
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="_updateTabsVisibilityStatus">
>+        <body>
>+          <![CDATA[
>+            let tabContainer = document.getBindingParent(this);
>+            // We don't want menu item decoration unless there is overflow.
>+            if (tabContainer.getAttribute("overflow") != "true")
>+              return;
>+
>+            let tabstripBO = tabContainer.mTabstrip.scrollBoxObject;
>+            for (let i = 0; i < this.childNodes.length; i++)
>+            {
>+              let curTabBO = this.childNodes[i].tab.boxObject;
>+              if (curTabBO.screenX >= tabstripBO.screenX &&
>+                  curTabBO.screenX + curTabBO.width <= tabstripBO.screenX + tabstripBO.width)
>+                this.childNodes[i].setAttribute("tabIsVisible", "true");
>+              else
>+                this.childNodes[i].removeAttribute("tabIsVisible");
>+            }
>+          ]]>
>+        </body>
>+      </method>
>+
>+      <method name="_createTabMenuItem">
>+        <parameter name="aTabNode"/>
>+        <body>
>+          <![CDATA[
>+            let menuItem = document.createElementNS(
>+              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+              "menuitem");
>+            menuItem.setAttribute("class", "menuitem-iconic alltabs-item icon-holder");
>+            menuItem.setAttribute("label", aTabNode.label);
>+            menuItem.setAttribute("crop",  aTabNode.getAttribute("crop"));
>+            menuItem.setAttribute("image", aTabNode.getAttribute("image"));
>+
>+            ["busy", "selected"].forEach(
>+              function(attribute)
>+              {
>+                if (aTabNode.hasAttribute(attribute))
>+                {
>+                  menuItem.setAttribute(attribute, aTabNode.getAttribute(attribute));
>+                }
>+              }
>+            );
>+
>+            // Keep some attributes of the menuitem in sync with its
>+            // corresponding tab (e.g. the tab label)
>+            aTabNode.mCorrespondingMenuitem = menuItem;
>+            document.addBroadcastListenerFor(aTabNode, menuItem, "label");
>+            document.addBroadcastListenerFor(aTabNode, menuItem, "crop");
>+            document.addBroadcastListenerFor(aTabNode, menuItem, "image");
>+            document.addBroadcastListenerFor(aTabNode, menuItem, "busy");
>+            document.addBroadcastListenerFor(aTabNode, menuItem, "selected");
>+            aTabNode.addEventListener("TabClose", this, false);
>+            menuItem.tab = aTabNode;
>+            menuItem.addEventListener("command", this, false);
>+            this.appendChild(menuItem);
>+            return menuItem;
>+          ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+
>+    <handlers>
>+      <handler event="popupshowing">
>+        <![CDATA[
>+          // set up the menu popup
>+          let tabcontainer = document.getBindingParent(this);
>+          let tabs = tabcontainer.childNodes;
>+
>+          // Listen for changes in the tab bar.
>+          let tabbrowser = document.getBindingParent(tabcontainer);
>+          tabbrowser.addEventListener("TabOpen", this, false);
>+          tabcontainer.mTabstrip.addEventListener("scroll", this, false);
>+
>+          for (let i = 0; i < tabs.length; i++)
>+            this._createTabMenuItem(tabs[i]);
>+          this._updateTabsVisibilityStatus();
>+        ]]>
>+      </handler>
>+
>+      <handler event="popuphiding">
>+        <![CDATA[
>+          // clear out the menu popup and remove the listeners
>+          while (this.hasChildNodes())
>+          {
>+            let menuItem = this.lastChild;
>+            document.removeBroadcastListenerFor(menuItem.tab, menuItem, "label");
>+            document.removeBroadcastListenerFor(menuItem.tab, menuItem, "crop");
>+            document.removeBroadcastListenerFor(menuItem.tab, menuItem, "image");
>+            document.removeBroadcastListenerFor(menuItem.tab, menuItem, "busy");
>+            document.removeBroadcastListenerFor(menuItem.tab, menuItem, "selected");
>+            menuItem.removeEventListener("command", this, false);
>+            menuItem.tab.removeEventListener("TabClose", this, false);
>+            menuItem.tab.mCorrespondingMenuitem = null;
>+            this.removeChild(menuItem);
>+          }
>+          let tabcontainer = document.getBindingParent(this);
>+          tabcontainer.mTabstrip.removeEventListener("scroll", this, false);
>+          document.getBindingParent(tabcontainer).removeEventListener("TabOpen", this, false);
>+        ]]>
>+      </handler>
>+
>+      <handler event="command">
>+        <![CDATA[
>+          let tabcontainer = document.getBindingParent(this);
>+          tabcontainer.selectedItem = event.target.tab;
>+        ]]>
>+      </handler>
>+
>+      <handler event="DOMMenuItemActive">
>+      <![CDATA[
>+        var tab = event.target.tab;
>+        if (tab) {
>+          let overLink = tab.linkedBrowser.currentURI.spec;
>+          if (overLink == "about:blank")
>+            overLink = "";
>+          XULBrowserWindow.setOverLink(overLink, null);
>+        }
>+      ]]></handler>
>+
>+      <handler event="DOMMenuItemInactive">
>+      <![CDATA[
>+        XULBrowserWindow.setOverLink("", null);
>+      ]]></handler>
>+
>+    </handlers>
>   </binding>
> </bindings>
>diff --git a/suite/locales/en-US/chrome/browser/tabbrowser.dtd b/suite/locales/en-US/chrome/browser/tabbrowser.dtd
>--- a/suite/locales/en-US/chrome/browser/tabbrowser.dtd
>+++ b/suite/locales/en-US/chrome/browser/tabbrowser.dtd
>@@ -8,10 +8,11 @@
> <!ENTITY  reloadAllTabs.label    "Reload All Tabs">
> <!ENTITY  reloadAllTabs.accesskey         "A">
> <!ENTITY  reloadTab.label        "Reload Tab">
> <!ENTITY  reloadTab.accesskey         "R">
> <!ENTITY  bookmarkGroup.label    "Bookmark This Group of Tabs">
> <!ENTITY  bookmarkGroup.accesskey  "B">
> <!ENTITY  closeTabButton.tooltip   "Close current tab">
> <!ENTITY  newTabButton.tooltip   "Open a new tab">
>+<!ENTITY  listAllTabs.label      "List all tabs">
> <!ENTITY  undoCloseTab.label    "Undo Close Tab">
> <!ENTITY  undoCloseTab.accesskey  "U">
>diff --git a/suite/themes/classic/jar.mn b/suite/themes/classic/jar.mn
>--- a/suite/themes/classic/jar.mn
>+++ b/suite/themes/classic/jar.mn
>@@ -365,29 +365,32 @@ classic.jar:
>   skin/classic/messenger/smime/icons/hdrCryptoOk.gif                    (messenger/smime/icons/hdrCryptoOk.gif)
>   skin/classic/messenger/smime/icons/hdrCryptoNotOk.gif                 (messenger/smime/icons/hdrCryptoNotOk.gif)
>   skin/classic/messenger-newsblog/feed-subscriptions.css                (messenger/newsblog/feed-subscriptions.css)
> #ifdef XP_MACOSX
>   skin/classic/navigator/navigator.css                                  (mac/navigator/navigator.css)
>   skin/classic/navigator/linkToolbar.css                                (mac/navigator/linkToolbar.css)
>   skin/classic/navigator/pageInfo.css                                   (mac/navigator/pageInfo.css)
>   skin/classic/navigator/tabbrowser.css                                 (mac/navigator/tabbrowser.css)
>+  skin/classic/navigator/icons/alltabs-box-bkgnd-icon.png               (mac/navigator/icons/alltabs-box-bkgnd-icon.png)
>   skin/classic/navigator/icons/navigatoricons.png                       (mac/navigator/icons/navigatoricons.png)
>   skin/classic/navigator/icons/navigatoricons-small.png                 (mac/navigator/icons/navigatoricons-small.png)
>   skin/classic/navigator/icons/restore.png                              (mac/navigator/icons/restore.png)
> #else
>   skin/classic/navigator/navigator.css                                  (navigator/navigator.css)
>   skin/classic/navigator/linkToolbar.css                                (navigator/linkToolbar.css)
>   skin/classic/navigator/pageInfo.css                                   (navigator/pageInfo.css)
>   skin/classic/navigator/tabbrowser.css                                 (navigator/tabbrowser.css)
>   skin/classic/navigator/icons/close.gif                                (navigator/icons/close.gif)
>   skin/classic/navigator/icons/minimize.gif                             (navigator/icons/minimize.gif)
>   skin/classic/navigator/icons/navigatoricons.png                       (navigator/icons/navigatoricons.png)
>   skin/classic/navigator/icons/navigatoricons-small.png                 (navigator/icons/navigatoricons-small.png)
>   skin/classic/navigator/icons/restore.gif                              (navigator/icons/restore.gif)
>+  skin/classic/navigator/icons/tab-arrow-left.png                       (navigator/icons/tab-arrow-left.png)
>+  skin/classic/navigator/icons/tab-arrow-right.png                      (navigator/icons/tab-arrow-right.png)
> #endif
>   skin/classic/navigator/btn1/feeds.png                                 (navigator/btn1/feeds.png)
>   skin/classic/navigator/btn1/first.gif                                 (navigator/btn1/first.gif)
>   skin/classic/navigator/btn1/first-disabled.gif                        (navigator/btn1/first-disabled.gif)
>   skin/classic/navigator/btn1/first-hover.gif                           (navigator/btn1/first-hover.gif)
>   skin/classic/navigator/btn1/last.gif                                  (navigator/btn1/last.gif)
>   skin/classic/navigator/btn1/last-disabled.gif                         (navigator/btn1/last-disabled.gif)
>   skin/classic/navigator/btn1/last-hover.gif                            (navigator/btn1/last-hover.gif)
>diff --git a/suite/themes/classic/mac/navigator/tabbrowser.css b/suite/themes/classic/mac/navigator/tabbrowser.css
>--- a/suite/themes/classic/mac/navigator/tabbrowser.css
>+++ b/suite/themes/classic/mac/navigator/tabbrowser.css
>@@ -112,16 +112,75 @@ tabpanels {
> .tabbrowser-tab[beforeselected="true"]:-moz-locale-dir(ltr) {
>   -moz-border-right-colors: transparent transparent;
> }
> 
> .tabbrowser-tab[beforeselected="true"]:-moz-locale-dir(rtl) {
>   -moz-border-left-colors: transparent transparent;
> }
> 
>+/* ::::: Tab scrollbox arrow ::::: */
>+
>+.scrollbutton-up,
>+.scrollbutton-down {
>+  border: 0;
>+  padding: 0 4px;
>+  margin: 0;
>+}
>+
>+.scrollbutton-up {
>+  border-right: 2px solid;
>+  -moz-border-right-colors: rgba(0, 0, 0, 0.19) transparent;
>+  list-style-image: url("chrome://messenger/skin/icons/tab-arrow-left.png");
>+  -moz-image-region: rect(0px, 7px, 11px, 0px);
>+}
>+
>+.scrollbutton-up[disabled="true"] {
>+  -moz-image-region: rect(0px, 14px, 11px, 7px);
>+  -moz-border-right-colors: transparent transparent;
>+}
>+
>+.scrollbutton-down {
>+  border-left: 2px solid;
>+  -moz-border-left-colors: rgba(0 ,0 ,0 , 0.19) transparent;
>+  list-style-image: url("chrome://messenger/skin/icons/tab-arrow-right.png");
>+  -moz-image-region: rect(0px, 7px, 11px, 0px);
>+  -moz-transition: 1s box-shadow ease-out;
>+}
>+
>+.scrollbutton-down[notifybgtab] {
>+  background-color: Highlight;
>+  box-shadow: 0 0 5px 5px Highlight inset;
>+  -moz-transition: none;
>+}
>+
>+.scrollbutton-down[disabled="true"] {
>+  -moz-image-region: rect(0px, 14px, 11px, 7px);
>+  -moz-border-left-colors: transparent transparent;
>+}
>+
>+.scrollbutton-up:hover:not([disabled="true"]),
>+.scrollbutton-down:hover:not([disabled="true"]) {
>+  background-color: rgba(0, 0, 0, 0.1);
>+}
>+
>+.scrollbutton-up:hover:active:not([disabled="true"]),
>+.scrollbutton-down:hover:active:not([disabled="true"]) {
>+  background-color: rgba(0, 0, 0 , 0.2);
>+}
>+
>+/* All Tabs Menupopup */
>+.alltabs-item {
>+  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.png");
>+}
>+
>+.alltabs-item[busy] {
>+  list-style-image: url("chrome://global/skin/icons/loading_16.png") !important;
>+}
>+
> /* ::::: close button ::::: */
> 
> .tabs-closebutton {
>   margin: 3px;
>   padding: 1px;
>   list-style-image: url("chrome://global/skin/icons/closetab.png");
> }
> 
>@@ -142,17 +201,17 @@ tabpanels {
>   margin: 0;
>   padding: 1px;
>   list-style-image: url("chrome://navigator/skin/icons/tab-new.gif");
> }
> 
> .tab-drop-indicator-bar {
>   height: 11px;
>   margin-top: -11px;
>-  -moz-margin-start: -8px;
>+  -moz-margin-start: -6px;
>   position: relative;
> }
>  
> .tab-drop-indicator {
>   height: 11px;
>   width: 11px;
>   margin-bottom: -5px;
>   position: relative;
>diff --git a/suite/themes/classic/navigator/icons/tab-arrow-left.png b/suite/themes/classic/navigator/icons/tab-arrow-left.png
>new file mode 100644
>index 0000000000000000000000000000000000000000..0ef2b1ae61e2d4396ac70fab94dab6a7b49ef1aa
>GIT binary patch
>literal 460
>zc%17D@N?(olHy`uVBq!ia0vp^azHG|!3-q-?s(%0q*es@gn;Nf&)*Y?UVixc;{6vj
>z4NbNBS!zvl)LQ0g8(ZjG*t*Z(9vB{DYGr5Izua=-3ad#gZJgblJbnB^A_C@b4vL5g
>zn!h<@`QFflTO$)vVw2KilhYH@vy!s%Iop<T3yBMKt`O{7CnPN=CZ{MibC1Nt4RUJQ
>zDhm$im{{qX+Zb5c8(G;cc78Vx=vJ+gAirP+Miw@9E>SUY2}xNkZDTjz8DOw<>2fgG
>zi~uvk13v?Gt@Lzp4ABT)d-1r?Aq4@p1Sglv3IZl&y&I?BIQ@J7Obf{f8-utzx3}AG
>z-pt@sufZ?eWUKzSLCyC~%83?_j-xui7v1=-$$0ASfn^qcl?{q(xqMrg)%=X*e$HFA
>z#>?4d#SXp7I|fspTLx7Ad9_i(uRkw#!K`E9{c~E=EaGEswPj83$e(9vyxMionk8v>
>zZ``vk5S}UzWJUOEY>ePh{I@4**0v4J`}ZA*%nv-p{r#BF&r3i@GkCiCxvX<aXaWEg
>CQq4;M
>
>diff --git a/suite/themes/classic/navigator/icons/tab-arrow-right.png b/suite/themes/classic/navigator/icons/tab-arrow-right.png
>new file mode 100644
>index 0000000000000000000000000000000000000000..96653e9e7a6471c27c41ee5f0e769d2c6eb2b0bb
>GIT binary patch
>literal 535
>zc%17D@N?(olHy`uVBq!ia0vp^azHG|!3-q-?s(%0q*&4&eH|GXHuiJ>Nn{1`ISV`@
>ziy0XB4uCLY*0oMfprB-lYeY$Kep*R+Vo@qXKw@TIiJqTph(ejMo~fRxXV;$xKt(G8
>zd_q7b-g*9>Nc8f<*B9@<sA*`b)z4CEnxob-Puti+-@?{?{`SD|7*i`d)Bfd_6IWPG
>zT503#=H%()7ZMRLe{)boOwjzzA<OrMF5DWKkP@4e9-Ew=ke-#4mCxC>j9W-tpmT*_
>z-#Q^_IWaj!v6*`$CT@^Z(^gq<K*z*N-`vK)%HGJzZn5*bc|f;nl?3?(GcdBSv2%%v
>ziAzYzYH1t0`OW}?rAwEC!Da-Q86Nl<sB4v{i(`mJ@Y0LNg$^kQG$bl4TH>IP@kT0H
>zf8+G;^~T>9q}aOf7ybU0E+N73vpn#a<>FA4HFBO0FLDXVNiME2e-JoHK}c_R^p%}&
>za&=z$y!g29&4D!zbE_KX$@B=FF>L>*#FMpf-u&Jts>|JftG(e5eQsDHd1D`s*DtNT
>zx?8O8grB|q#&<`z*L!W*9ji5?pY(m0bgi^EvwTI9`)U4f#x94X3U#!8zV_a?M%~YX
>W`@5~=rCOk?89ZJ6T-G@yGywnxcH)5m
>
>diff --git a/suite/themes/classic/navigator/tabbrowser.css b/suite/themes/classic/navigator/tabbrowser.css
>--- a/suite/themes/classic/navigator/tabbrowser.css
>+++ b/suite/themes/classic/navigator/tabbrowser.css
>@@ -43,16 +43,104 @@ tabpanels {
> }
> 
> .tabbrowser-tab[selected="true"] {
>   margin-bottom: 0px;
>   padding-top: 2px; /* compensates the top margin of background tabs */
>   padding-bottom: 3px; /* compensates the bottom margin of background tabs */
> }
> 
>+/* ::::: Tab scrollbox arrow, and all-tabs buttons ::::: */
>+
>+.scrollbutton-up,
>+.scrollbutton-down {
>+  margin: 2px 0 1px;
>+  padding: 0;
>+  border-top: 2px solid;
>+  border-right: 2px solid;
>+  border-left: 2px solid;
>+  border-bottom: 1px solid ThreeDHighlight;
>+  -moz-border-top-colors: ThreeDHighlight ThreeDLightShadow;
>+  -moz-border-right-colors: ThreeDDarkShadow ThreeDShadow;
>+  -moz-border-left-colors: ThreeDHighlight ThreeDLightShadow;
>+  border-radius: 2px 2px 0 0;
>+}
>+
>+.scrollbutton-up:-moz-locale-dir(rtl),
>+.scrollbutton-down:-moz-locale-dir(ltr) {
>+  -moz-border-right-colors: ThreeDHighlight ThreeDShadow;
>+}
>+
>+.scrollbutton-up,
>+.scrollbutton-down:-moz-locale-dir(rtl) {
>+  list-style-image: url("chrome://navigator/skin/icons/tab-arrow-left.png");
>+}
>+
>+.scrollbutton-down,
>+.scrollbutton-up:-moz-locale-dir(rtl) {
>+  list-style-image: url("chrome://navigator/skin/icons/tab-arrow-right.png");
>+}
>+
>+.scrollbutton-up,
>+.scrollbutton-down {
>+  -moz-image-region: rect(0, 15px, 17px, 0);
>+}
>+
>+.scrollbutton-up[disabled="true"],
>+.scrollbutton-down[disabled="true"] {
>+  opacity: .4;
>+}
>+
>+.scrollbutton-up:not([disabled="true"]):hover:active,
>+.scrollbutton-down:not([disabled="true"]):hover:active {
>+  -moz-image-region: rect(0, 30px, 17px, 15px);
>+}
>+
>+.scrollbutton-down {
>+  -moz-transition: 1s background-color ease-out;
>+}
>+
>+.scrollbutton-down[notifybgtab] {
>+  background-color: Highlight;
>+  -moz-transition: none;
>+}
>+
>+.tabs-alltabs-button {
>+  margin: 2px 0 1px;
>+}
>+
>+.tabs-alltabs-button > .toolbarbutton-text {
>+  display: none;
>+}
>+
>+.scrollbutton-up:-moz-lwtheme-brighttext,
>+.scrollbutton-down:-moz-lwtheme-brighttext,
>+.tabs-alltabs-button:-moz-lwtheme-brighttext {
>+  background-color: rgba(0,0,0,.5);
>+}
>+
>+.scrollbutton-up:-moz-lwtheme-darktext,
>+.scrollbutton-down:-moz-lwtheme-darktext,
>+.tabs-alltabs-button:-moz-lwtheme-darktext {
>+  background-color: rgba(255,255,255,.5);
>+}
>+
>+/* All tabs menupopup */
>+.alltabs-item {
>+  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.png");
>+}
>+
>+.alltabs-item[selected="true"] {
>+  font-weight: bold;
>+}
>+
>+.alltabs-item[busy] {
>+  list-style-image: url("chrome://communicator/skin/icons/loading.gif");
>+}
>+
> /* ::::: close button ::::: */
> 
> .tabs-closebutton {
>   margin: 3px;
>   list-style-image: url("chrome://communicator/skin/icons/close-button.gif");
> }
> 
> .tabs-closebutton > .toolbarbutton-icon {
>@@ -62,17 +150,17 @@ tabpanels {
> .tabs-newbutton {
>   margin: 0px;
>   list-style-image: url("chrome://navigator/skin/icons/tab-new.gif");
> }
> 
> .tab-drop-indicator-bar {
>     height: 11px;
>     margin-top: -11px;
>-    -moz-margin-start: -8px;
>+    -moz-margin-start: -6px;
>     position: relative;
> }
> 
> .tab-drop-indicator {
>     height: 11px;
>     width: 11px;
>     margin-bottom: -5px;
>     position: relative;
>diff --git a/suite/themes/modern/jar.mn b/suite/themes/modern/jar.mn
>--- a/suite/themes/modern/jar.mn
>+++ b/suite/themes/modern/jar.mn
>@@ -560,9 +560,10 @@ modern.jar:
>   skin/modern/navigator/icons/browser.png                          (navigator/icons/browser.png)
>   skin/modern/navigator/icons/browser-small.png                    (navigator/icons/browser-small.png)
>   skin/modern/navigator/icons/identity.png                         (navigator/icons/identity.png)
>   skin/modern/navigator/icons/popup-blocked.png                    (navigator/icons/popup-blocked.png)
>   skin/modern/navigator/icons/tab-drag-indicator.gif               (navigator/icons/tab-drag-indicator.gif)
>   skin/modern/navigator/icons/tab-new.gif                          (navigator/icons/tab-new.gif)
>   skin/modern/navigator/icons/tab-new-hov.gif                      (navigator/icons/tab-new-hov.gif)
>   skin/modern/navigator/icons/tab-new-act.gif                      (navigator/icons/tab-new-act.gif)
>+  skin/modern/navigator/icons/tabscroll.png                        (navigator/icons/tabscroll.png)
>   skin/modern/navigator/icons/windowcontrols.png                   (navigator/icons/windowcontrols.png)
>diff --git a/suite/themes/modern/navigator/icons/tabscroll.png b/suite/themes/modern/navigator/icons/tabscroll.png
>new file mode 100644
>index 0000000000000000000000000000000000000000..d5053d04734bec710a4b154c6106b16a8c7f56d2
>GIT binary patch
>literal 151
>zc%17D@N?(olHy`uVBq!ia0vp^B0$W^!VDx2ycetnQv3lvA+G-!7|I$at=_UvKv-ht
>z%$Y~GCe{F@7)yfuf*Bm1-ADs+%sgEjLn>}1CnSg@1PG-h1SKRS2#6&VF(tCGxg|4-
>wswX<DiL<gLCM$}o8#Fd|E)hG)(z27`yB(M7aoYtafF>|_y85}Sb4q9e08qmz=Kufz
>
>diff --git a/suite/themes/modern/navigator/tabbrowser.css b/suite/themes/modern/navigator/tabbrowser.css
>--- a/suite/themes/modern/navigator/tabbrowser.css
>+++ b/suite/themes/modern/navigator/tabbrowser.css
>@@ -50,29 +50,126 @@
> }
> 
> .tabs-left {
>   width: 3px;
> }
> 
> tab {
>   padding: 0px 3px;
>-  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.gif"); 
>+  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.gif");
> }
> 
> tab[busy] {
>   list-style-image: url("chrome://communicator/skin/icons/loading.gif");
> }
> 
> .tab-icon {
>   -moz-margin-end: 3px;
>   width: 16px;
>   height: 16px;
> }
> 
>+/* ::::: Tab scrollbox arrow, and all-tabs buttons ::::: */
>+
>+.scrollbutton-up,
>+.scrollbutton-down {
>+  list-style-image: url("chrome://navigator/skin/icons/tabscroll.png");
>+  padding: 0 3px;
>+  margin: 0px;
>+  border-bottom: 3px solid;
>+  -moz-border-bottom-colors: #000000 transparent;
>+}
>+
>+.scrollbutton-up {
>+  -moz-image-region: rect(0px, 5px, 9px, 0px);
>+  border-right: 2px solid;
>+  -moz-border-right-colors: #000000 transparent;
>+}
>+
>+.scrollbutton-down {
>+  -moz-image-region: rect(0px, 15px, 9px, 10px);
>+  border-left: 2px solid;
>+  -moz-border-left-colors: #000000 transparent;
>+  -moz-transition: 1s background-color ease-out;
>+}
>+
>+.scrollbutton-down[notifybgtab],
>+.scrollbutton-down:hover:not([disabled="true"]),
>+.scrollbutton-up:hover:active:not([disabled="true"]),
>+.scrollbutton-down[disabled="true"] {
>+  -moz-transition: none;
>+}
>+
>+.scrollbutton-up[disabled="true"] {
>+  -moz-image-region: rect(0px, 10px, 9px, 5px);
>+}
>+
>+.scrollbutton-down[disabled="true"] {
>+  -moz-image-region: rect(0px, 20px, 9px, 15px);
>+}
>+
>+.scrollbutton-down[notifybgtab] {
>+  background-color: #8C9AA8;
>+}
>+
>+.scrollbutton-up:hover:not([disabled="true"]) {
>+  -moz-border-right-colors: #000000 #86929E;
>+}
>+
>+.scrollbutton-down:hover:not([disabled="true"]) {
>+  -moz-border-left-colors: #000000 #EEF0F3;
>+}
>+
>+.scrollbutton-up:hover:active:not([disabled="true"]) {
>+  background-color: #8C9AA8;
>+  border-top-color: #454C55;
>+  -moz-border-right-colors: #000000 #7D848D;
>+  border-bottom-color: #7D848D;
>+  border-left-color: #454C55;
>+}
>+
>+.scrollbutton-down:hover:active:not([disabled="true"]) {
>+  background-color: #8C9AA8;
>+  border-top-color: #454C55;
>+  border-right-color: #7D848D;
>+  border-bottom-color: #7D848D;
>+  -moz-border-left-colors: #000000 #454C55;
>+}
>+
>+.tabs-alltabs-button {
>+  padding: 0;
>+  -moz-padding-start: 1px;
>+  -moz-padding-end: 3px;
>+  margin: 0px;
>+  border-bottom: 3px solid;
>+  -moz-border-bottom-colors: #000000 transparent;
>+}
>+
>+.tabs-alltabs-button > .toolbarbutton-text {
>+  display: none;
>+}
>+
>+/* All tabs menupopup */
>+.alltabs-item {
>+  list-style-image: url("chrome://communicator/skin/bookmarks/bookmark-item.gif");
>+}
>+
>+.alltabs-item[selected="true"] {
>+  font-weight: bold;
>+}
>+
>+.alltabs-item[busy] {
>+  list-style-image: url("chrome://communicator/skin/icons/loading.gif");
>+}
>+
>+.alltabs-item[busy][_moz-menuactive="true"] {
>+  -moz-image-region: rect(0px, 32px, 16px, 16px);
>+}
>+
> /* ::::: close button ::::: */
> 
> .tabs-closebutton {
>   margin: 0px 4px;
>   padding: 3px 2px;
>   border: none;
>   list-style-image: url("chrome://global/skin/icons/close.gif");
> }
>@@ -106,17 +203,17 @@ tab[busy] {
> 
> .tabs-newbutton:hover:active {
>   list-style-image: url("chrome://navigator/skin/icons/tab-new-act.gif");
> }
> 
> .tab-drop-indicator-bar {
>     height: 11px;
>     margin-top: -11px;
>-    -moz-margin-start: -8px;
>+    -moz-margin-start: -6px;
>     position: relative;
> }
> 
> .tab-drop-indicator {
>     height: 11px;
>     width: 11px;
>     margin-bottom: -5px;
>     position: relative;
Attachment #497068 - Flags: review?(iann_bugzilla)
Comment on attachment 497068 [details] [diff] [review]
Patch v1.2 Fix Mac UI issues.

+            if (this.mPrefs.getBoolPref("ui.click_hold_context_menus") &&
+                this.mContextTab.localName != "tab")
+              return false;

In tabmail it's not possible to get the "tab" context menu unless you right-click on a tab. The above makes tabbrowser behaviour pref-driven, but I would say that the tabmail behaviour makes more sense. Any reason for not going the tabmail route?
yeah I think you're right. I'll do it the tabmail way.
> In tabmail it's not possible to get the "tab" context menu unless you
> right-click on a tab. The above makes tabbrowser behaviour pref-driven, but I
> would say that the tabmail behaviour makes more sense. Any reason for not going
> the tabmail route?
Fixed.
Attachment #497068 - Attachment is obsolete: true
Attachment #497238 - Flags: ui-review?(stefanh)
Attachment #497238 - Flags: review?(iann_bugzilla)
Attachment #497068 - Flags: ui-review?(stefanh)
Attachment #497068 - Flags: review?(neil)
Attachment #497068 - Flags: review?(iann_bugzilla)
Comment on attachment 497238 [details] [diff] [review]
Patch v1.2 Really Fix Mac UI issues.

+  skin/classic/navigator/icons/alltabs-box-bkgnd-icon.png               (mac/navigator/icons/alltabs-box-bkgnd-icon.png)

Oops, you forgot to remove this ;-) Looks good otherwise, thanks.
Attachment #497238 - Flags: ui-review?(stefanh) → ui-review+
Comment on attachment 497238 [details] [diff] [review]
Patch v1.2 Really Fix Mac UI issues.

I've only looked at this quickly, it's not a full review.

>-          <xul:menupopup anonid="tabContextMenu" onpopupshowing="this.parentNode.parentNode.parentNode.updatePopupMenu(this);">
>+          <xul:menupopup anonid="tabContextMenu" onpopupshowing="return document.getBindingParent(this).updatePopupMenu(this);">
I guess you optimised this one because you had to change it anyway?

>+              return false;
>             var disabled = this.tabs.length == 1;
Nit: a blank line wouldn't go amiss.

>               case 1:
>                 // add a new blank tab to replace the one we're about to close
>                 // (this ensures that the remaining tab is as good as new)
>                 this.addTab("about:blank");
>+                this.tabContainer._fillTrailingGap();
Why would we have a trailing gap here?

>             if (aDragSession.sourceNode &&
>-                aDragSession.sourceNode.parentNode == this.tabContainer) {
>-              var newIndex = this.getDropIndex(aEvent);
>-              var tabIndex = this.getTabIndex(aDragSession.sourceNode);
>-              if (newIndex == tabIndex || newIndex == tabIndex + 1)
>-                return false;
>-            }
>+                aDragSession.sourceNode.parentNode == this.tabContainer &&
>+                (aEvent.screenX >= aDragSession.sourceNode.boxObject.screenX &&
>+                 aEvent.screenX <= (aDragSession.sourceNode.boxObject.screenX +
>+                                    aDragSession.sourceNode.boxObject.width)))
>+              return false;
So what happens now when you drop a tab on itself?

>+                  pixelsToScroll = tabStrip.scrollIncrement * -1;
Nit: -tabStrip.scrollIncrement

>+                tabStrip.scrollByPixels((ltr ? -1 : 1) * pixelsToScroll);
ltr isn't defined for another 7 lines :-(

>+                                     //ib.getBoundingClientRect().right - ind.clientWidth);
???

>+              newMargin = ltr? tabRect.left - rect.left:
Nit: spaces before ? and :

>-              var coord = this.tabs[i].boxObject.x +
>+              var coord = this.tabs[i].boxObject.screenX +
Does the scrollbox mess up the x coordinates?

>-          t.setAttribute("width", 0);
>-          t.setAttribute("flex", 100);
>+          t.width = 0;
>+          t.setAttribute("flex", "100");
Might as well set t.flex = 100; while you're about it.

>+              <children includes="tab"/>
Which other children are there?

>+            <xul:stack align="center" pack="end">
Align and pack make no sense on a stack...

>-              <xul:hbox class="tabs-closebutton-box" align="center" pack="end">
>+              <xul:hbox class="tabs-closebutton-box" align="stretch" pack="end">
This stretches the closebutton to the height of the tabstrip...

>+      <constructor>
>+        <![CDATA[
>+          this.mTabMinWidth = this.mPrefs.getIntPref("browser.tabs.tabMinWidth");
>+          this.mTabMaxWidth = this.mPrefs.getIntPref("browser.tabs.tabMaxWidth");
>+          var tab = this.firstChild;
>+          tab.style.minWidth = this.mTabMinWidth + "px";
>+          tab.style.maxWidth = this.mTabMaxWidth + "px";
These values are only ever used by the tabbrowser constructor, so it seems silly to cache them here for the lifetime of the window...

>+              this._fillTrailingGap();
Why would a new tab create a trailing gap? (Come to think of it, where does closing a tab get handled? It's not readily apparent...)

>+            if (!aEvent.isTrusted)
>+              return;
Not expecting events from content targetted at chrome elements...

>+            menuItem.setAttribute("image", aTabNode.getAttribute("image"));
>+
>+            ["busy", "selected"].forEach(
tabs don't necessarily have images either, might as well reuse code ;-)

>+            menuItem.addEventListener("command", this, false);
>+            this.appendChild(menuItem);
Unnecessary event listener, since the event bubbles up to its parent. Fortunately you don't listen to it in the handleEvent method.

>+            menuItem.tab.mCorrespondingMenuitem = null;
Might as well set menuItem.tab to null too ;-)

>+/* ::::: Tab scrollbox arrow, and all-tabs buttons ::::: */
So, why do these look like tabs?

>+.scrollbutton-up:not([disabled="true"]):hover:active,
>+.scrollbutton-down:not([disabled="true"]):hover:active {
>+  -moz-image-region: rect(0, 30px, 17px, 15px);
Odd choice of active feedback.

>-    -moz-margin-start: -8px;
>+    -moz-margin-start: -6px;
Any idea what caused this?
Comment on attachment 497238 [details] [diff] [review]
Patch v1.2 Really Fix Mac UI issues.

>+++ b/suite/browser/tabbrowser.xml

>-            if (!aDragSession.canDrop) {
>+            /* autoscroll the tab strip if we drag over the scroll
>+             * buttons, even if we aren't dragging a tab
>+             */
Nit: other comments in this section aren't written in this way, so use style used elsewhere.

>+            var scrollRect = tabStrip.scrollClientRect;
>+            var rect = this.getBoundingClientRect();
>+            var minMargin = scrollRect.left - rect.left;
>+            var maxMargin = Math.min(minMargin + scrollRect.width,
>+                                     ib.getBoundingClientRect().right);
>+                                     //ib.getBoundingClientRect().right - ind.clientWidth);
Nit: is this comment still needed?

>+++ b/suite/themes/classic/mac/navigator/tabbrowser.css

>+.scrollbutton-down {
>+  border-left: 2px solid;
>+  -moz-border-left-colors: rgba(0 ,0 ,0 , 0.19) transparent;
Nit: space after comma not before.

>+.scrollbutton-up:hover:active:not([disabled="true"]),
>+.scrollbutton-down:hover:active:not([disabled="true"]) {
>+  background-color: rgba(0, 0, 0 , 0.2);
Nit: extra space before 3rd comma.

>+++ b/suite/themes/classic/navigator/tabbrowser.css

>+.scrollbutton-up:-moz-lwtheme-brighttext,
>+.scrollbutton-down:-moz-lwtheme-brighttext,
>+.tabs-alltabs-button:-moz-lwtheme-brighttext {
>+  background-color: rgba(0,0,0,.5);
Nit: Missing spaces after comma and 0.5 looks better than .5

>+.scrollbutton-up:-moz-lwtheme-darktext,
>+.scrollbutton-down:-moz-lwtheme-darktext,
>+.tabs-alltabs-button:-moz-lwtheme-darktext {
>+  background-color: rgba(255,255,255,.5);
Nit: Missing spaces after comma and 0.5 looks better than .5

r=me with those and Neil's comments addressed (he may yet have more).
Attachment #497238 - Flags: review?(iann_bugzilla) → review+
> Stefan      2010-12-13 08:17:48 PST
> 
> +  skin/classic/navigator/icons/alltabs-box-bkgnd-icon.png              
> (mac/navigator/icons/alltabs-box-bkgnd-icon.png)
> 
> Oops, you forgot to remove this ;-) Looks good otherwise, thanks.
Fixed.

> Ian Neal      2010-12-14 14:37:02 PST
> 
>>+++ b/suite/browser/tabbrowser.xml
> 
>>-            if (!aDragSession.canDrop) {
>>+            /* autoscroll the tab strip if we drag over the scroll
>>+             * buttons, even if we aren't dragging a tab
>>+             */
> Nit: other comments in this section aren't written in this way, so use style
> used elsewhere.
Fixed.

>>+            var scrollRect = tabStrip.scrollClientRect;
>>+            var rect = this.getBoundingClientRect();
>>+            var minMargin = scrollRect.left - rect.left;
>>+            var maxMargin = Math.min(minMargin + scrollRect.width,
>>+                                     ib.getBoundingClientRect().right);
>>+                                     //ib.getBoundingClientRect().right - ind.clientWidth);
> Nit: is this comment still needed?
Removed.

>>+++ b/suite/themes/classic/mac/navigator/tabbrowser.css
> 
>>+.scrollbutton-down {
>>+  border-left: 2px solid;
>>+  -moz-border-left-colors: rgba(0 ,0 ,0 , 0.19) transparent;
> Nit: space after comma not before.
Fixed.

>>+.scrollbutton-up:hover:active:not([disabled="true"]),
>>+.scrollbutton-down:hover:active:not([disabled="true"]) {
>>+  background-color: rgba(0, 0, 0 , 0.2);
> Nit: extra space before 3rd comma.
Fixed.

>>+++ b/suite/themes/classic/navigator/tabbrowser.css
> 
>>+.scrollbutton-up:-moz-lwtheme-brighttext,
>>+.scrollbutton-down:-moz-lwtheme-brighttext,
>>+.tabs-alltabs-button:-moz-lwtheme-brighttext {
>>+  background-color: rgba(0,0,0,.5);
> Nit: Missing spaces after comma and 0.5 looks better than .5
Fixed.

>>+.scrollbutton-up:-moz-lwtheme-darktext,
>>+.scrollbutton-down:-moz-lwtheme-darktext,
>>+.tabs-alltabs-button:-moz-lwtheme-darktext {
>>+  background-color: rgba(255,255,255,.5);
> Nit: Missing spaces after comma and 0.5 looks better than .5
Fixed.

> r=me with those and Neil's comments addressed (he may yet have more).
What do you mean "may"?

> neil@parkwaycc.co.uk      2010-12-14 04:57:11 PST
> 
> Comment on attachment 497238 [details] [diff] [review]
> Patch v1.2 Really Fix Mac UI issues.
> 
> I've only looked at this quickly, it's not a full review.
> 
>>-          <xul:menupopup anonid="tabContextMenu" onpopupshowing="this.parentNode.parentNode.parentNode.updatePopupMenu(this);">
>>+          <xul:menupopup anonid="tabContextMenu" onpopupshowing="return document.getBindingParent(this).updatePopupMenu(this);">
> I guess you optimised this one because you had to change it anyway?
Yes. Also to sort of match the syntax tabmail uses.

>>+              return false;
>>             var disabled = this.tabs.length == 1;
> Nit: a blank line wouldn't go amiss.
Fixed.

>>               case 1:
>>                 // add a new blank tab to replace the one we're about to close
>>                 // (this ensures that the remaining tab is as good as new)
>>                 this.addTab("about:blank");
>>+                this.tabContainer._fillTrailingGap();
> Why would we have a trailing gap here?

sspitzer introduced this in Bug 346441 (Closing the rightmost tab in the overflow case looks bad)
Dão refactored it in Bug 473745 (Implement tabContainer._fillTrailingGap)

>>             if (aDragSession.sourceNode &&
>>-                aDragSession.sourceNode.parentNode == this.tabContainer) {
>>-              var newIndex = this.getDropIndex(aEvent);
>>-              var tabIndex = this.getTabIndex(aDragSession.sourceNode);
>>-              if (newIndex == tabIndex || newIndex == tabIndex + 1)
>>-                return false;
>>-            }
>>+                aDragSession.sourceNode.parentNode == this.tabContainer &&
>>+                (aEvent.screenX >= aDragSession.sourceNode.boxObject.screenX &&
>>+                 aEvent.screenX <= (aDragSession.sourceNode.boxObject.screenX +
>>+                                    aDragSession.sourceNode.boxObject.width)))
>>+              return false;
> So what happens now when you drop a tab on itself?
As expected. Nothing.

>>+                  pixelsToScroll = tabStrip.scrollIncrement * -1;
> Nit: -tabStrip.scrollIncrement
Fixed.

>>+                tabStrip.scrollByPixels((ltr ? -1 : 1) * pixelsToScroll);
> ltr isn't defined for another 7 lines :-(
Fixed.

>>+                                     //ib.getBoundingClientRect().right - ind.clientWidth);
> ???
Fixedd.

>>+              newMargin = ltr? tabRect.left - rect.left:
> Nit: spaces before ? and :
Fixed.

>>-              var coord = this.tabs[i].boxObject.x +
>>+              var coord = this.tabs[i].boxObject.screenX +
> Does the scrollbox mess up the x coordinates?
Yeah see Comment 29 where IanN describes the effect.

>>-          t.setAttribute("width", 0);
>>-          t.setAttribute("flex", 100);
>>+          t.width = 0;
>>+          t.setAttribute("flex", "100");
> Might as well set t.flex = 100; while you're about it.
Fixed.

>>+              <children includes="tab"/>
> Which other children are there?

The Firefox tabbrowser has these comments (which I didn't reproduce)

> # This is a hack to circumvent bug 472020, otherwise the tabs show up on the
> # right of the newtab button.
>         <children includes="tab"/>
> # This is to ensure anything extensions put here will go before the newtab
> # button, necessary due to the previous hack.
>         <children/>

See Bug 457651 comment 57 and Bug 457651 comment 98. As well as Bug 472020.

>>+            <xul:stack align="center" pack="end">
> Align and pack make no sense on a stack...
Fixed. I miscopied it from tabmail
Someone should tell mscott ... who copied it from sspitzer:
Bug 342900 - open tab in background gives no indication that tab opened in overflow area.
It was originally a hbox but when Seth changed it into a stack he forgot to remove the align/pack attributes. And Mano didn't notice it on review.
/me sighs.

>>-              <xul:hbox class="tabs-closebutton-box" align="center" pack="end">
>>+              <xul:hbox class="tabs-closebutton-box" align="stretch" pack="end">
> This stretches the closebutton to the height of the tabstrip...
I was hoping nobody noticed.
Fixed I think.

>>+      <constructor>
>>+        <![CDATA[
>>+          this.mTabMinWidth = this.mPrefs.getIntPref("browser.tabs.tabMinWidth");
>>+          this.mTabMaxWidth = this.mPrefs.getIntPref("browser.tabs.tabMaxWidth");
>>+          var tab = this.firstChild;
>>+          tab.style.minWidth = this.mTabMinWidth + "px";
>>+          tab.style.maxWidth = this.mTabMaxWidth + "px";
> These values are only ever used by the tabbrowser constructor, so it seems
> silly to cache them here for the lifetime of the window...
Fixed.

>>+              this._fillTrailingGap();
> Why would a new tab create a trailing gap?
I'm missing adjustTabstrip()
Removed for the time being.

Firefox does this:
> this.adjustTabstrip();
> this._fillTrailingGap();
> this._handleTabSelect();

I haven't implemented |adjustTabstrip| yet. It'll be part of the tabs-close-button patch which stalled way back after I got bikeshedded.

> (Come to think of it, where does
> closing a tab get handled? It's not readily apparent...)
In the removeTab method?

>>+            if (!aEvent.isTrusted)
>>+              return;
> Not expecting events from content targetted at chrome elements...
Removed.

Dunno introduced way back by seth@sspitzer.org with no explanation in:
Bug 343251 - add "all tabs" menu to tabstrip to address usability problems with tab overflow / scrolling

>>+            menuItem.setAttribute("image", aTabNode.getAttribute("image"));
>>+
>>+            ["busy", "selected"].forEach(
> tabs don't necessarily have images either, might as well reuse code ;-)
Fixed.

>>+            menuItem.addEventListener("command", this, false);
>>+            this.appendChild(menuItem);
> Unnecessary event listener, since the event bubbles up to its parent.
> Fortunately you don't listen to it in the handleEvent method.
Fixed.

>>+            menuItem.tab.mCorrespondingMenuitem = null;
> Might as well set menuItem.tab to null too ;-)
Fixed.

>>+/* ::::: Tab scrollbox arrow, and all-tabs buttons ::::: */
> So, why do these look like tabs?
I didn't like the way Firefox styled these. The discontinuity in styles between the tabs and the scrollbutton bookends was too jarring for me.
So I fiddled around a bit until I got something that matched the tabs and fitted better on the tabstrip.

>>+.scrollbutton-up:not([disabled="true"]):hover:active,
>>+.scrollbutton-down:not([disabled="true"]):hover:active {
>>+  -moz-image-region: rect(0, 30px, 17px, 15px);
> Odd choice of active feedback.
I copied the images and styles from Firefox since I didn't have any other graphics handy.

>>-    -moz-margin-start: -8px;
>>+    -moz-margin-start: -6px;
> Any idea what caused this?
I stared at my patch for a long time but couldn't see where the 2px came in. My WAG is somewhere in the switch from boxObject to getBoundingClientRect().

In a followup someone (else) can port Bug 508499 (simplify tab drop indicator code and styling) which removes the negative margins completely.
Attachment #497238 - Attachment is obsolete: true
Attachment #498122 - Flags: review?(neil)
Ironically the margin was there to simplify the code...
Comment on attachment 498122 [details] [diff] [review]
Patch v1.3 fixed review issues to date.

Looking at the Modern theme for this pass.

>+.scrollbutton-up,
>+.scrollbutton-down {
>+  list-style-image: url("chrome://navigator/skin/icons/tabscroll.png");
What's the difference between this image and the existing arrow images? (Or indeed, most of the rest of the styles too. I think the margin: 0px; is OK.)

>+  padding: 0 3px;
I think this causes problems with the default toolbarbutton padding, which gets overridden when the button is disabled. It seems unnecessary anyway.

>+  border-bottom: 3px solid;
>+  -moz-border-bottom-colors: #000000 transparent;
Nice way of forcing a bottom border, but why 3px? 2px will do.

>+  border-right: 2px solid;
>+  -moz-border-right-colors: #000000 transparent;
I like this, in particular when a tab is partially hidden. However it looks bad when the tabstrip is fully scrolled to the end. Conveniently you can avoid this by switching to a black border end colour (thus also fixing it in RTL, right?) as this is the automatically removed when the button is disabled.

>+.tabs-alltabs-button {
>+  padding: 0;
>+  -moz-padding-start: 1px;
>+  -moz-padding-end: 3px;
I bet this is to compensate for the margin on the toolbarbutton-icon. Easier would be to remove the margin or hide the icon instead. (But you might want to remove the margin on the scroll arrows for the same reason.)

>+.alltabs-item[busy][_moz-menuactive="true"] {
>+  -moz-image-region: rect(0px, 32px, 16px, 16px);
This doesn't work, loading.gif is only 16x16
Comment on attachment 498122 [details] [diff] [review]
Patch v1.3 fixed review issues to date.

>+    <resources>
>+      <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>+    </resources>
I think we might have too many of these. When I was inspecting stuff in DOM Inspector I saw some styles being applied three times! In this worst case I think one set came from the tabbrowser itself (since all of this stuff is anonymous tabbrowser content), one from the tabbrowser-tabs (which may well be unnecessary) and one from the tabbrowser-arrowscrollbox.

(In reply to comment #45)
> > So what happens now when you drop a tab on itself?
> As expected. Nothing.
If you notice, the previous code wouldn't even have shown a drop indicator.
Attached patch Patch v1.4 Modern Fixes, etc. (obsolete) — — Splinter Review 
> neil@parkwaycc.co.uk      2010-12-18 16:26:52 PST
> 
> Looking at the Modern theme for this pass.

Note: most of the styles copied from Kuden's Past Modern.

>>+.scrollbutton-up,
>>+.scrollbutton-down {
>>+  list-style-image: url("chrome://navigator/skin/icons/tabscroll.png");
> What's the difference between this image and the existing arrow images? (Or
> indeed, most of the rest of the styles too. I think the margin: 0px; is OK.)
Removed tabscroll.png. This was copied from Past Modern.

>>+  padding: 0 3px;
> I think this causes problems with the default toolbarbutton padding, which gets
> overridden when the button is disabled. It seems unnecessary anyway.
Removed.

>>+  border-bottom: 3px solid;
>>+  -moz-border-bottom-colors: #000000 transparent;
> Nice way of forcing a bottom border, but why 3px? 2px will do.
Fixed. 2px.

>>+  border-right: 2px solid;
>>+  -moz-border-right-colors: #000000 transparent;
> I like this, in particular when a tab is partially hidden. However it looks bad
> when the tabstrip is fully scrolled to the end. Conveniently you can avoid this
> by switching to a black border end colour (thus also fixing it in RTL, right?)
> as this is the automatically removed when the button is disabled.
Switched to -moz-border-end-colors: and (for the down button) -moz-border-start-colors:

>>+.tabs-alltabs-button {
>>+  padding: 0;
>>+  -moz-padding-start: 1px;
>>+  -moz-padding-end: 3px;
> I bet this is to compensate for the margin on the toolbarbutton-icon. Easier
> would be to remove the margin or hide the icon instead.
Fixed by hiding the icon.

> (But you might want to
> remove the margin on the scroll arrows for the same reason.)
Seems to be OK visually so not changing this.

>>+.alltabs-item[busy][_moz-menuactive="true"] {
>>+  -moz-image-region: rect(0px, 32px, 16px, 16px);
> This doesn't work, loading.gif is only 16x16
Oops. Fixed. Too much copying from Past Modern.

>>+    <resources>
>>+      <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>>+    </resources>
> I think we might have too many of these. When I was inspecting stuff in DOM
As per IRC Neil will ask bz for a definitive opinion.

> (In reply to comment #45)
>>> So what happens now when you drop a tab on itself?
>> As expected. Nothing.
> If you notice, the previous code wouldn't even have shown a drop indicator.
Fixed by reverting to the old code. Fixing this unmasked another bug in |onDragOver|.

        else if (!aDragSession.canDrop) {
should be
        if (!aDragSession.canDrop) {
Fixed.
Attachment #498122 - Attachment is obsolete: true
Attachment #498749 - Flags: review?(neil)
Attachment #498122 - Flags: review?(neil)
Comment on attachment 498749 [details] [diff] [review]
Patch v1.4 Modern Fixes, etc.

>+.alltabs-item[selected="true"] {
>+  font-weight: bold;
Would it be worth using [default="true"] which already includes the bold style?

(In reply to comment #47)
> (Or indeed, most of the rest of the styles too. I think the margin: 0px; is OK.)
As I recall, I was trying to include the hover and active styles too here. (I now notice that they include bottom border styles which will get ignored because of the -moz-bottom-border-color. Oops.)
(In reply to comment #49)
>>>+  border-right: 2px solid;
>>>+  -moz-border-right-colors: #000000 transparent;
>>I like this, in particular when a tab is partially hidden. However it looks bad
>>when the tabstrip is fully scrolled to the end. Conveniently you can avoid this
>>by switching to a black border end colour (thus also fixing it in RTL, right?)
>>as this is the automatically removed when the button is disabled.
>Switched to -moz-border-end-colors: and (for the down button)
>-moz-border-start-colors:
Hmm, so this turned out to be a little trickier than I thought. There are three choices:
1. Make this a 1px end border, and remove all the hover and active borders, meaning that only two of the borders respond to hover/activeness.
2. Restore the 2px -moz-border-right-colours, but override it when disabled as well as for hover and active, removing the other hover and active borders.
3. As 2. but override the bottom border for hover, active and disabled too, and also provide hover and open bottom border overrides for the menu widget.
(In reply to comment #49)
>>>+  border-right: 2px solid;
>>>+  -moz-border-right-colors: #000000 transparent;
>>I like this, in particular when a tab is partially hidden. However it looks bad
>>when the tabstrip is fully scrolled to the end. Conveniently you can avoid this
>>by switching to a black border end colour (thus also fixing it in RTL, right?)
>>as this is the automatically removed when the button is disabled.
>Switched to -moz-border-end-colors: and (for the down button)
>-moz-border-start-colors:
Oh, and unfortunately there's no such thing as -moz-border-end/start-colors, I was referring to the singular version for my 1px border comment above.
(In reply to comment #49)
> > (But you might want to
> > remove the margin on the scroll arrows for the same reason.)
> Seems to be OK visually so not changing this.
[FYI the buttons look left of centre to me.]
(In reply to comment #50)
>(From update of attachment 498749 [details] [diff] [review])
>>+.alltabs-item[selected="true"] {
>>+  font-weight: bold;
>Would it be worth using [default="true"] which already includes the bold style?
To answer my own question, there's a broadcast listener involved, so no.
>>+              this._fillTrailingGap();
>Why would a new tab create a trailing gap? (Come to think of it, where does
>closing a tab get handled? It's not readily apparent...)
To answer my own question again, it apparently doesn't need to get handled, and _fillTrailingGap appears to be completely unnecessary.
Hmm, so somehow putting the tabs in a scrollbox makes all the coordinates wrong by two, which is why we "fix" it by changing the negative margin.
Attached patch Simpler extract of dragover code (obsolete) — — Splinter Review 
To make the dragover code work it suffices to
a) switch from client to screen coordinates throughout
b) limit the coordinates to the containing scrollbox
Plus I added the pixelsToScroll check as the judder was noticeable.
[But I didn't add a comment so you might want to.]
Comment on attachment 498749 [details] [diff] [review]
Patch v1.4 Modern Fixes, etc.

>+          var tab = this.firstChild;
>+          tab.style.minWidth = this.mPrefs.getIntPref("browser.tabs.tabMinWidth") + "px";
>+          tab.style.maxWidth = this.mPrefs.getIntPref("browser.tabs.tabMaxWidth") + "px";
If we do this in the tabbrowser constructor, then we don't need mPrefs?

>+          window.addEventListener("resize", this, false);
Does this ever get removed?

Still haven't looked at Classic theme properly yet.
>>+.alltabs-item[selected="true"] {
>>+  font-weight: bold;
> Would it be worth using [default="true"] which already includes the bold style?
No.

> (In reply to comment #47)
>> (Or indeed, most of the rest of the styles too. I think the margin: 0px; is OK.)
> As I recall, I was trying to include the hover and active styles too here. (I
> now notice that they include bottom border styles which will get ignored
> because of the -moz-bottom-border-color. Oops.)
Fixed. I think.

> (In reply to comment #49)
>>>>+  border-right: 2px solid;
>>>>+  -moz-border-right-colors: #000000 transparent;
>>>I like this, in particular when a tab is partially hidden. However it looks bad
>>>when the tabstrip is fully scrolled to the end. Conveniently you can avoid this
>>>by switching to a black border end colour (thus also fixing it in RTL, right?)
>>>as this is the automatically removed when the button is disabled.
>>Switched to -moz-border-end-colors: and (for the down button)
>>-moz-border-start-colors:
> Hmm, so this turned out to be a little trickier than I thought. There are three
> choices:
> 1. Make this a 1px end border, and remove all the hover and active borders,
> meaning that only two of the borders respond to hover/activeness.
Option 1 seems the simplest so going for this. I've probably done this wrong somehow.

>>-moz-border-start-colors:
> Oh, and unfortunately there's no such thing as -moz-border-end/start-colors, I
> was referring to the singular version for my 1px border comment above.
OK. Fixed.

> (In reply to comment #49)
>> > (But you might want to
>> > remove the margin on the scroll arrows for the same reason.)
>> Seems to be OK visually so not changing this.
> [FYI the buttons look left of centre to me.]
Margins removed.

> neil@parkwaycc.co.uk      2010-12-24 05:16:27 PST
> 
> (In reply to comment #50)
>>(From update of attachment 498749 [details] [diff] [review])
>>>+.alltabs-item[selected="true"] {
>>>+  font-weight: bold;
>>Would it be worth using [default="true"] which already includes the bold style?
> To answer my own question, there's a broadcast listener involved, so no.
Yeah.

> neil@parkwaycc.co.uk 2010-12-24 14:46:13 PST
> 
>>>+              this._fillTrailingGap();
>>Why would a new tab create a trailing gap? (Come to think of it, where does
>>closing a tab get handled? It's not readily apparent...)
> To answer my own question again, it apparently doesn't need to get handled, and
> _fillTrailingGap appears to be completely unnecessary.
Removed.

> neil@parkwaycc.co.uk 2010-12-24 16:40:22 PST
> 
> Hmm, so somehow putting the tabs in a scrollbox makes all the coordinates wrong
> by two, which is why we "fix" it by changing the negative margin.
OK.

> neil@parkwaycc.co.uk 2010-12-24 16:48:58 PST

> Created attachment 499686 [details] [diff] [review]
> Simpler extract of dragover code

> To make the dragover code work it suffices to
> a) switch from client to screen coordinates throughout
> b) limit the coordinates to the containing scrollbox
Fixed. (Well copied. I don't pretend to understand drag'n'drop code at all).

> Plus I added the pixelsToScroll check as the judder was noticeable.
> [But I didn't add a comment so you might want to.]
Comment added.

> neil@parkwaycc.co.uk 2010-12-24 16:58:18 PST
> 
> Comment on attachment 498749 [details] [diff] [review]
> Patch v1.4 Modern Fixes, etc.

>>+          var tab = this.firstChild;
>>+          tab.style.minWidth = this.mPrefs.getIntPref("browser.tabs.tabMinWidth") + "px";
>>+          tab.style.maxWidth = this.mPrefs.getIntPref("browser.tabs.tabMaxWidth") + "px";
> If we do this in the tabbrowser constructor, then we don't need mPrefs?
I'll still need mPrefs eventually for the close buttons on tabs stuff e.g.
browser.tabs.closeButtons
browser.tabs.tabClipWidth

>>s+          window.addEventListener("resize", this, false);
> Does this ever get removed?
Uh oh. Fixed. (I don't see Firefox removing this I wonder why)

> Still haven't looked at Classic theme properly yet.
Now that's worrying.
Attachment #498749 - Attachment is obsolete: true
Attachment #499692 - Flags: review?(neil)
Attachment #498749 - Flags: review?(neil)
(In reply to comment #59)
> > Would it be worth using [default="true"] which already includes the bold style?
> No.
Hey, I already answered myself!

> > If we do this in the tabbrowser constructor, then we don't need mPrefs?
> I'll still need mPrefs eventually for the close buttons on tabs stuff e.g.
> browser.tabs.closeButtons
> browser.tabs.tabClipWidth
Makes sense I guess.

> > Does this ever get removed?
> Uh oh. Fixed. (I don't see Firefox removing this I wonder why)
It will get removed when the window is destroyed, so not vital ;-)

> > Still haven't looked at Classic theme properly yet.
> Now that's worrying.
Sorry for the delay, but I need to try it out in Luna as well.
Attachment #499692 - Attachment is patch: true
Attachment #499692 - Attachment mime type: application/octet-stream → text/plain
(From update of attachment 498749 [details] [diff] [review])
>+              // if we are scrolling, put the drop indicator at the edge
>+              // so that it doesn't jump while scrolling
This was the comment that I was thinking of.

(In reply to comment #60)
> (In reply to comment #59)
> > > Still haven't looked at Classic theme properly yet.
> > Now that's worrying.
> Sorry for the delay, but I need to try it out in Luna as well.
And in Luna I notice that the tab scroll arrows still look like toolbarbuttons, presumably because of their -moz-appearance, so you'll need to set a tab appearance (it does look a little odd but not as odd as a toolbarbutton or no appearance at all.) This also explains the blue active colour, since otherwise the scroll arrows get no active indication at all, which would be even worse.

Classic's menubutton could also do with being tweaked, but then I went back and compared with Modern and perhaps all tabbrowser toolbarbutton icon margins should be reset throughout. (This might also avoid having to use display: none; on the menubutton icon and text.) Interestingly in Classic the close button's icon margin is reset, although it doesn't work because it needs to be !important to override communicator. (Modern's margin is in toolbarbutton.css and therefore is automatically overriden by tabbrowser.css anyway.)

I've just noticed that session restore triggers the background tab animation.
Just noticed listAllTabs.label should be .tooltip instead.
Since Ratty isn't feeling too well, I've fixed the nits I mentioned... then I double-checked and found some related issues (tab appearance stops us from changing the background, so I had to fake the notification by switching to the selected attribute instead), and also fixed the Modern CSS to my satisfaction once and for all, because I got it wrong the last time.
Attachment #499686 - Attachment is obsolete: true
Attachment #499692 - Attachment is obsolete: true
Attachment #499777 - Flags: review?(iann_bugzilla)
Attachment #499777 - Flags: feedback?(philip.chee)
Attachment #499692 - Flags: review?(neil)
Comment on attachment 499777 [details] [diff] [review]
Double-checked for all my previous nits

> --- a/suite/themes/classic/mac/navigator/tabbrowser.css
> +++ b/suite/themes/classic/mac/navigator/tabbrowser.css
> +.scrollbutton-down[notifybgtab] {
I think you missed this bit.

> --- a/suite/themes/classic/navigator/tabbrowser.css
> +++ b/suite/themes/classic/navigator/tabbrowser.css
> +.scrollbutton-up,
> +.scrollbutton-down {
> +  -moz-appearance: tab;
> +  margin: 2px 0 1px;
If you are going to do 0->0px you might as well fix the margin as well.

Tested on Windows7/x64:

OS Classic Theme. The only thing that changes when a background tab is opened is that the bottom margin of the scrolldown button blinks for a fraction of a second.
OS Luna; Luna Aero; Aero Themes. The fake background notification using selected works but blink and you'd miss it. 
I think using -moz-appearance: tab causes more problems than it fixes :P .
Attachment #499777 - Flags: feedback?(philip.chee) → feedback-
(In reply to comment #64)
> I think using -moz-appearance: tab causes more problems than it fixes :P .
Previously the appearance was toolbarbutton which wasn't exactly helpful.
Attached patch With box-shadow (obsolete) — — Splinter Review 
I didn't like the lwtheme background colour override either, so that's gone.
Attachment #499777 - Attachment is obsolete: true
Attachment #499852 - Flags: review?(iann_bugzilla)
Attachment #499852 - Flags: feedback?(philip.chee)
Attachment #499777 - Flags: review?(iann_bugzilla)
I just spotted a nit which presumably exists in all versions of this patch, and that is that selecting the current tab from the all tabs dropdown doesn't make the current tab visible in the tab bar.
Attached patch With new imagery — — Splinter Review 
I finally worked out why the box-shadow wasn't doing what I expected. The arrow images that Firefox uses have a small "glow" effect which is normally unnoticeable but very obvious when the highlight is active.

I also change the way the CSS works because gnomestripe only allows normal and disabled appearance so I use the highlight to provide feedback instead.
Attachment #499852 - Attachment is obsolete: true
Attachment #499934 - Flags: review?(iann_bugzilla)
Attachment #499934 - Flags: feedback?(philip.chee)
Attachment #499852 - Flags: review?(iann_bugzilla)
Attachment #499852 - Flags: feedback?(philip.chee)
Attached patch Patch v1.6. Fix tab select. (obsolete) — — Splinter Review 
Tested on Win7 OS Classic Theme and OS Aero Themes.

> +@media all and (-moz-windows-theme: aero) {
> +  .scrollbutton-up,
> +  .scrollbutton-down {
> +    padding: 0px 2px;
> +  }
> +}

In Aero the scrollbuttons without any padding look completely cramped with no visual separation between the arrow graphic and the button margins. So I added some Aero specific padding.

> +.scrollbutton-up:not([disabled="true"]):hover:active,
> +.scrollbutton-down:not([disabled="true"]):hover:active,
> +.scrollbutton-down[notifybgtab="true"] {
> +  box-shadow: 0px 0px 0px 9px ThreeDShadow inset;
> +}


I found Highlight to be too dark. I wasn't sure if I could see the arrow graphic when the button was selected. I tried ThreeDShadow at random and this seems to be a better fit.

> neil@parkwaycc.co.uk      2010-12-27 16:21:38 PST
> 
> I just spotted a nit which presumably exists in all versions of this patch, and
> that is that selecting the current tab from the all tabs dropdown doesn't make
> the current tab visible in the tab bar.
Fixed.
Attachment #500046 - Flags: review?(neil)
Comment on attachment 499934 [details] [diff] [review]
With new imagery

Theme changes look OK except for |Highlight| looking too dark on the scrollbar buttons. See comment 69.
Attachment #499934 - Flags: review?(iann_bugzilla)
Attachment #499934 - Flags: feedback?(philip.chee)
Attachment #499934 - Flags: feedback+
Comment on attachment 500046 [details] [diff] [review]
Patch v1.6. Fix tab select.

Bah, I forgot to port all of my changes to the Mac theme :s
Attachment #500046 - Flags: review?(neil) → review+
> Bah, I forgot to port all of my changes to the Mac theme :s
Hope I did it correctly.
Attachment #500046 - Attachment is obsolete: true
Attachment #500062 - Flags: superreview?(neil)
Attachment #500062 - Flags: review+
Attachment #500062 - Attachment is patch: true
Attachment #500062 - Attachment mime type: application/octet-stream → text/plain
Attachment #500062 - Flags: superreview?(neil) → superreview+
(In reply to comment #72)
> Created attachment 500062 [details] [diff] [review]
> Patch v1.6a Port theme changes to Mac Classic r=IanN
> 
> > Bah, I forgot to port all of my changes to the Mac theme :s
> Hope I did it correctly.

+.scrollbutton-down {
+  border-left: 2px solid;
+  -moz-border-left-colors: rgba(0, 0, 0, 0.19) transparent;
+  list-style-image: url("chrome://messenger/skin/icons/tab-arrow-right.png");
+  -moz-image-region: rect(0, 7px, 11px, 0);
+  box-shadow: 0px 0px 0px 9px transparent inset;
+  -moz-transition: box-shadow 1s ease-out;
+}
+
+.scrollbutton-down:hover,
+.scrollbutton-down[notifybgtab="true"],
+.scrollbutton-down[disabled="true"] {
+  -moz-transition: none;
+}
+
+.scrollbutton-up:not([disabled="true"]):hover:active,
+.scrollbutton-down:not([disabled="true"]):hover:active,
+.scrollbutton-down[notifybgtab="true"] {
+  box-shadow: 0px 0px 0px 9px ThreeDShadow inset;
+}

I have no idea how this look and I can't test this atm, so I hope you tested it... Why add rules that makes it look different from tabmail, btw?
+.scrollbutton-up:not([disabled="true"]):hover:active,
+.scrollbutton-down:not([disabled="true"]):hover:active,
+.scrollbutton-down[notifybgtab="true"] {
+  box-shadow: 0px 0px 0px 9px ThreeDShadow inset;
+}

It's mostly the hover:active rules here that I'm wondering about. I don't know how ThreeDShadow looks, but it's possible that it works as an animation for the notifybgtab. But we don't use box-shadow on hover:active anywhere else, so I suspect this will not look good. The scrollbuttons are the same as in tabmail, so I'm also a bit concerned that the look now differs from tabmail.
(In reply to comment #69)
> I found Highlight to be too dark. I wasn't sure if I could see the arrow
> graphic when the button was selected. I tried ThreeDShadow at random and this
> seems to be a better fit.

Fwiw, on mac "Highlight" is the selection colour for controls and it follows what you have set in your system prefs - the default is aqua blue.
I'm on a Tiger machine atm, so I can't test the patch but I checked the ThreeDShadow colour on this machine and it's pretty similar to the background-color of the buttons (which is the same as for the tabs), so I suspect you won't notice much. We also already have other hover:active rules for the buttons which changes the bg color and I think that should be enough.
(In reply to comment #76)
> We also already have other hover:active rules
> for the buttons which changes the bg color and I think that should be enough.
Then why did you need box-shadow for the notify colour?

[Speaking of which, I guess you want 0 0 0 9px rather than 0px 0px 0px 9px...]
Pushed v1.6 to comm-central without the Mac Classic changes.
http://hg.mozilla.org/comm-central/rev/63e1b348e747

Leaving open for the time being while we sort out the Mac theme.
(In reply to comment #77)
> (In reply to comment #76)
> > We also already have other hover:active rules
> > for the buttons which changes the bg color and I think that should be enough.
> Then why did you need box-shadow for the notify colour?

Changing the bg colour didn't worked as expected in the first place, so I suggested box-shadow, see comment #36.

(In reply to comment #78)
> Pushed v1.6 to comm-central without the Mac Classic changes.
> http://hg.mozilla.org/comm-central/rev/63e1b348e747
+.scrollbutton-down[notifybgtab="true"] {
    +  background-color: Highlight;
Oops...

Anyway, I can have a look at this later on today or tomorrow.
Sorry about that Stefan, there were just too many versions of this patch floating around on my disk.
>       <handler event="popuphiding">
[....]
>             menuItem.removeEventListener("command", this, false);
Note to self: not needed.
(In reply to comment #80)
> Sorry about that Stefan, there were just too many versions of this patch
> floating around on my disk.

Don't worry about it :-)
(In reply to comment #79)
> (In reply to comment #77)
> > (In reply to comment #76)
> > > We also already have other hover:active rules
> > > for the buttons which changes the bg color and I think that should be enough.
> > Then why did you need box-shadow for the notify colour?
> Changing the bg colour didn't worked as expected in the first place, so I
> suggested box-shadow, see comment #36.
Ah, so in that case we should be copying the Modern rules, which also uses background-color for the active and notify styles.
(In reply to comment #83)
> (In reply to comment #79)
> > Changing the bg colour didn't worked as expected in the first place, so I
> > suggested box-shadow, see comment #36.
> Ah, so in that case we should be copying the Modern rules, which also uses
> background-color for the active and notify styles.

Yeah, except that we wanted a separate background-color (Highlight) for notify.
Attached patch Mac tweaks — — Splinter Review 
Using the background rules from Modern for consistency, although I left the colour as Highlight rather than, say, ThreeDShadow.
Attachment #500187 - Flags: review?(stefanh)
Comment on attachment 500187 [details] [diff] [review]
Mac tweaks

The toolbarbutton-text/icon changes are of course fine, but there is a problem here with the transitions - you get the animation effect when going from hover to normal and from hover:active to normal. The difference with Modern is that we have different background-colors for normal, hover, hover:active and notify (disabled is the same as normal) while Modern only sets (the same) background-color for hover:active and notify.
That said, I now start to think that styling the notify state the same as the hover:active state makes sense.
Comment on attachment 500187 [details] [diff] [review]
Mac tweaks

Per my previous comments and discussion with Neil, I think we can do like this:

 .scrollbutton-down {
   border-left: 2px solid;
   -moz-border-left-colors: rgba(0, 0, 0, 0.19) transparent;
   list-style-image: url("chrome://messenger/skin/icons/tab-arrow-right.png");
   -moz-image-region: rect(0, 7px, 11px, 0);
-  -moz-transition: box-shadow 1s ease-out;
+  -moz-transition: background-color 1s ease-out;

Keep the "-moz-transition: box-shadow 1s ease-out;".

+.scrollbutton-down:hover,
+.scrollbutton-down[notifybgtab="true"],
+.scrollbutton-down[disabled="true"] {
+  -moz-transition: none;
 }

Don't add these rules.

 .scrollbutton-down[notifybgtab="true"] {
   background-color: Highlight;
-  box-shadow: 0 0 9px 9px Highlight inset;
-  -moz-transition: none;
 }

Don't remove these, but change the colour. That is, make it to:
"box-shadow: 0 0 9px 9px rgba(0, 0, 0, 0.2) inset;
-moz-transition: none;"

r=me with those changes
Attachment #500187 - Flags: review?(stefanh) → review+
(In reply to comment #49)
>>>+    <resources>
>>>+      <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>>>+    </resources>
>>I think we might have too many of these. When I was inspecting stuff in DOM
>As per IRC Neil will ask bz for a definitive opinion.
So, because the tabs/buttons are anonymous content of tabbrowser then tabbrowser.css gets to override anything in tabbox/toolbarbutton.css that the inherited XBL pulls in anyway, without having to declare it explicitly.
>>       <handler event="popuphiding">
> [....]
>>             menuItem.removeEventListener("command", this, false);
> Note to self: not needed.
Removed.

> neil@parkwaycc.co.uk      2010-12-30 15:51:27 PST
>>>>+    <resources>
>>>>+      <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>>>>+    </resources>
>>>I think we might have too many of these. When I was inspecting stuff in DOM
>>As per IRC Neil will ask bz for a definitive opinion.
> So, because the tabs/buttons are anonymous content of tabbrowser then
> tabbrowser.css gets to override anything in tabbox/toolbarbutton.css that the
> inherited XBL pulls in anyway, without having to declare it explicitly.
Removed redundant stylesheet resources.
Attachment #500590 - Flags: review?(neil)
Comment on attachment 500590 [details] [diff] [review]
Patch Bv1.0 Minor code cleanup. [checked-in Comment 92]

>-    <resources>
>-      <stylesheet src="chrome://navigator/skin/tabbrowser.css"/>
>-    </resources>
> 
Kill the blank lines too.
Attachment #500590 - Flags: review?(neil) → review+
Comment on attachment 500590 [details] [diff] [review]
Patch Bv1.0 Minor code cleanup. [checked-in Comment 92]

> Kill the blank lines too.
Fixed.

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/77e0e0e1e1b6
Attachment #500590 - Attachment description: Patch Bv1.0 Minor code cleanup. → Patch Bv1.0 Minor code cleanup. [checked-in Comment 92]
Comment on attachment 500187 [details] [diff] [review]
Mac tweaks

Pushed changeset 8520d711286f to comm-central.
OK I think that's the last of the issues fixed. Anything else should go into followup bugs. Thanks to everyone for helping to get this into the tree.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → seamonkey2.1b2
I love this change!  But with this code now in place, can somebody indicate the userChrome.css option that will now allow you to set a tab width?  Prior to this, the following would work:

tabs>tab {width: 167px !important}

Now it's being ignored.  Is there a different parameter (for absolute width or minimum / maximum width) or is controlling this no longer possible?
(In reply to comment #95)
> tabs>tab {width: 167px !important}
> 
> Now it's being ignored.  Is there a different parameter (for absolute width or
> minimum / maximum width) or is controlling this no longer possible?

You need to use a combination of min-width and max-width now. Alternatively, just set browser.tabs.tabMinWidth (default 100) and browser.tabs.tabMaxWidth (default 250) accordingly.
Depends on: 643294
Depends on: 655412
Blocks: 673878
Depends on: 738615
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: