Last Comment Bug 484968 - Make SeaMonkey tab bar scrollable to cope with tab overflow
: Make SeaMonkey tab bar scrollable to cope with tab overflow
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- enhancement with 2 votes (vote)
: seamonkey2.1b2
Assigned To: Philip Chee
:
Mentors:
: 471388 471390 612295 (view as bug list)
Depends on: 643294 655412 738615
Blocks: SMtabAPI taboverflow 673878
  Show dependency treegraph
 
Reported: 2009-03-24 06:51 PDT by simon anderson
Modified: 2012-03-23 06:01 PDT (History)
14 users (show)
kairo: wanted‑seamonkey2.0-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
WIP Patch v0.1 (21.57 KB, patch)
2010-11-05 09:59 PDT, Philip Chee
no flags Details | Diff | Review
WIP Patch v0.2 now with jar.mn (24.46 KB, patch)
2010-11-05 11:17 PDT, Philip Chee
stefanh: ui‑review+
Details | Diff | Review
Patch v1.0 Tabs scrolling complete with AllTabs menubutton (36.84 KB, patch)
2010-11-14 11:40 PST, Philip Chee
no flags Details | Diff | Review
Screenshot of the arrowscrollbox, allTabs pulldown and lwtheme support. (285.31 KB, image/png)
2010-11-14 11:43 PST, Philip Chee
no flags Details
Patch v1.0a Tabs scrolling complete with AllTabs menubutton (36.91 KB, patch)
2010-11-17 19:29 PST, Philip Chee
iann_bugzilla: review-
mnyromyr: feedback+
Details | Diff | Review
Patch v1.1 Fixed Drag'n'Drop and tab scrolling. (50.50 KB, patch)
2010-11-27 08:26 PST, Philip Chee
stefanh: ui‑review-
Details | Diff | Review
Patch v1.2 Fix Mac UI issues. (50.51 KB, patch)
2010-12-11 08:46 PST, Philip Chee
no flags Details | Diff | Review
Patch v1.2 Really Fix Mac UI issues. (50.52 KB, patch)
2010-12-13 03:06 PST, Philip Chee
iann_bugzilla: review+
stefanh: ui‑review+
Details | Diff | Review
Patch v1.3 fixed review issues to date. (48.84 KB, patch)
2010-12-16 08:46 PST, Philip Chee
no flags Details | Diff | Review
Patch v1.4 Modern Fixes, etc. (47.68 KB, patch)
2010-12-20 08:42 PST, Philip Chee
no flags Details | Diff | Review
Simpler extract of dragover code (2.19 KB, patch)
2010-12-24 16:48 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Patch v1.5 Fix nits to date (Modern, dragover, etc) (45.26 KB, patch)
2010-12-24 20:08 PST, Philip Chee
no flags Details | Diff | Review
Double-checked for all my previous nits (44.83 KB, patch)
2010-12-26 13:29 PST, neil@parkwaycc.co.uk
philip.chee: feedback-
Details | Diff | Review
With box-shadow (45.67 KB, patch)
2010-12-27 10:36 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Review
With new imagery (45.03 KB, patch)
2010-12-27 16:54 PST, neil@parkwaycc.co.uk
philip.chee: feedback+
Details | Diff | Review
Patch v1.6. Fix tab select. (45.61 KB, patch)
2010-12-28 08:09 PST, Philip Chee
neil: review+
Details | Diff | Review
Patch v1.6a Port theme changes to Mac Classic r=IanN (46.31 KB, patch)
2010-12-28 11:03 PST, Philip Chee
philip.chee: review+
neil: superreview+
Details | Diff | Review
Mac tweaks (2.17 KB, patch)
2010-12-29 06:25 PST, neil@parkwaycc.co.uk
stefanh: review+
Details | Diff | Review
Patch Bv1.0 Minor code cleanup. [checked-in Comment 92] (3.04 KB, patch)
2011-01-01 07:47 PST, Philip Chee
neil: review+
Details | Diff | Review

Description simon anderson 2009-03-24 06:51:30 PDT
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.
Comment 1 Robert Kaiser (not working on stability any more) 2009-09-17 17:48:48 PDT
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.
Comment 2 neil@parkwaycc.co.uk 2009-09-21 05:56:13 PDT
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.)
Comment 3 Tony Mechelynck [:tonymec] 2009-12-23 11:48:59 PST
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
Comment 4 Robert Kaiser (not working on stability any more) 2010-04-27 08:00:37 PDT
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.
Comment 5 Philip Chee 2010-11-05 09:59:50 PDT
Created attachment 488491 [details] [diff] [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] 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.
Comment 6 Philip Chee 2010-11-05 10:02:18 PDT
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.
Comment 7 Philip Chee 2010-11-05 10:18:39 PDT
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");
> }
Comment 8 Philip Chee 2010-11-05 10:21:14 PDT
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 9 Philip Chee 2010-11-05 10:59:12 PDT
Comment on attachment 488491 [details] [diff] [review]
WIP Patch v0.1

Bah forgot the jar.mn changes.
Comment 10 Philip Chee 2010-11-05 11:17:23 PDT
Created attachment 488513 [details] [diff] [review]
WIP Patch v0.2 now with jar.mn

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]
Comment 11 Philip Chee 2010-11-05 11:21:29 PDT
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.
Comment 12 Stefan [:stefanh] (away until May 28) 2010-11-05 15:35:02 PDT
(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.
Comment 13 simon anderson 2010-11-08 01:42:58 PST
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?
Comment 14 Philip Chee 2010-11-08 04:26:36 PST
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 15 Stefan [:stefanh] (away until May 28) 2010-11-08 14:35:12 PST
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).
Comment 16 Philip Chee 2010-11-08 20:21:08 PST
> 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.
Comment 17 Bruno 'Aqualon' Escherl 2010-11-09 06:04:22 PST
As a short remark before I forget about it, the patch contains some CR line endings in suite/browser/navigator.css
Comment 18 Philip Chee 2010-11-09 08:57:05 PST
I think you meant tabbrowser.xml couldn't find any DOS newlines in navigator.css.
Comment 19 Bruno 'Aqualon' Escherl 2010-11-09 09:06:41 PST
(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.
Comment 20 Philip Chee 2010-11-14 11:40:27 PST
Created attachment 490468 [details] [diff] [review]
Patch v1.0 Tabs scrolling complete with AllTabs menubutton

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.
Comment 21 Philip Chee 2010-11-14 11:43:54 PST
Created attachment 490472 [details]
Screenshot of the arrowscrollbox, allTabs pulldown and lwtheme support.
Comment 22 Philip Chee 2010-11-17 19:29:26 PST
Created attachment 491436 [details] [diff] [review]
Patch v1.0a Tabs scrolling complete with AllTabs menubutton

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.
>
Comment 23 Philip Chee 2010-11-17 19:30:52 PST
> 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 24 Philip Chee 2010-11-18 06:31:36 PST
*** Bug 612295 has been marked as a duplicate of this bug. ***
Comment 25 Philip Chee 2010-11-20 20:57:41 PST
*** Bug 471388 has been marked as a duplicate of this bug. ***
Comment 26 Philip Chee 2010-11-20 20:58:14 PST
*** Bug 471390 has been marked as a duplicate of this bug. ***
Comment 27 Ian Neal 2010-11-21 14:28:57 PST
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-
Comment 28 Philip Chee 2010-11-22 00:34:25 PST
> 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?
Comment 29 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.
> 
> > 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 30 Karsten Düsterloh 2010-11-22 14:51:33 PST
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.
Comment 31 Stefan [:stefanh] (away until May 28) 2010-11-23 00:37:53 PST
(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
Comment 32 Robert Kaiser (not working on stability any more) 2010-11-23 04:44:21 PST
(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 ;-)
Comment 33 Karsten Düsterloh 2010-11-23 05:15:52 PST
(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...
Comment 34 Philip Chee 2010-11-27 08:26:04 PST
Created attachment 493521 [details] [diff] [review]
Patch v1.1 Fixed Drag'n'Drop and tab scrolling.

> 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.
Comment 35 Philip Chee 2010-11-27 08:28:54 PST
Comment on attachment 493521 [details] [diff] [review]
Patch v1.1 Fixed Drag'n'Drop and tab scrolling.

Stefan. Please review the Mac UI. Thanks.
Comment 36 Stefan [:stefanh] (away until May 28) 2010-11-28 11:16:30 PST
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.
Comment 37 Philip Chee 2010-12-11 08:46:23 PST
Created attachment 497068 [details] [diff] [review]
Patch v1.2 Fix Mac UI issues.

> 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).
Comment 38 Philip Chee 2010-12-11 08:47:42 PST
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;
Comment 39 Stefan [:stefanh] (away until May 28) 2010-12-11 16:38:27 PST
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?
Comment 40 Philip Chee 2010-12-12 10:35:21 PST
yeah I think you're right. I'll do it the tabmail way.
Comment 41 Philip Chee 2010-12-13 03:06:47 PST
Created attachment 497238 [details] [diff] [review]
Patch v1.2 Really Fix Mac UI issues.

> 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.
Comment 42 Stefan [:stefanh] (away until May 28) 2010-12-13 08:17:48 PST
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.
Comment 43 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?

>+              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 44 Ian Neal 2010-12-14 14:37:02 PST
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).
Comment 45 Philip Chee 2010-12-16 08:46:18 PST
Created attachment 498122 [details] [diff] [review]
Patch v1.3 fixed review issues to date.

> 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.
Comment 46 neil@parkwaycc.co.uk 2010-12-17 05:15:50 PST
Ironically the margin was there to simplify the code...
Comment 47 neil@parkwaycc.co.uk 2010-12-18 16:26:52 PST
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 48 neil@parkwaycc.co.uk 2010-12-19 16:20:33 PST
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.
Comment 49 Philip Chee 2010-12-20 08:42:26 PST
Created attachment 498749 [details] [diff] [review]
Patch v1.4 Modern Fixes, etc.

> 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.
Comment 50 neil@parkwaycc.co.uk 2010-12-20 09:17:23 PST
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.)
Comment 51 neil@parkwaycc.co.uk 2010-12-22 13:41:48 PST
(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.
Comment 52 neil@parkwaycc.co.uk 2010-12-22 13:47:32 PST
(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.
Comment 53 neil@parkwaycc.co.uk 2010-12-22 13:51:13 PST
(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.]
Comment 54 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.
Comment 55 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.
Comment 56 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.
Comment 57 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
Plus I added the pixelsToScroll check as the judder was noticeable.
[But I didn't add a comment so you might want to.]
Comment 58 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?

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

Still haven't looked at Classic theme properly yet.
Comment 59 Philip Chee 2010-12-24 20:08:01 PST
Created attachment 499692 [details] [diff] [review]
Patch v1.5 Fix nits to date (Modern, dragover, etc)

>>+.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.
Comment 60 neil@parkwaycc.co.uk 2010-12-25 04:47:47 PST
(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.
Comment 61 neil@parkwaycc.co.uk 2010-12-26 04:48:16 PST
(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.
Comment 62 neil@parkwaycc.co.uk 2010-12-26 09:19:40 PST
Just noticed listAllTabs.label should be .tooltip instead.
Comment 63 neil@parkwaycc.co.uk 2010-12-26 13:29:47 PST
Created attachment 499777 [details] [diff] [review]
Double-checked for all my previous nits

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.
Comment 64 Philip Chee 2010-12-26 20:31:05 PST
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 .
Comment 65 neil@parkwaycc.co.uk 2010-12-27 05:08:56 PST
(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.
Comment 66 neil@parkwaycc.co.uk 2010-12-27 10:36:29 PST
Created attachment 499852 [details] [diff] [review]
With box-shadow

I didn't like the lwtheme background colour override either, so that's gone.
Comment 67 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.
Comment 68 neil@parkwaycc.co.uk 2010-12-27 16:54:37 PST
Created attachment 499934 [details] [diff] [review]
With new imagery

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.
Comment 69 Philip Chee 2010-12-28 08:09:37 PST
Created attachment 500046 [details] [diff] [review]
Patch v1.6. Fix tab select.

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.
Comment 70 Philip Chee 2010-12-28 08:13:01 PST
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.
Comment 71 neil@parkwaycc.co.uk 2010-12-28 08:44:42 PST
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
Comment 72 Philip Chee 2010-12-28 11:03:44 PST
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.
Comment 73 Stefan [:stefanh] (away until May 28) 2010-12-28 12:07:41 PST
(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?
Comment 74 Stefan [:stefanh] (away until May 28) 2010-12-28 12:28:11 PST
+.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.
Comment 75 Stefan [:stefanh] (away until May 28) 2010-12-28 12:49:07 PST
(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.
Comment 76 Stefan [:stefanh] (away until May 28) 2010-12-28 13:16:31 PST
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.
Comment 77 neil@parkwaycc.co.uk 2010-12-28 17:08:23 PST
(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...]
Comment 78 Philip Chee 2010-12-28 19:06:54 PST
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.
Comment 79 Stefan [:stefanh] (away until May 28) 2010-12-29 00:07:48 PST
(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.
Comment 80 Philip Chee 2010-12-29 01:44:18 PST
Sorry about that Stefan, there were just too many versions of this patch floating around on my disk.
Comment 81 Philip Chee 2010-12-29 02:15:20 PST
>       <handler event="popuphiding">
[....]
>             menuItem.removeEventListener("command", this, false);
Note to self: not needed.
Comment 82 Stefan [:stefanh] (away until May 28) 2010-12-29 02:38:11 PST
(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 :-)
Comment 83 neil@parkwaycc.co.uk 2010-12-29 02:42:03 PST
(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.
Comment 84 Stefan [:stefanh] (away until May 28) 2010-12-29 06:11:48 PST
(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.
Comment 85 neil@parkwaycc.co.uk 2010-12-29 06:25:45 PST
Created attachment 500187 [details] [diff] [review]
Mac tweaks

Using the background rules from Modern for consistency, although I left the colour as Highlight rather than, say, ThreeDShadow.
Comment 86 Stefan [:stefanh] (away until May 28) 2010-12-29 12:47:41 PST
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.
Comment 87 Stefan [:stefanh] (away until May 28) 2010-12-29 13:05:35 PST
That said, I now start to think that styling the notify state the same as the hover:active state makes sense.
Comment 88 Stefan [:stefanh] (away until May 28) 2010-12-29 13:58:47 PST
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
Comment 89 neil@parkwaycc.co.uk 2010-12-30 15:51:27 PST
(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.
Comment 90 Philip Chee 2011-01-01 07:47:10 PST
Created attachment 500590 [details] [diff] [review]
Patch Bv1.0 Minor code cleanup. [checked-in Comment 92]

>>       <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.
Comment 91 neil@parkwaycc.co.uk 2011-01-01 13:13:46 PST
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.
Comment 92 Philip Chee 2011-01-01 20:13:04 PST
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
Comment 93 neil@parkwaycc.co.uk 2011-01-02 09:00:43 PST
Comment on attachment 500187 [details] [diff] [review]
Mac tweaks

Pushed changeset 8520d711286f to comm-central.
Comment 94 Philip Chee 2011-01-02 09:33:35 PST
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.
Comment 95 Jason Bassford 2011-01-04 07:35:32 PST
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?
Comment 96 Jens Hatlak (:InvisibleSmiley) 2011-01-04 08:09:08 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.