465086 - When closing a tab, other tabs should not resize until cursor leaves toolbar region

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Matt Winkelmann]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4

When I close a tab, the other tabs immediately resize. I have to move the cursor if I want to close further tabs. It'd be a lot easier to close multiple tabs if the resizing were delayed for a few seconds or until the cursor leaves the tabs toolbar. The latter behaviour is used by google's chrome.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ian Pottinger]

See Google Chrome for an excellent implemetation of this behaviour.

Comment 2

[Ben Lerner]

Seconded -- when using Chrome, closing multiple tabs with the mouse Just Worked, and it took me a second to realize why, since intuitively I knew that the tabs were resizing on the tab bar... Delaying the resize until after the mouse leaves the chrome area at top (which is more general than just the tabstrip, to account for people's hands not moving perfectly horizontally as they try to get from tab to tab) would be a very nice piece of polish.

Comment 3

[sakar222]

i was desperately looking to solution of this issue and found this Bug 465086 which is exactly fits in my problem. (using Firefox 3.5.6)

i agree up posts' Chrome solution.
When closing multiple tabs, tab widhts' dynamically resizing should be delayed for a moment like Google Chrome behaviour.

Otherwise, if you are closing lots of tabs you should change mouse position time to time which is very waste of time, and annoying.
-I Hope, you take up this as a bug and find solution, regards.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

CC'ing Alex from the UX team who could give us possible solutions.

Comment 5

[eyal gruss]

note that there is also an advantage in the tabs resizing: if you stand on the last tab close button and start clicking, you can close many tabs without moving the cursor at all

Comment 6

[Ben Lerner]

@comment 5: True, for a while.  but consider the following way of looking at things; maybe it'll help split "use cases" from "I like this shiny better than that shiny" and help guide the UX folks...  there are three "stages" to tab crowding:

1) a few tabs (for me, 6) where all tabs are maximum size and there's leftover space on the tab bar
2) several (for me, 7--16) where the tab bar is full and the tabs are resizing
3) many (for me, 17+ ascending, 16+ descending) where the tab bar is full and the overflow buttons appear

Case 3, no one is arguing that tabs need to resize (the bar is full anyway).

Having the tabs resize under you helps solve case 2, as long as you're a) always closing tabs in order from the rightmost to the leftmost, and b) you don't go below that 6-tab limit (tabs can't resize any wider without looking weird).

Chrome's approach lets you slide smoothly between cases 2 and 1: the tabs stay a consistent (narrow) width as you close each tab, even if you drop below the 6-tab minimum limit.  Then when you leave the tabbar, they expand (up to a maximum width) to fill the available space.

I think the appeal of Chrome's approach isn't so much that you can "close many tabs without moving the cursor at all", but that you can consistently plan for how wide all the tabs are as you're closing them, so that even if you need to close non-adjacent, rightmost tabs, you can guesstimate via muscle memory how far you need to move the mouse to do so.

Comment 7

[Frank Yan (:fryn)]

This could go along with all the tab creation / D&D animation work for Firefox 4. UX people? 

Plus, we have CSS transitions that could potentially help with this, but I'm not sure if XUL flex and width/min-width play nice together with CSS.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

cc'ing Dao since I think he has been looking into potential tab animations as well.

Comment 11

[Michael Monreal [:micmon]]

What about increasing priority to get this in ASAP (for 4.0 if possible)? Almost everyone I talked to who tried Chrome absolutely loved this feature. For me, managing tabs in Fx suddenly feelt totally wrong after trying Chrome for a few minutes....

Comment 12

[Dave Garrett]

(In reply to comment #11)
It's already filed under the Firefox paper cuts project, so it's on the radar.

This is a common UI gripe. Requesting betaN blocking, if that's doable here.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #11)
> What about increasing priority to get this in ASAP (for 4.0 if possible)?

How about you attach a patch?

Comment 14

[Alex Faaborg [:faaborg] (Firefox UX)]

Clearing the uiwanted keyword, but add it back if there any follow up questions.  In terms of priority, yeah I definitely think this should block the final release.

Comment 15

[Frank Yan (:fryn)]

Attachment: patch v1

Addresses non-overflow and overflow cases.

Help wanted with robustly detecting when the mouse cursor leaves the tab toolbar.

Comment 16

[Michael Monreal [:micmon]]

Something to consider: should we show the tab close icon longer (always?). Maybe we could also decrease the minimal tab size a bit.

Comment 17

[Frank Yan (:fryn)]

Attachment: patch v2

If the patch for bug 380960 lands before this one, it will require a bit of additional code to integrate the closing animation when in overflow mode.

Comment 19

[Michael Monreal [:micmon]]

(In reply to comment #17)
> patch v2

This seems to work quite well already. There is one bug in overflow mode:

- add a lot of tabs, trigger overflow
- move all the way to the right (the ">" button should be grayed out)
- now start removing tabs on the left
- the ">" will turn clickable and after removing another tab, the tabs will expand

Comment 20

[Frank Yan (:fryn)]

Attachment: patch v3 (better precision, reversion timing, & cleanup)

Comment 21

[Johnathan Nightingale [:johnath]]

Doesn't block, but I want it bad. Get it reviewed and ask for approval. You will get it.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 459910 [details] [diff] [review]
patch v3 (better precision, reversion timing, & cleanup)

>       <method name="removeTab">
>         <parameter name="aTab"/>
>+        <parameter name="aInitiatedByMouse"/>

The second parameter should be an object (see patch in bug 380960).

>+      <field name="_overflowSpacer">
>+        document.getAnonymousElementByAttribute(this, "anonid", "tab-overflow-spacer");
>+      </field>
>+
>+      <field name="_needsResize">false</field>
>+      <field name="_needsShift">false</field>
>+
>+      <method name="_animateClose">

>+      <method name="_animateAdjust">

These need better names. (E.g. the spacer isn't just about overflow, it's unclear what needs resizing, the methods don't animate anything, etc.)

>+              if (document.getElementById("tabContextMenu").state != "open" &&

What's the point of this?

I saw 250 a couple of times in this patch. You shouldn't hardcode this.

I'm not sure what this patch tries to do in overflow mode when closing a tab that's not the last tab -- looks broken.

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> I'm not sure what this patch tries to do in overflow mode when closing a tab
> that's not the last tab -- looks broken.

As a start maybe we should only support this when the tab bar doesn't overflow.

Comment 24

[Frank Yan (:fryn)]

(In reply to comment #22)
> >+              if (document.getElementById("tabContextMenu").state != "open" &&
> 
> What's the point of this?

To make sure that, while the context menu is still open, the context tab doesn't move.

> I saw 250 a couple of times in this patch. You shouldn't hardcode this.
> 
> I'm not sure what this patch tries to do in overflow mode when closing a tab
> that's not the last tab -- looks broken.

How do I get the default max-width of the tabs?
If a tab is initialized as a pinned tab, I don't think getComputedStyle(tab, null).width will work.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> > >+              if (document.getElementById("tabContextMenu").state != "open" &&
> > 
> > What's the point of this?
> 
> To make sure that, while the context menu is still open, the context tab
> doesn't move.

Why would the context menu be open at that point?

> > I saw 250 a couple of times in this patch. You shouldn't hardcode this.
> > 
> > I'm not sure what this patch tries to do in overflow mode when closing a tab
> > that's not the last tab -- looks broken.
> 
> How do I get the default max-width of the tabs?
> If a tab is initialized as a pinned tab, I don't think getComputedStyle(tab,
> null).width will work.

Use the first tab that isn't pinned?
(also, s/width/maxWidth/)

Comment 26

[Frank Yan (:fryn)]

(In reply to comment #25)
> (In reply to comment #24)
> > > >+              if (document.getElementById("tabContextMenu").state != "open" &&
> > > 
> > > What's the point of this?
> > 
> > To make sure that, while the context menu is still open, the context tab
> > doesn't move.
> 
> Why would the context menu be open at that point?

Have a bunch of tabs open, close a couple with the mouse to trigger the smart resizing, right-click a tab.
While hovering the mouse over the context menu, the mouse may leave the tab bar region, so we need to prevent the tab sizes from reverting until the context menu is closed.

Comment 27

[Frank Yan (:fryn)]

Attachment: patch v4

I renamed overflowSpacer to tabTempSpacer. Let me know if you have better suggestions.

I also adjusted the transition-duration of the spacer to .25ms to match your tab closing animation.

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #17)
> If the patch for bug 380960 lands before this one, it will require a bit of
> additional code to integrate the closing animation when in overflow mode.

How do you think the integration would look like?

Comment 29

[Frank Yan (:fryn)]

(In reply to comment #28)
> (In reply to comment #17)
> > If the patch for bug 380960 lands before this one, it will require a bit of
> > additional code to integrate the closing animation when in overflow mode.
> 
> How do you think the integration would look like?

Actually, I didn't realize that bug 380960 doesn't handle overflow mode yet, so no additional code will be required. I can change the transition-duration of .25s back to .15s if it looks better.

Comment 30

[Dão Gottwald [:dao]]

(In reply to comment #29)
> Actually, I didn't realize that bug 380960 doesn't handle overflow mode yet

I'm not sure what this means...

Comment 31

[Frank Yan (:fryn)]

(In reply to comment #30)
> (In reply to comment #29)
> > Actually, I didn't realize that bug 380960 doesn't handle overflow mode yet
> 
> I'm not sure what this means...

Oh, my bad. I'd been failing at reading code all day.
AFAIK, the integration will be modifying the spacer to grow smoothly and in sync with the closing tab animation.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'd love to get this into beta 4.

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 461305 [details] [diff] [review]
patch v4

>+.tab-temp-spacer {

closing-tabs-spacer? Seems still suboptimal, but maybe slightly better than tab-temp-spacer.

>       <method name="removeTab">
>         <parameter name="aTab"/>
>+        <parameter name="aParams"/>
>         <body>
>           <![CDATA[
>-            this._endRemoveTab(this._beginRemoveTab(aTab, false, null, true));
>+            this._endRemoveTab(this._beginRemoveTab(aTab, false, null, true),
>+                               aParams && aParams.mouse);

Instead of having this argument, I wonder whether we should track whether the mouse is over the tab strip, and preserve the space when a tab is closed by any means while the mouse is over the tab strip. Would it make sense user-experience wise? Would it simplify code?

>+            case "mousemove":
>+              // Readjust tabs when the mouse leaves the tab container
>+              if (document.getElementById("tabContextMenu").state != "open" &&
>+                  (aEvent.screenX < this.boxObject.screenX ||
>+                   aEvent.screenX > this.boxObject.screenX + this.boxObject.width ||
>+                   aEvent.screenY < this.boxObject.screenY ||
>+                   aEvent.screenY > this.boxObject.screenY + this.boxObject.height)) {
>+                window.removeEventListener("mousemove", this, false);
>+                this._revertTabSizing();

I suppose listening to the mouseout event on the tab container doesn't quite work here?

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

I would also love to get this in beta 4, but can't see it as a blocker, just a really-really-want. Looks like it's on the right path, and I'll happily approve it.

Comment 36

[Frank Yan (:fryn)]

Attachment: patch v5 (updated to tip of tree)

(In reply to comment #34)
> closing-tabs-spacer? Seems still suboptimal, but maybe slightly better than
> tab-temp-spacer.

I renamed it to closing-tabs-spacer.

> >       <method name="removeTab">
> >         <parameter name="aTab"/>
> >+        <parameter name="aParams"/>
> >         <body>
> >           <![CDATA[
> >-            this._endRemoveTab(this._beginRemoveTab(aTab, false, null, true));
> >+            this._endRemoveTab(this._beginRemoveTab(aTab, false, null, true),
> >+                               aParams && aParams.mouse);
> 
> Instead of having this argument, I wonder whether we should track whether the
> mouse is over the tab strip, and preserve the space when a tab is closed by any
> means while the mouse is over the tab strip. Would it make sense
> user-experience wise? Would it simplify code?

I don't think it makes much sense user-experience wise, since using keyboard shortcuts to close tabs means that the user probably doesn't care about the tab / close button position.
It also wouldn't simplify code, since checking the hover state takes several more lines.

> 
> >+            case "mousemove":
> >+              // Readjust tabs when the mouse leaves the tab container
> >+              if (document.getElementById("tabContextMenu").state != "open" &&
> >+                  (aEvent.screenX < this.boxObject.screenX ||
> >+                   aEvent.screenX > this.boxObject.screenX + this.boxObject.width ||
> >+                   aEvent.screenY < this.boxObject.screenY ||
> >+                   aEvent.screenY > this.boxObject.screenY + this.boxObject.height)) {
> >+                window.removeEventListener("mousemove", this, false);
> >+                this._revertTabSizing();
> 
> I suppose listening to the mouseout event on the tab container doesn't quite
> work here?

It didn't work for me. I logged events, and mouseout was called on different nodes, depending on state and direction of mouse movement. This method was reliable.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 465096 [details] [diff] [review]
patch v5 (updated to tip of tree)

ui-r=beltzner, fryn and I went over this in person today

Comment 38

[Dão Gottwald [:dao]]

Comment on attachment 465096 [details] [diff] [review]
patch v5 (updated to tip of tree)

>           <![CDATA[
>             var isLastTab = (this.tabs.length - this._removingTabs.length == 1);
>             if (aParams)
>               var animate = aParams.animate;
> 
>             if (!this._beginRemoveTab(aTab, false, null, true))
>               return;
> 
>+            if (!aTab.pinned && aParams && aParams.mouse)
>+              this.tabContainer._resizeTabsOnClose(aTab);

Rename 'mouse' to 'viaMouse', 'byMouse' or something along these lines. Assign it to a local variable at the top of the method so that it's clearer which parameters are supported. (While you're at it, please move isLastTab below that section dealing with aParams.)

>+            var wasPinned = aTab.pinned;
>+
>             // update the UI early for responsiveness
>             aTab.collapsed = true;
>             if (aNewTab)
>               this.addTab("about:blank", {skipAnimation: true});
>             this.tabContainer._fillTrailingGap();
>             this._blurTab(aTab);
> 
>             this._removingTabs.splice(this._removingTabs.indexOf(aTab), 1);
>@@ -1489,18 +1497,16 @@
>             // cleanup ourselves.
>             // This has to happen before we remove the child so that the
>             // XBL implementation of nsIObserver still works.
>             browser.destroy();
> 
>             if (browser == this.mCurrentBrowser)
>               this.mCurrentBrowser = null;
> 
>-            var wasPinned = aTab.pinned;
>-

What's this about?

>+      <method name="_revertTabSizing">
>+        <body><![CDATA[
>+          if (this._hasTabTempMaxWidth) {
>+            this._hasTabTempMaxWidth = false;
>+            for (let i = 0; i < this.childNodes.length; i++)
>+              this.childNodes[i].style.maxWidth = this._tabDefaultMaxWidth ?
>+                this._tabDefaultMaxWidth + "px" : "";

Why are you messing with this._tabDefaultMaxWidth here rather than always resetting the maxWidth?

Looks like everything dealing with childNodes should use visibleTabs instead.

Closing tabs other than the last one in overflow mode still seems weird, making tabs scroll randomly to the left or to the right.

Comment 39

[Frank Yan (:fryn)]

Attachment: patch v6 [ui-r=beltzner] (updated as per dao's comment #38)

Comment 40

[Michael Monreal [:micmon]]

Some small problems I found:

a) add as many tabs as needed to fill the whole tab bar (tabs shrink a bit). Now close the second tab. The first tab first resizes to a larger size, then shrinks back. It should just keep its old size in this case.

b) again, fill the tab bar and add 2-3 more tabs (overflow still not on). Now, close the second to last tab. The last tab will move in its place. Close it as well. Now, the tabs from the front expand but they expand too far: the mouse ends up in the middel of the tab, not hovering the close button.

c) add a lot of tabs (overflow needs to be active). Close a tab in the middle of the tab bar and don't move the mouse. The tab behind it will move in place correctly. Close this tab as well. Now, the tabs in front will move back. If you click again now you will close the tab before the previously closed one.

Comment 41

[Frank Yan (:fryn)]

Attachment: patch v7 [ui-r=beltzner] (updated to work with gBrowser.visibleTabs)

I was not aware that gBrowser.visibleTabs excluded the closing tabs. This version takes that into account, so the issues in comment #39 should now be resolved.

Comment 42

[Michael Monreal [:micmon]]

Problem "b" is fixed, "a" and "c" still happen.

Additionally:

d) open as much tabs as it needs to trigger overflow. Close the second to last tab. The last tab now moves in its place. Close this one, too. The tabs move to the right now and stop at the correct position (x below mouse pointer). Click again, tab closes and now all tabs move to the right to fill all the space.

Comment 43

[Frank Yan (:fryn)]

(In reply to comment #40)

I don't see problems a and c at all. What OS are you using? Could you make a screencast of this happening?

(In reply to comment #42)
> Click again, tab closes and now all tabs move to the right to fill all the space.

This occurs when the scroll buttons are removed (underflow triggered), no? If that is the case, it is a known issue that will be addressed in a followup bug.

Comment 44

[Michael Monreal [:micmon]]

(In reply to comment #43)
> I don't see problems a and c at all. What OS are you using? 
Linux

> Could you make a screencast of this happening?
I'll link short screencasts for issue a, c, d in a minute

Comment 45

[Michael Monreal [:micmon]]

Here they are:

a) http://dl.dropbox.com/u/114398/tab-a.ogv
c) http://dl.dropbox.com/u/114398/tab-c.ogv
d) http://dl.dropbox.com/u/114398/tab-d.ogv

Comment 46

[Frank Yan (:fryn)]

Comment on attachment 469441 [details] [diff] [review]
patch v7 [ui-r=beltzner] (updated to work with gBrowser.visibleTabs)

(In reply to comment #45)

Ugh, I suppose I'll need to set up a Linux VM to look into this.
Platform-specific problems OTL

Comment 47

[Michael Monreal [:micmon]]

Is this feature in danger yet because of upcoming freezes?

Comment 48

[imradyurrad]

(In reply to comment #47)
> Is this feature in danger yet because of upcoming freezes?


-it's not blocking
-it doesn't have a reviewed patch yet
-code freeze is supposed to be Sept. 10th (tomorrow)

I'm assuming this won't make it into 4.0 
Maybe 4.1...or 4.5, who knows?

Comment 49

[Dave Garrett]

There's no strings; it's a bugfix more than a feature. It's already been UI reviewed, too. I think the rules would allow it into a later beta.

Comment 50

[Frank Yan (:fryn)]

Attachment: patch v8 [ui-r=beltzner] (attempted fix for linux)

I call tab.clientTop in an attempt to force style resolution.

Michael, could you please test this?
Issue "d" will still exist (leaving that for a followup), but "a" and "c" will hopefully be fixed.

Comment 51

[Michael Monreal [:micmon]]

(In reply to comment #50)
> Michael, could you please test this?
> Issue "d" will still exist (leaving that for a followup), but "a" and "c" will
> hopefully be fixed.

I can confirm that issue a and c seem to be fixed now. Generally, the experience seems to be very good now, only the translation from overflow to no overflow is still bugged (issue d). Worth checking in now imho.

Comment 52

[Frank Yan (:fryn)]

Comment on attachment 474459 [details] [diff] [review]
patch v8 [ui-r=beltzner] (attempted fix for linux)

Michael, as I wrote before, issue "d" will be resolved in the followup bug 595694 as soon as I have time. It's more troublesome to fix that the others, since it seems that it may require a change in logic to the underflow calculations themselves, for which dependencies include the existence and width of the scroll buttons.

Comment 53

[Frank Yan (:fryn)]

(In reply to comment #51)

By the way, thanks for all the testing, Michael :)

Comment 54

[Frank Yan (:fryn)]

Attachment: patch v9 [ui-r=beltzner] (updated to tip of tree)

Comment 55

[Mike Beltzner [:beltzner, not reading bugmail]]

While this isn't a blocker, it's a big win for users if we can get a review on it, Dao.

Comment 56

[ithinc]

Frank Yan, this feature has been implemented in Tab Utilities 0.9.9.9pre4+ (https://addons.mozilla.org/en-US/firefox/addon/59961/versions/). You may give it a try and get some hint. 
I have implemented the following:
1/ delayed resizing (set max-width = width when mouseover tab, reset max-width when mouseout tab bar)
2/ immediate resizing when closing last tab (set max-width = width * (n + 1)/n)
3/ delayed underflowing (postpone underflow event to mouseout tab bar)

Comment 57

[ithinc]

Also, have you noticed Bug 606727? It will lead to tab flicking. _handleNewTab is unnecessarily called when transitionend event happens.

Comment 58

[Frank Yan (:fryn)]

Attachment: patch v10 [ui-r=beltzner] (updated to tip of tree)

Comment 59

[ithinc]

Comment on attachment 488003 [details] [diff] [review]
patch v10 [ui-r=beltzner] (updated to tip of tree)

There are quite a few extensions to set "style: max-width" for tabs. It seems not proper to set "style: max-width" to "" when reverting tab sizing. Maybe save and restore its original value?
And "browser.tabs.animate" should be valid for additional tab animation.

Comment 60

[ithinc]

And gBrowser.visibleTabs is not much reliable. We'd better count "boxObject.width > 0 && flex > 0 && not removing" tabs.

Comment 61

[ithinc]

Comment on attachment 488003 [details] [diff] [review]
patch v10 [ui-r=beltzner] (updated to tip of tree)

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>+      <method name="_resizeTabsOnClose">
>+        <parameter name="aTab"/>
>+        <body><![CDATA[
>+          var tabs = this.tabbrowser.visibleTabs;

Replace tabbrowser.visibleTabs with Array.filter(gBrowser.mTabs, function(aTab) aTab.boxObject.width > 0 && (aTab.flex > 0 || aTab.flex === "" && getComputedStyle(aTab, null).MozBoxFlex > 0)), to be more accurate if some tab is hidden or iconified by an extension.

>+      <method name="_revertTabSizing">
>+        <body><![CDATA[
>+          if (this._hasTabTempMaxWidth) {
>+            this._hasTabTempMaxWidth = false;
>+            let tabs = this.tabbrowser.visibleTabs;
>+            for (let i = 0; i < tabs.length; i++)
>+              tabs[i].style.maxWidth = "";
>+          }

Restoring style.maxWidth to a saved value, instead of setting it to "", would be better.

Comment 62

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

We shouldn't work around visibleTabs shortcomings in an ad-hoc fashion. If we want visibleTabs to exclude tabs hidden by other means we should make that change in the getter. It's not clear to me that we should be doing work to accomodate addons that behave this way, though.

Comment 63

[ithinc]

(In reply to comment #62)
> We shouldn't work around visibleTabs shortcomings in an ad-hoc fashion. If we
> want visibleTabs to exclude tabs hidden by other means we should make that
> change in the getter. It's not clear to me that we should be doing work to
> accomodate addons that behave this way, though.

I agree with you that visibleTabs should not be touched. But it is a different case here. Maybe add a flexibleTabs getter?

Comment 64

[Frank Yan (:fryn)]

(In reply to comment #62)
> It's not clear to me that we should be doing work to
> accomodate addons that behave this way, though.

I concur that there isn't a compelling argument to accommodate this type of add-on behavior.

(In reply to comment #63)
> I agree with you that visibleTabs should not be touched. But it is a different
> case here. Maybe add a flexibleTabs getter?

Gavin just explained that, if we want to make this type of change, it should be made to visibleTabs.

Comment 65

[ithinc]

(In reply to comment #64)
> Gavin just explained that, if we want to make this type of change, it should be
> made to visibleTabs.

So far as I know, FaviconizeTab is a welcome add-on, which set the width of a tab to an icon's width. The tab is visible, but not flexible. This type of change couldn't involve visibleTabs.

Comment 66

[Rob Campbell [:rc] (:robcee)]

I think this got missed from triage. It's annoying. It's a chrome parity thing. And we have a patch.

Comment 67

[Rob Campbell [:rc] (:robcee)]

my bad, missed the blocking2.0 minus in the history. Disregard!

Comment 68

[Patrick Walton (:pcwalton)]

Appearing very frequently in input:

http://input.mozilla.com/en-US/opinion/906183
http://input.mozilla.com/en-US/opinion/903132
http://input.mozilla.com/en-US/opinion/889072

This is over the past 24 hours. May I humbly suggest that the blocking status on this one be reconsidered, given that we have a patch?

Comment 69

[Patrick Walton (:pcwalton)]

Renommed. Again, apologies if this is out of line, but I keep seeing this again and again in input and comments around the web when comparing Chrome and Firefox (search for "chrome tab" in Input and you'll see it mentioned a lot), and I'd hate for us to lose this little UI papercut when there's a patch.

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

If the patch is virtuous and good, we'll approve it. I can't see us holding the release on this issue. The best way to help it forward would be to take a review pass yourself to see if there's obvious things that need fixing, and of course to run things through tryserver (if it hasn't been done already) to understand perf issues on all three platforms.

The best way to get non-blocking patches in is to work to reduce risk; testing across platforms to ensure we don't get regressions from the patch helps!

Comment 71

[Justin Dolske [:Dolske]]

Comment on attachment 488003 [details] [diff] [review]
patch v10 [ui-r=beltzner] (updated to tip of tree)

Gavin agreed to be on the hook for finishing up the review for this, and will give up his puppy dog if he doesn't get to it soon (please, think of the puppy dog).

Comment 72

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 11: Resize on overflow.

I've split this patch up into two parts to lessen risk and make it easier to review: part 1 moves the close button into the place where the mouse pointer will be when the tabs overflow, and part 2 will make the tabs no longer resize. Currently, part 2 is broken due to some animation changes that landed between version 10 of this patch and now.

Part 1 is being tested on try server right now.

Comment 73

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 12: Resize on overflow.

Patch part 1, version 12 removes the unused field.

Comment 74

[Patrick Walton (:pcwalton)]

Try server tests were successful.

Comment 75

[Brendan Eich [:brendan]]

Patrick just demo'ed this, it's sweet. Many thumbs up.

/be

Comment 76

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 13: Resize on overflow.

Patch part 1, version 13 makes the method even simpler.

Comment 77

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2: Lock tab size when the mouse is in the tab area.

Patch part 2 locks the tab size when the mouse is in the tab area. Review requested for both of these: I made them as simple and low-risk as I could.

Comment 78

[Frank Yan (:fryn)]

Comment on attachment 499440 [details] [diff] [review]
Proposed patch, part 1, version 13: Resize on overflow.

(In reply to comment #75)
> Patrick just demo'ed this, it's sweet. Many thumbs up.
> 
> /be

Thanks. :)

(In reply to comment #76)
> Created attachment 499440 [details] [diff] [review]
> Proposed patch, part 1, version 13: Resize on overflow.
> 
> Patch part 1, version 13 makes the method even simpler.

> +   spacer.style.width = parseInt(spacer.style.width) + tabWidth + "px";

We might want to do parseFloat instead of parseInt, since tabWidth will almost certainly be a float every time. Otherwise, looks good. :)

Comment 79

[Frank Yan (:fryn)]

Comment on attachment 499443 [details] [diff] [review]
Proposed patch, part 2: Lock tab size when the mouse is in the tab area.

(In reply to comment #77)
> Created attachment 499443 [details] [diff] [review]
> Proposed patch, part 2: Lock tab size when the mouse is in the tab area.
> 
> Patch part 2 locks the tab size when the mouse is in the tab area. Review
> requested for both of these: I made them as simple and low-risk as I could.

This needs to enlarge the tabs (up to their usual max-width) when closing the rightmost tab. Is there going to be a part 3 for that?

Thanks for pitching in, by the way.

Comment 80

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 14: Resize the blank space on overflow.

Patch part 1, version 14 addresses feedback comments.

Comment 81

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 2: Lock tab size when the mouse is in the tab area.

Patch part 2, version 2 locks the tab size when the mouse is in the tab area, as before, except that the tabs resize when the user closes the rightmost tab.

Comment 82

[Frank Yan (:fryn)]

Comment on attachment 499453 [details] [diff] [review]
Proposed patch, part 1, version 14: Resize the blank space on overflow.

The !isEndTab check needs to stay there.

We only want the tabs to partially resize/revert in non-overflow mode.

Comment 83

[Frank Yan (:fryn)]

Comment on attachment 499453 [details] [diff] [review]
Proposed patch, part 1, version 14: Resize the blank space on overflow.

> +          this.mTabstrip.addEventListener("mouseover", this, false);
> +          this.mTabstrip.addEventListener("mouseout", this, false);

It looks like we never remove these event listeners.

Comment 84

[Frank Yan (:fryn)]

Comment on attachment 499454 [details] [diff] [review]
Proposed patch, part 2, version 2: Lock tab size when the mouse is in the tab area.

The tabs should only partially resize not completely revert.

Something like this (partially pseudocode):

if (!inOverflowMode && isEndTab) {
  let numNormalTabs = visibleTabs.length - numPinnedTabs;
  let tabWidth = currentTabWidth * (numNormalTabs + 1) / numNormalTabs;
  if (tabWidth > parseFloat(this._tabDefaultMaxWidth))
    tabWidth = this._tabDefaultMaxWidth;
  else
    tabWidth += "px";
  this._tabStyleDeclaration.setProperty("max-width", tabWidth, "");
}

Comment 85

[ithinc]

Comment on attachment 499454 [details] [diff] [review]
Proposed patch, part 2, version 2: Lock tab size when the mouse is in the tab area.

>+      <method name="_findTabStyleDeclaration">
>+          // Find the browser.css style sheet.
>+
>+          // Find the rule that sets the max width of a tab in the style sheet.
>+
>+          // Record the style so that we can alter it later, and record the
>+          // max width that was in the style sheet.

I'm afraid this find is not enough if the tab width is altered by some add-on or user style. A dynamic rule could be added like: '.tabbrowser-tabs[dontresize] > .tabbrowser-tab {max-width: #maxWidth#px !important;}'. Set dontresize attribute on tab-container when mouseover and remove it when mouseout.

Comment 86

[Dão Gottwald [:dao]]

(In reply to comment #83)
> > +          this.mTabstrip.addEventListener("mouseover", this, false);
> > +          this.mTabstrip.addEventListener("mouseout", this, false);
> 
> It looks like we never remove these event listeners.

I don't think they really need to be removed.

Comment 88

[ithinc]

Comment on attachment 499453 [details] [diff] [review]
Proposed patch, part 1, version 14: Resize the blank space on overflow.

>+              var byMouse = aParams.byMouse;

As there is _tabSizingLocked, byMouse parameter seems superfluous.

>+      <field name="_closingTabsSpacer">
>+        document.getAnonymousElementByAttribute(this, "anonid", "closing-tabs-spacer");
>+      </field>

If tabMaxWidth is set properly, _closingTabsSpacer seems superfluous.

>+            case "mouseout": {
>+              // Readjust tabs when the mouse leaves the tab container.

There is a drawback. If you move mouse quickly to the content area after closing a tab, the mouseout event doesn't bubble to the tab container as the tab has been removed. I'm not sure whether this is a bug or an intended behavior.

Comment 89

[ithinc]

Comment on attachment 499454 [details] [diff] [review]
Proposed patch, part 2, version 2: Lock tab size when the mouse is in the tab area.

>+          // If the tab that was just closed is the last tab, we *want* the
>+          // tabs to resize.
>+          if (aTab._tPos > tabs[tabs.length-1]._tPos) {
>+            this._revertTabSizing();
>+            return;
>+          }

This is not true. We want the tabs to resize to the current mouse position instead. The mouse position is not always at the end of the tab bar.


>+            case "mouseover": {
>               this._tabSizingLocked = true;
>+
>+              // Find the first unpinned tab to determine the tab size.

Do only if _tabSizingLocked  is false.

Comment 90

[Patrick Walton (:pcwalton)]

(In reply to comment #88) 
> >+      <field name="_closingTabsSpacer">
> >+        document.getAnonymousElementByAttribute(this, "anonid", "closing-tabs-spacer");
> >+      </field>
> 
> If tabMaxWidth is set properly, _closingTabsSpacer seems superfluous.

No, the spacer prevents the tabs from scrolling over on overflow. That's independent of the width of the tabs.

> >+            case "mouseout": {
> >+              // Readjust tabs when the mouse leaves the tab container.
> 
> There is a drawback. If you move mouse quickly to the content area after
> closing a tab, the mouseout event doesn't bubble to the tab container as the
> tab has been removed. I'm not sure whether this is a bug or an intended
> behavior.

Boo. I guess I'll switch it back to the old way, then. This seems like a bug to me.

Comment 91

[ithinc]

(In reply to comment #90)
> No, the spacer prevents the tabs from scrolling over on overflow. That's
> independent of the width of the tabs.

OK, I misunderstood it. We may intercept the underflow event in tabSizingLocked mode and resend it when revertTabSizing. gBrowser.mTabContainer.mTabstrip._scrollbox.style.MozPaddingEnd also works.

Comment 92

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 15: Resize the blank space on overflow.

Patch part 1, version 15 addresses feedback. The spacer is gone now!

Comment 93

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 2, version 3: Lock tab size when the mouse is in the tab area.

Patch part 2, version 3 addresses feedback. Review requested.

Comment 94

[Patrick Walton (:pcwalton)]

(In reply to comment #89)
> Comment on attachment 499454 [details] [diff] [review]
> Proposed patch, part 2, version 2: Lock tab size when the mouse is in the tab
> area.
> 
> >+          // If the tab that was just closed is the last tab, we *want* the
> >+          // tabs to resize.
> >+          if (aTab._tPos > tabs[tabs.length-1]._tPos) {
> >+            this._revertTabSizing();
> >+            return;
> >+          }
> 
> This is not true. We want the tabs to resize to the current mouse position
> instead. The mouse position is not always at the end of the tab bar.

That's true. What I do now in version 3 is that I split out the "add space to the end" logic and the "hammer in max-width" logic. Closing the last tab undoes the latter, but not the former, which ought to work in all cases.

> I'm afraid this find is not enough if the tab width is altered by some add-on
> or user style. A dynamic rule could be added like:
> '.tabbrowser-tabs[dontresize] > .tabbrowser-tab {max-width: #maxWidth#px
> !important;}'. Set dontresize attribute on tab-container when mouseover and
> remove it when mouseout.

Ok, now I'm a bit more fuzzy in the matching: it's now looking for a rule named ".tabbrowser-tab" that sets "max-width". Ultimately, there's no way to be foolproof in the presence of add-ons, though.

Comment 95

[YUKI "Piro" Hiroshi]

(In reply to comment #94)
> Ok, now I'm a bit more fuzzy in the matching: it's now looking for a rule named
> ".tabbrowser-tab" that sets "max-width". Ultimately, there's no way to be
> foolproof in the presence of add-ons, though.

The change doesn't solve the problem he said. For example, if there are two CSS rules:

  chrome://browser/skin/browser.css:
    .tabbrowser-tab:not([pinned]) { max-width: 250px; }

  userChrome.css in the profile:
    .tabbrowser-tab:not([pinned]) { max-width: 200px !important; }

then, you should reset max-width of tabs to 200px, not 250px.

The point of the idea in the comment #85 is "apply new max-width for tabs without modifying existing CSS rules". So, you can do it like:

  case "mouseover": {
    this._tabSizingLocked = true;
    ...
    this._tabSizingLockedStyle = document.createProcessingInstruction(
      'xml-stylesheet',
      'type="text/css" href="data:text/css,'+
        escape('.tabbrowser-tab:not([pinned]) { max-width:'+
               tabWidth+'px !important }')+'"'
    );
    document.insertBefore(this._tabSizingLockedStyle, document.documentElement);

and

  <method name="_revertTabSizing">
    <body><![CDATA[
      if (!this._tabSizingLocked || !this._tabSizingLockedStyle)
        return;
      document.removeChild(this._tabSizingLockedStyle);
      this._tabSizingLockedStyle = null;

Comment 96

[YUKI "Piro" Hiroshi]

Oops, I forgot to set "dontresize" attribute. The added stylesheet can be ignored by "!important" in the userChrome.css...

  case "mouseover": {
    this._tabSizingLocked = true;
    ...
    this._tabSizingLockedStyle = document.createProcessingInstruction(
      'xml-stylesheet',
      'type="text/css" href="data:text/css,'+
        escape('.tabbrowser-tabs[dontresize] > .tabbrowser-tab:not([pinned]) {'+
               'max-width:'+tabWidth+'px !important }')+'"'
    );
    document.insertBefore(this._tabSizingLockedStyle,
                          document.documentElement);
    this.setAttribute('dontresize', true);

and

  <method name="_revertTabSizing">
    <body><![CDATA[
      if (!this._tabSizingLocked || !this._tabSizingLockedStyle)
        return;
      document.removeChild(this._tabSizingLockedStyle);
      this.removeAttribute('dontresize');
      this._tabSizingLockedStyle = null;

Comment 97

[ithinc]

(In reply to comment #94)
> That's true. What I do now in version 3 is that I split out the "add space to
> the end" logic and the "hammer in max-width" logic. Closing the last tab undoes
> the latter, but not the former, which ought to work in all cases.

Maybe we can avoid touching the max-width if we always "add space to the end"? And we may "add space to the start" if the last tab is being closed? If so, the logic will be simple enough, and moreover, the overflown case (bug 595694) may also be handled properly.

Comment 98

[Patrick Walton (:pcwalton)]

(In reply to comment #95)
> (In reply to comment #94)
> > Ok, now I'm a bit more fuzzy in the matching: it's now looking for a rule named
> > ".tabbrowser-tab" that sets "max-width". Ultimately, there's no way to be
> > foolproof in the presence of add-ons, though.
> 
> The change doesn't solve the problem he said. For example, if there are two CSS
> rules:

I tried something similar (adding a new rule entirely that sets the max-width with !important). Unfortunately, that causes animation problems. We have to modify the rule in which the animation is being set, it seems.

Comment 99

[Patrick Walton (:pcwalton)]

(In reply to comment #97)
> Maybe we can avoid touching the max-width if we always "add space to the end"?
> And we may "add space to the start" if the last tab is being closed? If so, the
> logic will be simple enough, and moreover, the overflown case (bug 595694) may
> also be handled properly.

Interesting. Will try in a bit.

Comment 100

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 16: Code changes.

New version of the patch brings back the spacer and only uses it: no max-width hacks necessary! Thanks ithinc :)

Comment 101

[Frank Yan (:fryn)]

Comment on attachment 499844 [details] [diff] [review]
Proposed patch, version 16: Code changes.

The primary issue, which is the next close button not being directly under the cursor upon closing a tab with the mouse, has been fixed, which is fantastic (great work :D ), but the UI jank has not. Upon closing a tab, the other tabs shrink and then grow back, which is not what we want. We want only the closed tab to shrink and disappear. (That's why I set max-width.) To fix this using the new patch, we could have the non-closing, non-opening tabs not transition their max-width.

Comment 102

[Frank Yan (:fryn)]

Correction:
Ignore the last sentence of the previous comment. :P
I think transitioning the size of the spacer, at least in non-overflow mode, will fix the jank.

Comment 103

[ithinc]

(In reply to comment #100)
> New version of the patch brings back the spacer and only uses it: no max-width
> hacks necessary! Thanks ithinc :)

My pseudo-code is (no spacer needed, padding on scrollbox):

if closing non-last tab
  paddingEnd += tab.width;
else {
  paddingStart += tab.width;
  if (paddingStart > scrollPosition) {
    paddingStart = 0;
    paddingEnd -= scrollSize - scrollPosition - scrollClientSize;
    //intercept underflow event and resend it when mouseout
  }
}

Comment 104

[Patrick Walton (:pcwalton)]

(In reply to comment #102)
> Correction:
> Ignore the last sentence of the previous comment. :P
> I think transitioning the size of the spacer, at least in non-overflow mode,
> will fix the jank.

Doesn't work, the tabs before the one that closed bounce a little bit in seemingly random cases :( I think I'm going to go back to the hammering-in-the-max-width approach. Patch forthcoming.

> My pseudo-code is (no spacer needed, padding on scrollbox):
> 
> if closing non-last tab
>   paddingEnd += tab.width;
> else {
>   paddingStart += tab.width;
>   if (paddingStart > scrollPosition) {
>     paddingStart = 0;
>     paddingEnd -= scrollSize - scrollPosition - scrollClientSize;
>     //intercept underflow event and resend it when mouseout
>   }
> }

Doesn't work in the presence of tab animation. There are many many ways to make this work, but it's very tricky to get a flawless animation.

Comment 105

[ithinc]

(In reply to comment #104)
> > if closing non-last tab
> >   paddingEnd += tab.width;
> > else {
> >   paddingStart += tab.width;
> >   if (paddingStart > scrollPosition) {
> >     paddingStart = 0;
> >     paddingEnd -= scrollSize - scrollPosition - scrollClientSize;
> >     //intercept underflow event and resend it when mouseout
> >   }
> > }
> 
> Doesn't work in the presence of tab animation. There are many many ways to make
> this work, but it's very tricky to get a flawless animation.

If padding animation could make it work, I don't take it as a trick. 
There are three cases for this feature:
1/ non-overflow,
2/ overflow with scrollPosition = 0,
3/ overflow with scrollPosition > 0.

Your patch could handle properly only the first two cases?

Comment 106

[ithinc]

This logic maybe works better, and is more simple.

if closing non-last tab
  paddingEnd += tab.width;
else
  paddingEnd -= scrollSize - scrollPosition - scrollClientSize;

Comment 107

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, part 1, version 17: Lock tab size when the pointer is in the tab bar

Patch version 17 freezes the widths of the tabs, and of the strip itself, when the mouse pointer is inside the tab strip. This makes the animations correct, but it doesn't have the nice behavior when the last tab is closed.

There are a few ways we could fix this:
(1) Just unlock the tab width when the last tab is closed. This works except in what I'll call the "24" case: if you have tabs "1, 2, 3, 4" and then close tabs 2 and 4 without the pointer leaving the tab bar, it doesn't move the close tab button into place. This seems to be a rare case to me, though.
(2) Add some padding to the start of the tab strip to hold the place of the first tab when the last tab is closed. This has the advantage of working even in cases where the max width of the tabs would otherwise prevent the previous tab's close button from sliding into place properly. The disadvantage is that it feels a little strange at first...
(3) Try to calculate manually what the optimum tab width should be to put the close button under the mouse pointer. This is a little tricky and seems a little error-prone.

I'd like to go for (1) personally as a "good enough" measure to get this patch out the door. But I'm open to other ideas.

Comment 108

[Brendan Eich [:brendan]]

Mozilla is all about "good enough" when it is time to ship.

/be

Comment 109

[ithinc]

(In reply to comment #107)
> Patch version 17 freezes the widths of the tabs, and of the strip itself, when
> the mouse pointer is inside the tab strip. This makes the animations correct,
> but it doesn't have the nice behavior when the last tab is closed.

Maybe we can use a spacer with flex="10000" and set max-width of the spacer, but not width?

Comment 110

[ithinc]

When closing the last tab, we need a fixed spacer;
When closing a non-last tab, we need a growing spacer.

Comment 111

[ithinc]

The pseudo-code could be:

if closing non-last tab {
  spacer.maxWidth += tab.width;
  spacer.maxWidth -= scrollSize - scrollPosition - scrollClientSize;

  if (scrollPosition > 0)
    spacer.minWidth = spacer.maxWidth;
}

Comment 112

[ithinc]

In the mean time, capture and stopPropagation the underflow event, and resend it when mouse moves out of the tab bar.

Comment 113

[Patrick Walton (:pcwalton)]

(In reply to comment #111)
> The pseudo-code could be:
> 
> if closing non-last tab {
>   spacer.maxWidth += tab.width;
>   spacer.maxWidth -= scrollSize - scrollPosition - scrollClientSize;
> 
>   if (scrollPosition > 0)
>     spacer.minWidth = spacer.maxWidth;
> }

I don't see any way to make spacers work with the animation. Spacers are out of the question as far as I can see...

Well, I suppose we could hook the transitionend event and only add the spacer when the animation finishes. But that's a little messy. I'd like to get the simplest part of this patch in first, and then leave the rest to followup bugs (possibly post-Fx-4).

Comment 114

[ithinc]

I just realized that we cannot have exactly equal growing spacer as decreasing tab, so controlling the width or max-width seems a must, but as you said, we could add a spacer or padding after the tab is removed entirely, to solve your case "24". So animation is the trouble. Could the closing animation be canceled in this case?

Comment 115

[ithinc]

The Tab Utilities extension has provided this feature for a while. In version 1.0pre13, I implemented it with paddingEnd, so incorrect animation. Before version 1.0pre12, I implemented it with max-width, thus correct animation, but does not work for overflown case with scrollPosition > 0. You may take it as a reference.

https://addons.mozilla.org/en-US/firefox/addon/59961/versions/1.0pre13 line 924-981 in tabutils.js
https://addons.mozilla.org/en-US/firefox/addon/59961/versions/1.0pre12 line 922-1001 in tabutils.js

Comment 116

[Frank Yan (:fryn)]

Comment on attachment 500133 [details] [diff] [review]
Proposed patch, part 1, version 17: Lock tab size when the pointer is in the tab bar

This patch doesn't seem to do anything on my machine. What is it supposed to do?

The method of looking for styles within stylesheets, which requires setting constants and running a loop, does not seem to be less risky or more performant than simply setting tab.style.maxWidth, which runs in constant time and does not have a stylesheet dependency. For example, our code should not easily break when changing a line or two in our own browser.css, and it should not become slow when extensions include many stylesheets, one of which could be named browser.css.

A low-risk patch shouldn't be touching toolkit/.

Comment 117

[Frank Yan (:fryn)]

Nevermind about the toolkit/ bit. I see that it's just adding an anonid.

Comment 118

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 18: Lock tab size when the pointer is in the tab bar.

Yup, you're right. Patch version 18 just uses the style property. It's a lot cleaner now, I think.

And option (1) above is implemented now.

Comment 119

[Patrick Walton (:pcwalton)]

Oh, forgot to mention: if the patch isn't doing anything for you, make sure you're recompiling toolkit/ as well as browser/

Comment 120

[Frank Yan (:fryn)]

Comment on attachment 500250 [details] [diff] [review]
Proposed patch, version 18: Lock tab size when the pointer is in the tab bar.

AIUI, tabbrowser.visibleTabs runs Array.filter in its getter every time, so we really should cache that. It looks like the entire _lockTabSizing and _unlockTabSizing methods are being run every time. Setting and checking an isLocked boolean might be a good idea.

The idea of hammering in the width of the scrollbox is pretty clever, avoiding the underflow event in most cases. :)

When opening several tabs and moving the cursor to the tab strip during the opening animation, the tab strip can end up in a state where the tabs are larger than minimum size and in overflow mode when they should not be. This is due to _lockTabSizing being called on mousemove and _getTabWidth using tab.getBoundingClientRect().width, which is often in flux and thus unreliable. _lockTabSizing could instead be called inside removeTab, and _getTabWidth could use getComputedStyle(tab).maxWidth. This means we need to set max-width as we did before instead of width, but it avoids these problems and makes it easier to handle closing the last tab, since we already have a transition on max-width in browser.css.

Locking the tab width with the cursor inside the tab strip even when closing tabs with a keyboard shortcut is unnecessary but not much of an issue.

(In reply to comment #119)
> Oh, forgot to mention: if the patch isn't doing anything for you, make sure
> you're recompiling toolkit/ as well as browser/

That did it. Thanks.

(In reply to comment #107)
> Patch version 17 freezes the widths of the tabs, and of the strip itself, when
> the mouse pointer is inside the tab strip. This makes the animations correct,
> but it doesn't have the nice behavior when the last tab is closed.
> 
> There are a few ways we could fix this:
> (1) Just unlock the tab width when the last tab is closed. This works except in
> what I'll call the "24" case: if you have tabs "1, 2, 3, 4" and then close tabs
> 2 and 4 without the pointer leaving the tab bar, it doesn't move the close tab
> button into place. This seems to be a rare case to me, though.

patch v18 also reverts tab width when the tab strip is filled with n tabs and closing from a middle tab to the last tab. This behavior is unexpected, and a general rule for usability is that predictable behavior is just as important than clever or good behavior.

> (2) Add some padding to the start of the tab strip to hold the place of the
> first tab when the last tab is closed. This has the advantage of working even
> in cases where the max width of the tabs would otherwise prevent the previous
> tab's close button from sliding into place properly. The disadvantage is that
> it feels a little strange at first...

I considered this behavior months ago when writing my patch for this and came to the following conclusion:
Shifting tabs to the right and leaving a gap on the left is an additional movement that is not triggered by any other event in the browser, which is why the effect can look odd.
By resizing the tabs partway or, in overflow mode, leaving the space temporarily, we are having the tabs partly transition to their new state and deferring the remainder of the transition until the cursor leaves, rather than adding new transitions.

> (3) Try to calculate manually what the optimum tab width should be to put the
> close button under the mouse pointer. This is a little tricky and seems a
> little error-prone.

I prefer that we fix the case of closing the last tab. By solving this case, we accomplish both good and predictable behavior.

Comment 121

[Frank Yan (:fryn)]

> patch v18 also reverts tab width when the tab strip is filled with n tabs and
closing from a middle tab to the last tab.

To clarify, I mean, closing unpinned tabs i, i+1, i+2, i+3, ..., n, in that order, where i > 1. This is just to note that, by reverting whenever isAtEnd is true, it creates unexpected behavior for more than just the "2-4" case.

Comment 122

[ithinc]

(In reply to comment #116)
> The method of looking for styles within stylesheets, which requires setting
> constants and running a loop, does not seem to be less risky or more performant
> than simply setting tab.style.maxWidth, which runs in constant time and does
> not have a stylesheet dependency. 

Setting tab.style.maxWidth involves another loop. We may add a static rule ".tabbrowser-tabs[dontresize=true] > .tabbrowser-tab" in browser.css and hold a reference to it in tabbrowser.xml, and turn it on/off with an attribute on the tabcontainer.

Comment 123

[ithinc]

(In reply to comment #120)
> This means we need to set max-width as we
> did before instead of width, but it avoids these problems and makes it easier
> to handle closing the last tab, since we already have a transition on max-width
> in browser.css.

I agree with this.

> Locking the tab width with the cursor inside the tab strip even when closing
> tabs with a keyboard shortcut is unnecessary but not much of an issue.

I think that is a good part. I prefer no byMouse parameter in removeTab. We just set/unset lockingMode.

Comment 124

[ithinc]

About the mousemove/mouseout event, I suggest:
Lock tab sizing with mouseover event (mouse over on tab);
Unlock tab sizing with mousemove event (mouse move out of tabContainer +/- height * 0.5).

When moving mouse, it's easy to slide out of the tab bar a bit.

Comment 125

[Patrick Walton (:pcwalton)]

(In reply to comment #120)
> When opening several tabs and moving the cursor to the tab strip during the
> opening animation, the tab strip can end up in a state where the tabs are
> larger than minimum size and in overflow mode when they should not be. This is
> due to _lockTabSizing being called on mousemove and _getTabWidth using
> tab.getBoundingClientRect().width, which is often in flux and thus unreliable.
> _lockTabSizing could instead be called inside removeTab, and _getTabWidth could
> use getComputedStyle(tab).maxWidth. This means we need to set max-width as we
> did before instead of width, but it avoids these problems and makes it easier
> to handle closing the last tab, since we already have a transition on max-width
> in browser.css.

I'm not sure this works; it's not just max-width that controls the size of the tabs at any given time, but rather a combination of min-width, max-width, and moz-box-flex. The animation problems with the original patch posted here were a result of the animation of these three properties interacting badly with the desire to keep the tab width constant. Turning off flexbox entirely (via setting MozBoxFlex to 0) seemed to me to be the easiest way to fix this.

I'll try a workaround.

Comment 126

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.

Version 19 addresses the feedback comments.

Comment 127

[ithinc]

Comment on attachment 500282 [details] [diff] [review]
Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.

>+            if (!isAtEnd && byMouse && !isClosing && isFullyOpen)
>+              this.tabContainer._lockTabSizing(aTab);

Check _tabSizingLocked instead of byMouse?

>+          this.mTabstrip.addEventListener("mouseout", this, false);

'Mouseout' event is not reliable.

Comment 128

[Patrick Walton (:pcwalton)]

(In reply to comment #127)
> Comment on attachment 500282 [details] [diff] [review]
> Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.
> 
> >+            if (!isAtEnd && byMouse && !isClosing && isFullyOpen)
> >+              this.tabContainer._lockTabSizing(aTab);
> 
> Check _tabSizingLocked instead of byMouse?
> 
> >+          this.mTabstrip.addEventListener("mouseout", this, false);
> 
> 'Mouseout' event is not reliable.

Not sure how to get around it without putting a "mousemove" event handler on the entire browser, which I'm not sure about from a performance standpoint.

Comment 129

[ithinc]

(In reply to comment #128)
> Not sure how to get around it without putting a "mousemove" event handler on
> the entire browser, which I'm not sure about from a performance standpoint.

Add the event handler when lockTabSizing and remove it when expected mousemove happens. There's quite a few instances of mousemove in browser.js.

Comment 130

[ithinc]

Comment on attachment 500282 [details] [diff] [review]
Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.

>+          // Find the width of the tab strip, and hammer it in.
>+          let innerBoxNode = document.getAnonymousElementByAttribute(
>+            this.mTabstrip._scrollbox, "anonid", "scrollbox-innerbox");
>+          innerBoxNode.style.minWidth = innerBoxNode.clientWidth + "px";

It seems a good idea! Maybe add a "_box" property in scrollbox binding? document.getAnonymousNodes(this.mTabstrip._scrollbox)[0] also works, assuming scrollbox has always one child only.

Comment 131

[ithinc]

(In reply to comment #107)
> There are a few ways we could fix this:
> (1) Just unlock the tab width when the last tab is closed. 

As you set the min-width of scrollbox-innerbox, filling the trailing space with paddingEnd or spacer before unlocking the tab width seems OK in case of closing last tab.

Comment 132

[ithinc]

Comment on attachment 500282 [details] [diff] [review]
Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.

>+          // Find the width of the tab strip, and hammer it in.
>+          let innerBoxNode = document.getAnonymousElementByAttribute(
>+            this.mTabstrip._scrollbox, "anonid", "scrollbox-innerbox");
>+          innerBoxNode.style.minWidth = innerBoxNode.clientWidth + "px";

This seems to have another drawback. When closing the last tab, the remaining tabs grow despite of the tabContainer width, and the next tab won't be pushed under the mouse pointer if the tab has grown to its max-width. I still suggest capturing the underflow event.

Comment 133

[ithinc]

(In reply to comment #113)
> I don't see any way to make spacers work with the animation. Spacers are out of
> the question as far as I can see...

If we set both max-width and min-width, maybe we could make spacers to work with animation.The max-width and min-width should not be set on TabClose, because they have "animation" and won't take into effect immediately.

.tabbrowser-tabs[dontresize] > .tabbrowser-tab {min-width: #width#px !important; max-width: #width#px !important;}
.tabbrowser-tabs[dontresize] .closing-tabs-spacer {max-width: #width#px; -moz-box-flex: 10000;}

Comment 134

[Patrick Walton (:pcwalton)]

(In reply to comment #132)
> Comment on attachment 500282 [details] [diff] [review]
> Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.
> 
> >+          // Find the width of the tab strip, and hammer it in.
> >+          let innerBoxNode = document.getAnonymousElementByAttribute(
> >+            this.mTabstrip._scrollbox, "anonid", "scrollbox-innerbox");
> >+          innerBoxNode.style.minWidth = innerBoxNode.clientWidth + "px";
> 
> This seems to have another drawback. When closing the last tab, the remaining
> tabs grow despite of the tabContainer width, and the next tab won't be pushed
> under the mouse pointer if the tab has grown to its max-width. I still suggest
> capturing the underflow event.

Yes, that's the problem I mentioned with (1) above.

Comment 135

[ithinc]

(In reply to comment #125)
> I'm not sure this works; it's not just max-width that controls the size of the
> tabs at any given time, but rather a combination of min-width, max-width, and
> moz-box-flex. The animation problems with the original patch posted here were a
> result of the animation of these three properties interacting badly with the
> desire to keep the tab width constant. Turning off flexbox entirely (via
> setting MozBoxFlex to 0) seemed to me to be the easiest way to fix this.

Agreed with that setting width and MozBoxFlex would be better to make the tab stick with the width, but what if unlocking the tab size? As min-width and max-wdith won't change in this case, there will be no animation. So maybe we can set width, MozBoxFlex, together with max-width.

Comment 136

[ithinc]

There are still animation problem with setting width and MozBoxFlex. When closing a 2nd tab, it will have no closing animation.

What we should do is:
Turn off animation temporally when lockTabSizing,
Set max-width and min-width,
Turn on animation when lockTabSizing is finished.

Comment 137

[ithinc]

A perfect solution has been implemented in the Tab Utilities extension 1.0pre15. It handles all the cases of this feature. I use both max-width and min-width, together with a spacer. 

https://addons.mozilla.org/en-US/firefox/addon/59961/versions/1.0pre15 line
920-1025 in tabutils.js

The pseudo-code is:

if closing non-last tab {
  lockTabSizing; (set max-width and min-width)

  spacer.maxWidth += tab.width;
  spacer.maxWidth -= scrollSize - scrollPosition - scrollClientSize;
  if (scrollPosition > 0)
    spacer.minWidth = spacer.maxWidth;
}
else {
  spacer.minWidth = spacer.maxWidth;
  unlockTabSizing;
}

When lockTabSizing or unlockTabSizing, turn off the tab animation temporally.

Comment 138

[Patrick Walton (:pcwalton)]

(In reply to comment #137)
> A perfect solution has been implemented in the Tab Utilities extension
> 1.0pre15. It handles all the cases of this feature. I use both max-width and
> min-width, together with a spacer. 

I'd like to keep the spacer to a followup, to contain the scope of this patch. I definitely want the perfect solution in, but given the schedule and review constraints I'd like to focus on the minimum viable product :)

Comment 139

[(no longer active)]

I'd like to mention that this should be tested in RTL mode too.  I'd be happy to do the testing if you can point me to a try server build.

Comment 140

[Frank Yan (:fryn)]

(In reply to comment #139)
> I'd like to mention that this should be tested in RTL mode too.  I'd be happy
> to do the testing if you can point me to a try server build.

Yeah, I've been testing all my tabbed browsing patches (including bug 455694) in RTL. I haven't tested Patrick's in RTL yet, but will do now.

Comment 141

[Frank Yan (:fryn)]

(In reply to comment #128)
> (In reply to comment #127)
> > Comment on attachment 500282 [details] [diff] [review]
> > Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.
> > 
> > >+            if (!isAtEnd && byMouse && !isClosing && isFullyOpen)
> > >+              this.tabContainer._lockTabSizing(aTab);
> > 
> > Check _tabSizingLocked instead of byMouse?

byMouse is fine. We don't need to lock anything until a tab is closed by a pointing device.

> > >+          this.mTabstrip.addEventListener("mouseout", this, false);
> > 
> > 'Mouseout' event is not reliable.
> 
> Not sure how to get around it without putting a "mousemove" event handler on
> the entire browser, which I'm not sure about from a performance standpoint.

I found that mouseout was not reliable too but have forgotten in which cases it breaks. Temporarily attaching a mousemove handler to the entire browser didn't seem to cause any problems, and yeah, we already do that in other code.
(In reply to comment #137)
> A perfect solution has been implemented in the Tab Utilities extension
> 1.0pre15. It handles all the cases of this feature. I use both max-width and
> min-width, together with a spacer. 

I just tried it, and it causes overflow at incorrect times when hovering with the mouse cursor over the tab strip.

(In reply to comment #138)
> I'd like to keep the spacer to a followup, to contain the scope of this patch.
> I definitely want the perfect solution in, but given the schedule and review
> constraints I'd like to focus on the minimum viable product :)

Technically, your patch does fix the issue in the bug description.
I'm now okay with leaving the fix for closing the last tab to a followup, since I don't know if I'll have time to fix that case until next week.

(In reply to comment #139)
> I'd like to mention that this should be tested in RTL mode too.  I'd be happy
> to do the testing if you can point me to a try server build.

Tested and works just as well as in LTR.

Comment 142

[Frank Yan (:fryn)]

Comment on attachment 500282 [details] [diff] [review]
Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.

Just one last note:
Both Patrick's patch and mine use a loop through all the visible tabs to fix their width. In my patch, this loop only ran when the tab strip was in non-overflow mode, so the number of iterations was bounded by the tab strip width divided by 100, which in actual cases, comes out to about 6-20. In Patrick's patch, he uses this loop also in overflow mode, so the number of iterations is unbounded and simply determined by the number of tabs. I have not benchmarked how fast the loop is, so I am not certain of the performance implications of this, so I suppose that it is a reasonable expectation that when a user has several hundred tabs, the UI will not be as snappy as when having only a dozen. If we can optimize adjusting/fixing the tab width, we should, changing styles in stylesheets is okay with me, as long as it is robust. We should not write code that accounts for add-ons messing with it. If add-ons want to change our tabbed browsing behavior, they can look to our code to see how to write code that will function properly, especially since we have not released a Add-ons SDK-like API for this.

Comment 143

[Frank Yan (:fryn)]

Comment on attachment 500282 [details] [diff] [review]
Proposed patch, version 19: Lock tab size when the pointer is in the tab bar.

Since this patch has different behavior than mine, we should get UI review to confirm that not fixing the "closing any non-rightmost tabs then closing the rightmost tab" case is an acceptable standalone fix for now.

I will look into fixing the aforementioned case as soon as I have time.

Comment 144

[Frank Yan (:fryn)]

The landing of bug 572160 bitrotted this patch. Please update to tip, and link to tryserver builds for ease of review and UI feedback.

Comment 145

[Alex Limi (:limi) — Firefox UX Team]

I'm happy to do the UI review on this once there is a tryserver build.

Comment 146

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 20: Lock tab size when the pointer is in the tab bar.

Patch version 20 updates to trunk. Try server build forthcoming!

Comment 147

[ithinc]

(In reply to comment #141)
> > > Check _tabSizingLocked instead of byMouse?
> 
> byMouse is fine. We don't need to lock anything until a tab is closed by a
> pointing device.

Then have you checked what will happen if closing tabs with mouse and keyboard interchangeably? Should unlockTabSizing be done if byMouse is false?

> I found that mouseout was not reliable too but have forgotten in which cases it
> breaks. Temporarily attaching a mousemove handler to the entire browser didn't
> seem to cause any problems, and yeah, we already do that in other code.

See comment 88.

> > A perfect solution has been implemented in the Tab Utilities extension
> > 1.0pre15. It handles all the cases of this feature. I use both max-width and
> > min-width, together with a spacer. 
> 
> I just tried it, and it causes overflow at incorrect times when hovering with
> the mouse cursor over the tab strip.

I'm not sure what you mean. Any steps to reproduce it?

Comment 148

[Patrick Walton (:pcwalton)]

Try builds available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-435641070249/

Comment 149

[Siddhartha Dugar [:sdrocking]]

Doesn't look good with overflow. I suggest, when closing the 2nd last tab bring the 3rd last instead of the last one to its place. Not sure if this has been tried before.

Comment 150

[Frank Yan (:fryn)]

(In reply to comment #149)
> Doesn't look good with overflow. I suggest, when closing the 2nd last tab bring
> the 3rd last instead of the last one to its place. Not sure if this has been
> tried before.

We should always try to move the close button of the tab that will be selected next under the cursor, so this doesn't make sense, except in cases where the 3rd last tab is going to be selected next.

(In reply to comment #146)
> Created attachment 502184 [details] [diff] [review]
> Proposed patch, version 20: Lock tab size when the pointer is in the tab bar.

> +            if (!isAtEnd && byMouse && !isClosing && isFullyOpen)
> +              this.tabContainer._lockTabSizing(aTab);

This and version 19 do not take into account which tab will be selected next. See what I just wrote above.
We really should address this too. That's why I included !aTab.opener in the if condition for my patch (version 10).
I'm reluctant to leave patches to followups unless there is a known plan of attack for the followup patch that doesn't require rewriting a significant portion of the main patch. (Defeats the point of a followup.)

Comment 151

[Alex Limi (:limi) — Firefox UX Team]

* There's some significant flicker going on when closing the rightmost tab with lots of tabs open, is there any way we can make this smoother (tested on OS X).

* Background tabs seem to lose their close buttons even when there are few enough of them to allow it. Happens when you open a lot of tabs, close enough of them to go under the threshold where close buttons should come back, and move the pointer away from the tab bar. If you resize the window, the close buttons are back, so moving the pointer outside the tab bar should probably trigger the calculation of whether tab close buttons should be shown.

Comment 152

[ithinc]

(In reply to comment #2)
> Delaying the resize until after the
> mouse leaves the chrome area at top (which is more general than just the
> tabstrip, to account for people's hands not moving perfectly horizontally as
> they try to get from tab to tab) would be a very nice piece of polish.

This should be considered.

Comment 153

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 21.

Here's a new version of this patch. It fixes all the issues I can think of:
* When closing the last tab, the next-to-last tab slides under the mouse cursor as expected.
* Tabs get their close buttons back when the mouse pointer leaves the tab bar.
* The tabs animate to the correct size when the mouse pointer leaves the tab bar.
* I believe the flicker issue you mentioned is gone. I think that was an animation bug that has been fixed since the last update of this patch.

Comment 154

[Patrick Walton (:pcwalton)]

Baking on try server. I'll have a try build ready when it's done.

Comment 155

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 22.

Patch version 22 fixes some RTL issues and cleans up the animation a bit.

Comment 156

[Patrick Walton (:pcwalton)]

Try builds here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-9b38fa818346/

Comment 157

[Patrick Walton (:pcwalton)]

(In reply to comment #152)
> (In reply to comment #2)
> > Delaying the resize until after the
> > mouse leaves the chrome area at top (which is more general than just the
> > tabstrip, to account for people's hands not moving perfectly horizontally as
> > they try to get from tab to tab) would be a very nice piece of polish.
> 
> This should be considered.

Patch version 22 does this.

Comment 158

[Frank Yan (:fryn)]

Comment on attachment 507023 [details] [diff] [review]
Proposed patch, version 22.

I only tested on OS X, and this works well for the most part in LTR mode.

Unfortunately, it breaks in RTL mode, when closing multiple tabs starting from a non-leftmost tab and closing enough to exhaust all tabs to the left.

It also breaks when there are multiple tab groups such a tab in a non-visible tab group has a _tPos value greater than the rightmost tab in the visible tab group. This can be reproduced by the following steps:
1. Open enough tabs such that the tabs are well under the maximum tab width.
2. Enter tab view.
3. Move the last tab from the current group into another group.
4. Return to normal view in the current group.
5. Close the last tab. Tabs don't expand/slide over like they should.
From glancing at the code, I think the culprit is |var isAtEnd = aTab._tPos == this.tabs.length - 1;|.
I'm not happy with how Panorama interactions have been injected into our tabbed browsing code, but we don't have time to address that before Firefox 4, so we'll just have to do a workaround here.

---

Two cases with suboptimal behavior that we could handle in a followup:

Case A
1. Open enough tabs to overflow and then several more. In LTR mode, the rightmost tab should now be visible.
2. Counting from the leftmost _visible_ tab, select the 3rd or 4th tab.
3. Click the tab close button to close that tab.
4. Keep closing tabs without moving the mouse. Eventually, this will exhaust all the tabs on the right, tabs will start moving in from the left. At some point, the tabs will enlarge and move too far to the right such that the tab close button no longer slides directly underneath the mouse cursor.

Case B
1. Open enough tabs such that they were well under the maximum tab width. (Overflow or not doesn't matter.)
2. Select one of the visible tabs in the middle, and navigate that tab to http://news.google.com/
3. Click on one of the headlines. The story should open in a new tab.
4. Click on the newly selected tab's close button.
5. Google News's tab becomes selected, but the mouse cursor is over the unselected tab.
To fix this case, we simply need to check |tab.owner| and possibly the pref |browser.tabs.selectOwnerOnClose|.

Comment 159

[Patrick Walton (:pcwalton)]

(In reply to comment #158)
> Two cases with suboptimal behavior that we could handle in a followup:
> 
> Case A
> 1. Open enough tabs to overflow and then several more. In LTR mode, the
> rightmost tab should now be visible.
> 2. Counting from the leftmost _visible_ tab, select the 3rd or 4th tab.
> 3. Click the tab close button to close that tab.
> 4. Keep closing tabs without moving the mouse. Eventually, this will exhaust
> all the tabs on the right, tabs will start moving in from the left. At some
> point, the tabs will enlarge and move too far to the right such that the tab
> close button no longer slides directly underneath the mouse cursor.

The problem here is that the tab strip is scrolling to the left for some reason. Notice that if you click no the right arrow the tabs scroll over to the right position. Grr. I'll investigate and see if there's a way to fix this.

Comment 160

[Frank Yan (:fryn)]

(In reply to comment #159)

It could be that the scrollbox is trying to ensure that the selected tab is completely visible (ensureElementIsVisible).
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml#197

Comment 161

[Patrick Walton (:pcwalton)]

(In reply to comment #158)
> Two cases with suboptimal behavior that we could handle in a followup:
> 
> Case A
> 1. Open enough tabs to overflow and then several more. In LTR mode, the
> rightmost tab should now be visible.
> 2. Counting from the leftmost _visible_ tab, select the 3rd or 4th tab.
> 3. Click the tab close button to close that tab.
> 4. Keep closing tabs without moving the mouse. Eventually, this will exhaust
> all the tabs on the right, tabs will start moving in from the left. At some
> point, the tabs will enlarge and move too far to the right such that the tab
> close button no longer slides directly underneath the mouse cursor.

Interestingly, Chrome has the same problem when you get down to one or two tabs -- try it!

Comment 162

[Frank Yan (:fryn)]

(In reply to comment #161)
> Interestingly, Chrome has the same problem when you get down to one or two tabs
> -- try it!

Yup; it only happens for me when you have so many tabs that each tab is <30 pixels side, and then start closing tabs. Hint: opportunity to exceed Chrome parity, especially since we have better overflow handling. :)

Comment 163

[Patrick Walton (:pcwalton)]

(In reply to comment #158)
> Two cases with suboptimal behavior that we could handle in a followup:

The solution to both of these is to add a spacer to the beginning of the tab strip as well as to the end. For Case A, we want to move the tabs over to the right a little bit. For Case B, we want to add a spacer to the start of the tab strip to move the owner tab over to the right. Because these bugs are solvable in basically the same way, I'd like to leave them to a followup.

Comment 164

[Frank Yan (:fryn)]

(In reply to comment #163)

I really don't think we should add spacers to the beginning of the tab strip, as I explained in comment 120 and https://frankyan.wordpress.com/making-tab-closing-easy/ and https://frankyan.wordpress.com/making-tab-closing-easy/#comment-49

Comment 165

[Patrick Walton (:pcwalton)]

(In reply to comment #164)
> (In reply to comment #163)
> 
> I really don't think we should add spacers to the beginning of the tab strip,
> as I explained in comment 120 and
> https://frankyan.wordpress.com/making-tab-closing-easy/ and
> https://frankyan.wordpress.com/making-tab-closing-easy/#comment-49

Well, I'm not sure how to solve (A) in particular otherwise. Either we have to move the tabs over, or we have to stretch the tab to become larger than it ordinarily would be. The latter is hard, because setting max-width screws up the length calculation, and trying to calculate the width manually is difficult and error-prone. I'm not saying it can't be done, but I think it requires a separate approach, built on top of this patch.

Comment 166

[Frank Yan (:fryn)]

(In reply to comment #165)

I understand; I ran into that problem too. Let's leave that to a followup. Once the RTL and Panorama problems are fixed (assuming no other major issues are found), I'm happy to feedback+ this.

Comment 167

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 23.

Patch version 23 fixes the RTL issue and the Panorama bug, as well as I can tell. It also mitigates papercut (A) a bit.

Comment 168

[Alex Limi (:limi) — Firefox UX Team]

This seems to be pretty solid now (and a huge UX win), so I'd like to nominate it as a softblocker. 

Needs review from a peer, of course — but I believe strongly that this is worth our consideration for Firefox 4, even this late in the cycle.

Comment 169

[Dão Gottwald [:dao]]

Please don't add [softblocker] to bugs that don't block. It breaks simple sw:softblocker queries.

Comment 170

[Frank Yan (:fryn)]

IIRC, Beltzner said not to use [d?] anymore.

Comment 171

[Dietrich Ayala (:dietrich)]

Comment #21 remains true. This still doesn't block. Whether the patch is pretty solid in terms of conceptual and code progress has no bearing on it's blocker-ness.

But as J said, we want it, so would take a low-risk, good-test-coverage, reviewed patch.

Comment 172

[Patrick Walton (:pcwalton)]

Working on a test case now. Limi, can you do ui-r on this?

Comment 173

[Alex Limi (:limi) — Firefox UX Team]

(In reply to comment #172)
> Working on a test case now. Limi, can you do ui-r on this?

Yes, more than happy to.

Comment 174

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 24.

Patch version 24 adds a unit test. Baking on try server; builds will be here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-b54200a778b0

IMO this is nearing the end of the cycle; reviews and feedback would be much appreciated.

Comment 175

[Cork]

I think i've found something here:

1. start a new window
2. create new tabs until you get 4 tabs overflow on the left side
3. set focus on the first completely visible tab
4. close all tabs

When you get to the second last tab, it doesn't end up under the cursor.

If you open one tab less, sometimes the last tab doesn't scroll into view at all.

Tested on:
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre

Comment 176

[Patrick Walton (:pcwalton)]

(In reply to comment #175)
> I think i've found something here:
> 
> 1. start a new window
> 2. create new tabs until you get 4 tabs overflow on the left side
> 3. set focus on the first completely visible tab
> 4. close all tabs
> 
> When you get to the second last tab, it doesn't end up under the cursor.
> 
> If you open one tab less, sometimes the last tab doesn't scroll into view at
> all.
> 
> Tested on:
> Mozilla/5.0 (X11; Linux i686 on x86_64; rv:2.0b11pre) Gecko/20110127
> Firefox/4.0b11pre

See above; that sounds like issue (A) to me.

Comment 177

[Frank Yan (:fryn)]

Comment on attachment 507721 [details] [diff] [review]
Proposed patch, version 24.

>+                Services.console.logStringMessage("*** mousemove!\n");

This should be removed.

(In reply to comment #175)
> If you open one tab less, sometimes the last tab doesn't scroll into view at
> all.

Even though the cause of this is the same as that of issue (a), we should at least fix this part.
Would something like |scrollbox.ensureElementIsVisible(selectedTab)| do it?

feedback+ with these addressed.

Comment 178

[Frank Yan (:fryn)]

The testcase looks good. Thanks for writing it.

From the user's perspective, he/she will be clicking repeatedly without moving the cursor to close multiple tabs. A test might run like that too: find the center of a tab's close button and start simulating mouse events (perhaps one each second) at that location; tabs should keep being closed. Having a test like this isn't necessary to land this feature, but we ought to consider writing a test that simulates how a user might interact with the feature, since that's the point of implementing it.

Comment 179

[Patrick Walton (:pcwalton)]

(In reply to comment #177)
> (In reply to comment #175)
> > If you open one tab less, sometimes the last tab doesn't scroll into view at
> > all.
> 
> Even though the cause of this is the same as that of issue (a), we should at
> least fix this part.
> Would something like |scrollbox.ensureElementIsVisible(selectedTab)| do it?

Unfortunately, if we do that, we regress the partial solution to issue (A) I implemented. What we should do is something like "if the tab is *completely* invisible, only then call ensureElementIsVisible() on it".

Comment 180

[Patrick Walton (:pcwalton)]

(In reply to comment #179)
> Unfortunately, if we do that, we regress the partial solution to issue (A) I
> implemented. What we should do is something like "if the tab is *completely*
> invisible, only then call ensureElementIsVisible() on it".

I tried this, and it ended up being confusing. I'm going to back out the partial solution to (A), I think; better to be consistent than to do it halfway and make the edge cases confusing.

Comment 181

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 25.

Patch version 25 addresses feedback and adds in the ensureElementIsVisible() again. Review and UI review requested.

Comment 182

[Frank Yan (:fryn)]

(In reply to comment #180)
> (In reply to comment #179)
> > Unfortunately, if we do that, we regress the partial solution to issue (A) I
> > implemented. What we should do is something like "if the tab is *completely*
> > invisible, only then call ensureElementIsVisible() on it".
> 
> I tried this, and it ended up being confusing. I'm going to back out the
> partial solution to (A), I think; better to be consistent than to do it halfway
> and make the edge cases confusing.
Yeah, I know it's not ideal, but we need to avoid regressing from 3.6. The selected tab always scrolls into view in 3.6 and should continue to do so in 4.

Gavin, I think this is ready for a more thorough review.

Comment 183

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 507954 [details] [diff] [review]
Proposed patch, version 25.

This looks good, and works great for me. 

The one thing we should fix before we land are the RTL mode bugs:

* When you run out of tabs on the left, it doesn't pull in tabs from the right in overflow mode.

* In non-overflow mode, it just collapses everything to minimum width when you run out of tabs to close on the left.

Comment 184

[Frank Yan (:fryn)]

To force RTL mode for testing in Firefox, you can use Ehsan's awesome extension:
https://addons.mozilla.org/addon/force-rtl/

Comment 185

[Patrick Walton (:pcwalton)]

(In reply to comment #183)
> Comment on attachment 507954 [details] [diff] [review]
> Proposed patch, version 25.
> 
> This looks good, and works great for me. 
> 
> The one thing we should fix before we land are the RTL mode bugs:
> 
> * When you run out of tabs on the left, it doesn't pull in tabs from the right
> in overflow mode.
> 
> * In non-overflow mode, it just collapses everything to minimum width when you
> run out of tabs to close on the left.

I can't reproduce these. Did you open a new window after setting intl.uidirection.en-US to "rtl"? The code sets some internal state based on the current direction.

Frank, can you reproduce these issues with v25?

Comment 186

[Patrick Walton (:pcwalton)]

Baking again on try server just to be sure. Builds will be here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-7c0ef6e973cb

Comment 187

[Frank Yan (:fryn)]

Comment on attachment 507954 [details] [diff] [review]
Proposed patch, version 25.

(In reply to comment #185)
> I can't reproduce these. Did you open a new window after setting
> intl.uidirection.en-US to "rtl"? The code sets some internal state based on the
> current direction.
> 
> Frank, can you reproduce these issues with v25?

You are correct. I tried it in a new window and could not reproduce the issue.
In that case, flipping the flag to ui-review+, since that's the only issue that limi and i found with it that would have blocked landing.

Thanks.

Comment 188

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 507954 [details] [diff] [review]
Proposed patch, version 25.

Yup, seems to work great once the RTL stuff is initialized properly.

Comment 189

[Patrick Walton (:pcwalton)]

I am told that this is no longer possible for Firefox 4.

This will be released as an addon.

Comment 190

[avada]

(In reply to comment #189)
> I am told that this is no longer possible for Firefox 4.
> 
> This will be released as an addon.

Why wouldn't it be?

Comment 191

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #189)
> I am told that this is no longer possible for Firefox 4.

I think you were told that it is late in the cycle to be adding a bunch of risk, and with 25 versions of a large patch, at a glance, this seems to be risky. It's not "no longer possible" - I have expressed (as have other people) great interest in seeing it done. It's just not trivial, adds risk, and not a blocker which would prevent us from shipping the release.

If we get reviews on this, and if people can help us understand why we think it's not introducing a bunch of risk, then please please please ask for approval and ping me on IRC. You will see how interested I am.

Comment 192

[Patrick Walton (:pcwalton)]

As always, if someone wants to review I'm more than happy to address comments. This is basically out of my hands at this point.

Comment 193

[Patrick Walton (:pcwalton)]

How about preffing it off for Firefox 4? Would someone review it in that case?

Comment 194

[Bill Gianopoulos [:WG9s]]

I have created my own Linux and Windows builds including this patch and have found a couple of issues with it:

1:  It does not work as described in the summary.  Instead of not resizing until you move the mouse out of the tabbar, it does not resize the tabs unless you move the mouse out into the main content area.  Moving to the sidebar another toolbar, the titlebar or even entirely outside the window has no effect.  Oddly enough, even Minimizing to the taskbar and restoring does not result in the tabs resizing.

2.  If you have enough tabs open so that they overflow the tabbar and close a tab in the middle of the screen, it creates a space for a new tab to be added, but does not move one of the tabs that overflowed in to fill the space.  There is no logical reason to delaly doing this until the mouse moves off the tabbar because the space for the tab was already created anyway and adding the extra tab should not shift any of the existing tabs as long as they are all locked to the current size.  Oddly, even if you resize the window to be smaller so that more tabs overflow off, and then widen the window so the ones that overflowed because of the window resize are correctly restored, when you get back the original width, that one more tab that there is now space for will not display until you move the mouse to the content area.

Comment 195

[Bill Gianopoulos [:WG9s]]

Suggested fixes for above issues:

1:  to fix issue 1, I think you will need to define a container around tabbrowser-tabs and the new-tabs button and setup a mouseout event handler on it if you are doing a lock and no lock was already in effect, and when the event fires call_unlockTabSizing.  The reason for doing this rather than  setting up the handler on the tabbar is that in Linux, the menu button is on the tabbar and i think if you move the mouse out of the tab area to the menu button, the tabs should resize.  The reason for needing the container, is that if you just put the handler on the tabbrowser-tabs object, then if you close a tab and then try to target the new tab button it will move out from under your mouse.  BTW, don't bother asking me for help on how to make the event handler work, as I have never done this either.  I just think that is what needs to be done here.

2:  To fix issue number 2, I would just do softlocks and never hardlocks if the tabs have overflowed the tabbar.

Comment 196

[Patrick Walton (:pcwalton)]

(In reply to comment #195)
> setup a mouseout event handler on
> it if you are doing a lock and no lock was already in effect, and when the
> event fires call_unlockTabSizing.

Sigh. Originally this is what I did. It was changed due to objections that "mouseout is not reliable".

I think we should identify what's causing the problems with mouseout and fix them at the platform level instead of trying to work around it in the browser code, but that's neither here nor there. As a workaround I could try unlocking the tabs on either mouseout *or* mousemove, to try to catch as many situations as possible.

> 2:  To fix issue number 2, I would just do softlocks and never hardlocks if the
> tabs have overflowed the tabbar.

I don't think that works, though I'm happy to be proven wrong :) Instead I think we would need to change the way that hard lock works, either by changing which box it hammers in the size of or by introducing a new box.

Comment 197

[Patrick Walton (:pcwalton)]

I would like to leave (2) to a followup bug; I'm reasonably confident it can be fixed, but this patch has gotten big enough.

Also I think that the next version of this patch is going to have this behavior be preffed off by default, due to Firefox 4's schedule. I hope that this will increase the chances of getting a review.

Comment 198

[Brendan Eich [:brendan]]

Why does review depend on pref'ing this off? What's the hold-up? Haven't heard a peep from any of the reviewers who were solicited.

/be

Comment 199

[imradyurrad]

See Comment 191

Comment 200

[Frank Yan (:fryn)]

(In reply to comment #197)
> Also I think that the next version of this patch is going to have this behavior
> be preffed off by default, due to Firefox 4's schedule. I hope that this will
> increase the chances of getting a review.

I'd really prefer not start landing pref'd off code that is rather integral to an existing feature and not even a full feature itself.

Comment 201

[Brendan Eich [:brendan]]

My point in comment 198 was not to advocate pref'ing off -- I agree with comment 200 on that. It was to ask why review is not happening.

Re: comment 199, comment 191 would seem to encourage all of: getting review, getting the patch ready to land, analyzing risk more carefully, and of course testing harder. We can do some of those things without review and landing, but we'll need review sooner or later and sooner's better.

/be

Comment 202

[ithinc]

I suggest you guys fix the depended bug 606727, bug 622266 first.

Comment 203

[Patrick Walton (:pcwalton)]

(In reply to comment #202)
> I suggest you guys fix the depended bug 606727, bug 622266 first.

The current patch does not depend on these bugs being fixed.

Comment 204

[ithinc]

(In reply to comment #203)
> The current patch does not depend on these bugs being fixed.

I saw a new "fullyopen" attribute is added in your patch, which is similar to what bug 622266 is requesting.

Comment 205

[Bill Gianopoulos [:WG9s]]

(In reply to comment #196)
> > 2:  To fix issue number 2, I would just do softlocks and never hardlocks if the
> > tabs have overflowed the tabbar.
> 
> I don't think that works, though I'm happy to be proven wrong :) Instead I
> think we would need to change the way that hard lock works, either by changing
> which box it hammers in the size of or by introducing a new box.

It sort of works, because in the overflow case, the new tab that will slide in will be tame width as the tab you removed so it all works as expected.  The problem is, in the case where you remove the tab which will permit the overflow to no longer be required.  This results in the removal of the overflow indicators so the tabs all resize in that case.

The main issue i have with this case, is that the current patch here breaks a process that I use all the time in my day-to-day browsing.  I often run into a case where I know the first off tabbar tab is the one I want to open, but also notice that one of the displayed tabs is something i no longer need.  Without this patch i close the tab and slide the mouse to the right to click on the tab newly added back.  With this patch i need to close the tab, move the mouse off the tab bar and then go select the tab I want to display.

Comment 206

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 26.

Patch version 26 unlocks the tabs when the pointer leaves the tab strip.

Comment 207

[Patrick Walton (:pcwalton)]

(In reply to comment #206)
> Created attachment 510468 [details] [diff] [review]
> Proposed patch, version 26.
> 
> Patch version 26 unlocks the tabs when the pointer leaves the tab strip.

Via exiting the window, I mean.

Comment 208

[Bill Gianopoulos [:WG9s]]

In order to provide a method for more widespread testing of this patch, I am including it in my builds, which I make available at http://www.wg9s.com/mozilla/firefox/

The builds are Linux and Windows only as I don't have the capability to create OSX builds.

Comment 209

[Patrick Walton (:pcwalton)]

(In reply to comment #205)
> The main issue i have with this case, is that the current patch here breaks a
> process that I use all the time in my day-to-day browsing.  I often run into a
> case where I know the first off tabbar tab is the one I want to open, but also
> notice that one of the displayed tabs is something i no longer need.  Without
> this patch i close the tab and slide the mouse to the right to click on the tab
> newly added back.  With this patch i need to close the tab, move the mouse off
> the tab bar and then go select the tab I want to display.

I can't reproduce this; could you provide exact steps to reproduce?

Comment 210

[Bill Gianopoulos [:WG9s]]

(In reply to comment #209)
> (In reply to comment #205)
> > The main issue i have with this case, is that the current patch here breaks a
> > process that I use all the time in my day-to-day browsing.  I often run into a
> > case where I know the first off tabbar tab is the one I want to open, but also
> > notice that one of the displayed tabs is something i no longer need.  Without
> > this patch i close the tab and slide the mouse to the right to click on the tab
> > newly added back.  With this patch i need to close the tab, move the mouse off
> > the tab bar and then go select the tab I want to display.
> 
> I can't reproduce this; could you provide exact steps to reproduce?

OK start with 2 tabs open the one on the right being the one you want to be able to open later.
Then create a bunch of new tabs between the 2 you currently have open like by ctrl-clicking bookmarks or something until the right hand tab you planned to open later is the first tab overflowed off the tabbar.

Now middle click on one of your dummy added tabs int the middle of the tabbar and try to go over to the right of the tabbar to open the tab that was one tab overflowed, but you can;t because it is not on the tabbar.  Without this patch that would have worked.

Comment 211

[ithinc]

One problem, steps to reproduce:
1. Open enough tabs to make the tab bar overflown.
2. Close tabs from the 2nd last tab one by one.

When the remaining tabs have grown to the max-width of 250px, they won't move to the mouse pointer, but the most left tab is still out of the tab bar.

Comment 212

[Bill Gianopoulos [:WG9s]]

OK. On Friday, I am either going to remove this patch entirely form those I include in my builds, or decide to leave it in and include the patch that I mentioned in  comment 195 to fix issue #2 (this will also fix the issue mentioned in comment 211)

The reason for this is simple.  I include fixes, on occasion, to test pending patches, but the builds I create are really what I run for my own use.  The inclusion of the current patch on this bug really does not meet that requirement.  Without the patch to fix my issue I really would rather run builds without this fix at all.

Comment 213

[Patrick Walton (:pcwalton)]

(In reply to comment #210)
> (In reply to comment #209)
> > (In reply to comment #205)
> > > The main issue i have with this case, is that the current patch here breaks a
> > > process that I use all the time in my day-to-day browsing.  I often run into a
> > > case where I know the first off tabbar tab is the one I want to open, but also
> > > notice that one of the displayed tabs is something i no longer need.  Without
> > > this patch i close the tab and slide the mouse to the right to click on the tab
> > > newly added back.  With this patch i need to close the tab, move the mouse off
> > > the tab bar and then go select the tab I want to display.
> > 
> > I can't reproduce this; could you provide exact steps to reproduce?
> 
> OK start with 2 tabs open the one on the right being the one you want to be
> able to open later.
> Then create a bunch of new tabs between the 2 you currently have open like by
> ctrl-clicking bookmarks or something until the right hand tab you planned to
> open later is the first tab overflowed off the tabbar.
> 
> Now middle click on one of your dummy added tabs int the middle of the tabbar
> and try to go over to the right of the tabbar to open the tab that was one tab
> overflowed, but you can;t because it is not on the tabbar.  Without this patch
> that would have worked.

I still can't reproduce this.

Comment 214

[Patrick Walton (:pcwalton)]

Could I get a review on this?

Comment 215

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

Comment on attachment 510468 [details] [diff] [review]
Proposed patch, version 26.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="removeTab">

>+            var isAtEnd = false;
>+            for (var i = this.tabs.length - 1; i >= 0; i--) {
>+              if (!this.tabs[i].hidden) {
>+                isAtEnd = aTab == this.tabs[i];
>+                break;
>+              }
>+            }

Could this not just compare aTab to this.visibleTabs.slice(-1)?

>+            // Lock the tab sizing until the pointer leaves the tab strip if
>+            // the tab was closed by mouse, unless transitions haven't finished
>+            // yet (so that our size calculations will be accurate).
>+            if (byMouse && !isClosing && isFullyOpen) {

Does this also want to check !aTab.hidden? In theory byMouse will catch that case already, but a sanity check wouldn't hurt given that this is a public API and we can't necessarily rely on byMouse being passed properly.

Should this also check whether aTab is an app tab? I think we never want to do this for app tabs, right?

>+          let spacer = this._closingTabsSpacer;
>+          spacer.addEventListener("transitionend",
>+                                  this.adjustTabstrip.bind(this), false);

I think this needs to be removed, per our in-person discussion Wednesday.

>+      <!-- Equal to one of the _tabSizingLocked* constants below. -->
>+      <field name="_tabSizingLocked">false</field>
>+
>+      <field name="_tabSizingUnlocked" readonly="true">0</field>
>+      <field name="_tabSizingLockedSoft" readonly="true">1</field>
>+      <field name="_tabSizingLockedHard" readonly="true">2</field>
>+
>+      <property name="_innerBox">

scrollBoxInnerBox?

>+      <!-- Makes the tabs and the tab strip no longer resize. The given tab
>+           parameter is the last tab that was closed. -->
>+      <method name="_hardLockTabSizing">
>+        <parameter name="aTab"/>

Does this need to take aTab? Can't it just use this.visibleTabs[this._numPinnedTabs]'s width?
("the last tab that was closed" is somewhat confusing - it's actually "the tab we're closing now", isn't it?)

>+          // Unlock the tabs when the cursor enters the tab browser or leaves
>+          // the window.
>+          this.tabbrowser.addEventListener("mousemove", this, false);
>+          window.addEventListener("mouseout", this, false);

We may need to tweak this behavior - don't we actually want "when the mouse leaves the tabstrip"?

>+      <method name="_unlockTabSizing">

>+          switch (this._tabSizingLocked) {

>+          case this._tabSizingLockedHard:
>+            this._softLockTabSizing();

You only really want the "make the tabs flexible" part of this, right? Seems like it would be worth splitting off into its own helper to avoid doing the spacer width calculation work that will just be overridden below.

>+            case "mousemove":
>+              if (this._tabSizingLocked != this._tabSizingUnlocked)

These checks seems redundant (already done within _unlockTabSizing) - also applies to mouseout.

>+            case "mouseout": {
>+              // If the "related target" (the node to which the pointer went)
>+              // is not a child of the current document, the mouse just left
>+              // the window.
>+              let relatedTarget = aEvent.relatedTarget;
>+              if (relatedTarget && relatedTarget.ownerDocument == document)
>+                break;

Does mouseout not fire on the window itself? I.e. can you not just check event.target = window?

>       <method name="_handleNewTab">

>+          tab.setAttribute("fullyopen", "true");

This seems somewhat hacky - I think this should just be a JS prop, if it's needed at all. I'd like to hear dao's thoughts.

>diff --git a/browser/base/content/test/browser_bug465086.js b/browser/base/content/test/browser_bug465086.js

>+function addTabs() {

>+  win.setTimeout(closeTabInMiddle, 0);

Use executeSoon (here and anywhere else you're using setTimeout)

>+function closeTabInMiddle() {
>+  // Close the third tab, and wait for the animation to complete.

Why pass {animate: true} and rely on transitionend rather than just not passing that and using TabClose? Doesn't seem like it makes the test any more realistic, and it does make it somewhat more fragile. I think it would remove the need for a few of the setTimeouts too.

Comment 216

[Patrick Walton (:pcwalton)]

(In reply to comment #215)
> Should this also check whether aTab is an app tab? I think we never want to do
> this for app tabs, right?

I think byMouse catches this, since app tabs have no close button, but your point about this being a public API makes sense.

Comment 217

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

Apps tabs can still be closed by middle click, though.

Comment 218

[Patrick Walton (:pcwalton)]

(In reply to comment #215)
> >+          // Unlock the tabs when the cursor enters the tab browser or leaves
> >+          // the window.
> >+          this.tabbrowser.addEventListener("mousemove", this, false);
> >+          window.addEventListener("mouseout", this, false);
> 
> We may need to tweak this behavior - don't we actually want "when the mouse
> leaves the tabstrip"?

Originally, that was what it did, but per some comments in this bug I switched it to only resize when the cursor leaves the navigation area at the top of the window. It makes it easier to aim the mouse pointer without having the tabs resize on you.

Comment 219

[Patrick Walton (:pcwalton)]

(In reply to comment #215)
> >+            case "mouseout": {
> >+              // If the "related target" (the node to which the pointer went)
> >+              // is not a child of the current document, the mouse just left
> >+              // the window.
> >+              let relatedTarget = aEvent.relatedTarget;
> >+              if (relatedTarget && relatedTarget.ownerDocument == document)
> >+                break;
> 
> Does mouseout not fire on the window itself? I.e. can you not just check
> event.target = window?

The target is the element that the mouse pointer left (usually tabbrowser), unfortunately.

Comment 220

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 27.

Patch version 27 addresses review comments.

Comment 221

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

Comment on attachment 523471 [details] [diff] [review]
Proposed patch, version 27.

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="removeTab">

>+            var isFullyOpen = aTab.fullyOpen;

nit: this doesn't really need a separate variable.

>+            case "mousemove":
>+              if (this._tabSizingLocked != this._tabSizingUnlocked)

>+            case "mouseout": {

>+              if (this._tabSizingLocked != this._tabSizingUnlocked)

Again, I think you should remove these redundant checks.

>diff --git a/browser/base/content/test/browser_bug465086.js b/browser/base/content/test/browser_bug465086.js

>+function addTabs() {

>+  executeSoon(closeTabInMiddle);

Is this really needed? I don't see why it would be, offhand. All of the executeSoons in this test seem somewhat suspect. Can you either explain or remove them all?

>+function closeTabInMiddle() {

>+  tabs[2].addEventListener("transitionend", closeTabAtEnd, false);
>+  win.gBrowser.removeTab(tabs[2], { byMouse: true });
>+
>+  closeTabAtEnd();

I don't think you want closeTabAtEnd to be called both as an event handler and explicitly...

r=me, but I think the test needs cleanup.

Comment 222

[Frank Yan (:fryn)]

Comment on attachment 523471 [details] [diff] [review]
Proposed patch, version 27.

Perhaps, this has taken me to long, but nevertheless, I would like to voice my concerns about this patch.

1) It is unnecessary to have a distinction between hard- and soft-locking and the change to the binding. The code's calculations should not have to be this subtle. As I am one of the main people that have been and will be regularly working with and modifying these files, I find these changes to be confusing and non-intuitive.

2) This patch is now longer and more complex than my patch. The purpose of revising this patch from v.10 was to reduce risk and make things simpler, and that has failed.

3) The behavior of the patch in dealing with underflow involves enlarging the tabs, which doesn't make sense, since that results in only a few tabs being visible, each 200 pixels wide, even though scroll buttons are still visible and many tabs are off-screen. (To reproduce, open many tabs and start closing them with the mouse from the second-to-last tab.) He noted that this can be handled in a followup, but the person likely to be working on this code in the future is me, and I'd rather not have to have work around it, if there is a better solution, and I have one. (I will upload an unbitrotted, improved version of my patch now.)

Comment 223

[Frank Yan (:fryn)]

Attachment: patch v28 - unbitrotted, improved version of my original approach

Patrick told me that the primary issue he had with my patch v.10 was that it didn't work properly with the transitions for me. I have been unable to reproduce that problem in this unbitrotted patch. If there are problems, do let me know.

This patch also deals with underflow more elegantly than patch version 27.

Another reason that we might go with this approach instead of patch version 27 is that the next code likely to be landed on tabbrowser.xml is my patch for tab drag-and-drop animations, and that patch is more easily rebased on top of this one.

Comment 224

[Frank Yan (:fryn)]

Comment on attachment 523471 [details] [diff] [review]
Proposed patch, version 27.

I don't remember if I told Patrick this before, but I find the test to be extremely artificial.

The feature we are implementing here is:
If you click a tab to close it, the tab that will be selected next should have its close button move to where the closed tab's close button was. Then you can just click again to close that tab.

The test should simulate this exact process:
1) Find a tab's close button.
2) Simulate a mouse event at that location.
3) Wait a half a second or so for the tab closing animation to complete.
4) Repeat steps 2 and 3, until tabs expand to their maximum width.

Comment 225

[ithinc]

(In reply to comment #223)
> Created attachment 524295 [details] [diff] [review]
> unbitrotted, improved version of my original approach
> 
> Patrick told me that the primary issue he had with my patch v.10 was that it
> didn't work properly with the transitions for me. I have been unable to
> reproduce that problem in this unbitrotted patch. If there are problems, do let
> me know.

To set MozTransitionDuration to "0s" should be working. Maybe better to set MozTransition to "none"?
 
> This patch also deals with underflow more elegantly than patch version 27.

Your patch seems failed to handle the "overflow and !_scrollButtonDown.disabled" case?

Comment 226

[Justin Dolske [:Dolske]]

Just a followup to comment 222 - comment 224, for the record... 

Frank, Patrick, Gavin, and I sat down (since we're all in MV this week) to discuss the issues with the patches here. Long story short, Patrick's concerns about the original approach (complexity / animation jankyness) have been addressed in later version from Frank. Gavin ran through a review of Frank's current patch, and although there were a number of nits to fix up we all agreed to go ahead with Frank's flavor of this patch.

Despite the non-optimal long and bumpy path to getting this feature landed, we do get the benefit of a well-discussed patch with tons of edge-cases considered. :)

Comment 227

[Frank Yan (:fryn)]

Attachment: patch v29

AFAICT ATM, it's ready for review and landing.
I've tested this on OS X 10.6, Windows XP, Windows 7, and Ubuntu 10.10.

I pushed patch v28 to tryserver, and it came up all green:
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=dc4af5e171cf

This final version is running on tryserver:
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=40bd7086613b
I'll link to builds when they are available.

Comment 228

[ithinc]

Comment on attachment 524805 [details] [diff] [review]
patch v29

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>       <handler event="overflow"><![CDATA[
>-         if (event.detail == 0)
>-           return; // Ignore vertical events
>-
>-         var tabs = document.getBindingParent(this);
>-         tabs.setAttribute("overflow", "true");
>-         tabs._positionPinnedTabs();
>+        if (event.detail == 0)
>+          return; // Ignore vertical events
>+
>+        var tabs = document.getBindingParent(this);
>+        tabs.setAttribute("overflow", "true");
>+        tabs._positionPinnedTabs();
>       ]]></handler>

What changed? Trailing spaces?

>+            for (let i = numPinnedTabs; i < tabs.length; i++) {
>+              let tab = tabs[i];
>+              tab.style.maxWidth = tabWidth;

.setProperty("max-width", tabWidth, "important")?

>+              // If the tabs need to stay the same width, we need to skip the animation
>+              // to avoid tab expansion jank.
>+              if (!resize) {
>+                tab.style.MozTransitionDuration = "0s";

.MozTransition = "none"? "important"?


>+          let spacer = this._closingTabsSpacer;
>+          spacer.style.width = parseInt(spacer.style.width) + pixels + "px";

parseFloat?

>+      <method name="_unlockTabSizing">
>+        <body><![CDATA[

Add a _tabSizingLocked field and check it here?

>+            case "mousemove":
>+              if (document.getElementById("tabContextMenu").state != "open")
>+                this._unlockTabSizing();

No check for mousemove distance? Chrome doesn't do so.

>diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css
> .tabs-newtab-button {
>-  width: 30px;
>+  width: 28px;
>+}
>+
>+#TabsToolbar > #new-tab-button {
>+  width: 26px;
> }

How is this related?

Comment 229

[Dão Gottwald [:dao]]

> >diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css
> > .tabs-newtab-button {
> >-  width: 30px;
> >+  width: 28px;
> >+}
> >+
> >+#TabsToolbar > #new-tab-button {
> >+  width: 26px;
> > }
> 
> How is this related?

Yes, how is it?

You don't seem to be setting mFullyOpen on the first tab. Also, use "_" instead of "m" for pseudo-private stuff.

Comment 230

[Frank Yan (:fryn)]

(In reply to comment #228)
> Comment on attachment 524805 [details] [diff] [review]
> patch v29
> 
> What changed? Trailing spaces?

Indentation was wrong.

> >+            for (let i = numPinnedTabs; i < tabs.length; i++) {
> >+              let tab = tabs[i];
> >+              tab.style.maxWidth = tabWidth;
> 
> .setProperty("max-width", tabWidth, "important")?

We don't want !important here.

> .MozTransition = "none"?

> parseFloat?

Fixed in next iteration; thanks.

> Add a _tabSizingLocked field and check it here?

I don't think this is necessary. There are already two boolean checks.

> >+            case "mousemove":
> >+              if (document.getElementById("tabContextMenu").state != "open")
> >+                this._unlockTabSizing();
> 
> No check for mousemove distance? Chrome doesn't do so.

What do you mean? If you are referring unlocking immediately upon leaving the tab bar: Gavin and Patrick are of the opinion that the sizing should not unlock until the cursor leaves the entire toolbar region. Limi and I are of the opinion that it should unlock immediately upon leaving the tab strip region. We are going with the former behavior for now.

> >diff --git a/browser/themes/winstripe/browser/browser.css b/browser/themes/winstripe/browser/browser.css
> > .tabs-newtab-button {
> >-  width: 30px;
> >+  width: 28px;
> >+}
> >+
> >+#TabsToolbar > #new-tab-button {
> >+  width: 26px;
> > }
> 
> How is this related?

Stephen intended for the new tab button to take up the same amount of space regardless of overflow state, so that's what this code does. Doing this also simplifies the calculation of how much the spacer should expand when the tab bar underflows, which is why it's in this patch. It's still 1px off on OS X, but that will be fixed when we update the tab theming.

Comment 231

[Frank Yan (:fryn)]

Attachment: patch v30

(In reply to comment #229)
> You don't seem to be setting mFullyOpen on the first tab. Also, use "_" instead
> of "m" for pseudo-private stuff.

Fixed; thanks.

Comment 232

[ithinc]

(In reply to comment #230)
> > .setProperty("max-width", tabWidth, "important")?
> 
> We don't want !important here.

I'm afraid you will get lots of complaints when landing it, for people seem to like using "important" in userChrome.css.

> What do you mean? If you are referring unlocking immediately upon leaving the
> tab bar: Gavin and Patrick are of the opinion that the sizing should not unlock
> until the cursor leaves the entire toolbar region. Limi and I are of the
> opinion that it should unlock immediately upon leaving the tab strip region. We
> are going with the former behavior for now.

I suggest the sizing should not unlock when the cursor is in the region of tab strip +/- height * 0.5 (or 1.0). The current behavior should still have some difficulty when tabs on bottom.

> Stephen intended for the new tab button to take up the same amount of space
> regardless of overflow state, so that's what this code does. Doing this also
> simplifies the calculation of how much the spacer should expand when the tab
> bar underflows, which is why it's in this patch. It's still 1px off on OS X,
> but that will be fixed when we update the tab theming.

Thanks for the explanation.

Comment 233

[Frank Yan (:fryn)]

(In reply to comment #232)
> I'm afraid you will get lots of complaints when landing it, for people seem to
> like using "important" in userChrome.css.

We should not make our code harder to maintain and eschew best practices to accommodate a small minority's userChrome.css tweaks, and users should not expect this either.

Comment 234

[ithinc]

Comment on attachment 524885 [details] [diff] [review]
patch v30

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>+          if (this.getAttribute("overflow") && // OVERFLOW MODE ONLY
>+              // If the tab has an owner that will become the active tab, the owner will
>+              // be to the left of it, so we actually want the left tab to slide over.
>+              // This can't be done as easily in non-overflow mode, so we don't bother.
>+              !aTab.owner && !isEndTab && this.mTabstrip._scrollButtonDown.disabled)
>+            this._expandSpacerBy(tabWidth);

Is there any plan to handle overflow case with _scrollButtonDown enabled?

Comment 235

[Frank Yan (:fryn)]

(In reply to comment #234)
> Is there any plan to handle overflow case with _scrollButtonDown enabled?

It already worked just fine without this patch in most cases with _scrollButtonDown enabled.
The only two cases where it fails slightly can be addressed in a followup:
1) Closing the tab causes _scrollButtonDown to become disabled, since there was less than one tab width of overflow on the right side.
2) The tab has an owner, so closing the tab selects a tab to the left, instead of to the right.

Comment 236

[matthias koplenig [:metasieben]]

while the patch is awesome, i'm wondering if this has been addressed already…

http://www.screenr.com/hwe

*first try:
(1) opening 20 tabs
(2) closing tabs starting at the end of the tab-bar(second to last tab)
(3) max 16 tabs can be closed.

*second try:
(1) opening 20 tabs
(2) closing tabs starting at the beginning of the (visible) tab-bar
(3) 19 tabs can be closed.

Comment 237

[imradyurrad]

(In reply to comment #236)

That's how the feature is supposed to behave.

Comment 238

[matthias koplenig [:metasieben]]

(In reply to comment #237)
> That's how the feature is supposed to behave.

it's true that this takes care of the parity-chrome issue, however imho it should be possible to close the same amount of tabs in both cases. 

…after re-reading the comments, this will probably be handled in a follow-up bug, right?

Comment 239

[Frank Yan (:fryn)]

(In reply to comment #238)

No. The feature you are requesting requires one of two behaviors, both of which we've tried and found to be awkward and to create states that the user would never see otherwise:
1) Temporarily expand the tabs beyond their usual max-width.
2) Insert an additional spacer on the left side (in LTR mode).

> it's true that this takes care of the parity-chrome issue

This patch goes beyond parity-chrome: it also provides elegant handling when there are a very large number of tabs.

> imho it should be possible to close the same amount of tabs in both cases.

With or without my patch, it is always possible to close all the tabs. Ctrl+W/Cmd+W, or just move the cursor. Easy.

Comment 240

[matthias koplenig [:metasieben]]

to add a little more ot-bugspam. *sorry*

i'm aware of cmd|ctrl+w, and moreso that my _request_ is an edge-case in itself.

again, *great* work guys. ;)

Comment 241

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

Comment on attachment 524885 [details] [diff] [review]
patch v30

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+      <handler event="underflow" phase="capturing"><![CDATA[

>+        if (tabs._lastTabClosedByMouse)
>+          tabs._expandSpacerBy(this._scrollButtonDown.clientWidth);

Shouldn't this be checking _usingClosingTabsSpacer ? Then I think you can get rid of _lastTabClosedByMouse.

>       <method name="removeTab">

>+            if (!aTab.pinned && byMouse)
>+              this.tabContainer._lockTabSizing(aTab);

&& !aTab.hidden, as a sanity check?


>+      <method name="_lockTabSizing">

>+          if (!this._tabDefaultMaxWidth) {

Having this lazy getter here is somewhat confusing - you could make it a property

>+            let tab = tabs[tabs.length-1];
>+            if (tab.pinned || !tab._fullyOpen)

I don't think tab.pinned can be true, because there is at least one unpinned tab (aTab). Can you mvoe the _fullyOpen check to removeTab and just use aTab here?

>+            this._tabDefaultMaxWidth =
>+              parseFloat(window.getComputedStyle(tab, null).maxWidth);

Don't need the second "null" param now, IIRC.
 
>+              if (tabWidth > this._tabDefaultMaxWidth)
>+                tabWidth = this._tabDefaultMaxWidth;

It would be nice to do one loop of resetting the maxWidth once we hit this case, and then avoid doing more maxWidth-setting work from then on for subsequent closes, until tab sizing is unlocked again. I guess that's probably best done in a followup though.

>+            for (let i = numPinned; i < tabs.length; i++) {
>+              let tab = tabs[i];
>+              tab.style.maxWidth = tabWidth;
>+              if (!resize) { // keep tabs the same width
>+                tab.style.MozTransition = "none";
>+                tab.clientTop; // force style resolution to skip animation
>+                tab.style.MozTransition = "";
>+              }

File a followup for understanding this, mention the bug # here? (discussed this IRL, will also talk to dbaron about a mimized testcase that demonstrates the problem)

This method's logic seems like it would be clearer as:

if (this.getAttribute("overflow") == "true") {
  // comment explaining reasoning for this check
  if (isEndTab && !this._hasTabTempMaxWidth)
    return;

  // Force tabs to stay the same width, unless we're closing the last tab, in which
  // case we need to let them expand just enough so that the overal tabbar width is
  // the same.

  //...
} else {
  // Don't need to do anything if we're in overflow mode and aren't scrolled all
  // the way to the right, or if we're closing the last tab.
  if (isEndTab || !this.mTabstrip._scrollButtonDown.disabled)
    return;

  // comment explaining reasoning for this check
  if (aTab.owner)
    return;

  this._expandSpacerBy();
}
this.tabbrowser.addEventListener("mousemove", this, false);
window.addEventListener("mouseout", this, false);

"isEndTab" and "resize" seem redundant, since they're always the same value in this block. Just use isEndTab, and rely on the comment to clarify the different behavior?

>+      <method name="_expandSpacerBy">

>+          this.tabbrowser.addEventListener("mousemove", this, false);
>+          window.addEventListener("mouseout", this, false);

Would need to remove these given the refactoring above.

>       <method name="_positionPinnedTabs">
>+        <parameter name="dueToUnderflow"/>
>         <body><![CDATA[
>+          if (!dueToUnderflow)
>+            this._unlockTabSizing();

Why is this needed?

Comment 242

[Frank Yan (:fryn)]

Attachment: patch v31

(In reply to comment #241)
> >       <method name="_positionPinnedTabs">
> >+        <parameter name="dueToUnderflow"/>
> >         <body><![CDATA[
> >+          if (!dueToUnderflow)
> >+            this._unlockTabSizing();
> 
> Why is this needed?

In almost every case that we position pinned tabs, we also need unlock tab sizing, so I included _unLockTabSizing() inside it. In the sole case of underflow, we actually want to keep the sizing locked, so that it is easy to continue closing tabs. This asynchronous interaction with underflow is also why we need _lastTabClosedByMouse.

All review comments have been addressed. I also removed the need for the "!important" rule in favor of clearing style.maxWidth in removeTab() as discussed in-person and on IRC.

Followup bugs will be filed shortly.

Comment 243

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

(In reply to comment #242)
> In almost every case that we position pinned tabs, we also need unlock tab
> sizing

Which cases can come up in practice?

- the calls in pinTab/unpinTab seem unlikely to occur, but could be covered by adding a call to unlockTabSizing in moveTabTo
- the call in _endRemoveTab doesn't need to unlock tab sizing (will have already been unlocked by the apptab removal)
- overflow handler doesn't need it (only possible on addTab/resize, right?)

So I think instead you could just add a call to unlockTabSizing inside moveTabTo?

Comment 244

[Frank Yan (:fryn)]

Attachment: patch v32

(In reply to comment #243)
> - the calls in pinTab/unpinTab seem unlikely to occur

We prevent unlocking while the tab context menu is open, so this can occur.

> - the call in _endRemoveTab doesn't need to unlock tab sizing (will have
> already been unlocked by the apptab removal)
> - overflow handler doesn't need it (only possible on addTab/resize, right?)

You're right.

> So I think instead you could just add a call to unlockTabSizing inside
> moveTabTo?

I think it's a bad idea to unlock tab sizing while a user is rearranging tabs.
I just added a call to unlockTabSizing inside pinTab and unpinTab instead.

I also included a tiny CSS fix for the closing tabs spacer so that double-clicking empty space of the tab bar still opens a new tab.

Comment 245

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

Comment on attachment 525275 [details] [diff] [review]
patch v32

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>       <method name="removeTab">

>+            aTab.style.maxWidth = "";

Add a comment explaining why this is needed?

>+      <method name="_lockTabSizing">

>+            this._tabDefaultMaxWidth =
>+              parseFloat(window.getComputedStyle(aTab, null).maxWidth);

Lose the "null" parameter (it's no longer needed)

>+          this._lastTabClosedByMouse = true;

This is potentially confusing - if we hit this, but then short-circuit below such that we don't add the listeners, we'll potentially leave this flag set indefinitely. This can happen in practice if you e.g. get into overflow mode, then close the last tab with the mouse (staying in overflow mode), and then resize the window until an underflow occurs. It's minor but we should fix it somehow (in a followup if needed).

>+              if (!isEndTab) { // keep tabs the same width
>+                tab.style.MozTransition = "none";
>+                tab.clientTop; // force style resolution to skip animation
>+                tab.style.MozTransition = "";

Reference a bug # here.

It would be good to have Dao also look over this, but I suspect he already has (and/or will).

r=me with those addressed.

Comment 246

[Frank Yan (:fryn)]

Attachment: hg export of patch v32 + comments addressed [r=gavin]

(In reply to comment #245)
> Comment on attachment 525275 [details] [diff] [review]
> patch v32
> 
> >diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
> 
> >       <method name="removeTab">
> 
> >+            aTab.style.maxWidth = "";
> 
> Add a comment explaining why this is needed?

Added.

> >+      <method name="_lockTabSizing">
> 
> >+            this._tabDefaultMaxWidth =
> >+              parseFloat(window.getComputedStyle(aTab, null).maxWidth);
> 
> Lose the "null" parameter (it's no longer needed)

Lost.

> >+          this._lastTabClosedByMouse = true;
> 
> This is potentially confusing - if we hit this, but then short-circuit below
> such that we don't add the listeners, we'll potentially leave this flag set
> indefinitely. This can happen in practice if you e.g. get into overflow mode,
> then close the last tab with the mouse (staying in overflow mode), and then
> resize the window until an underflow occurs. It's minor but we should fix it
> somehow (in a followup if needed).

Fixed by setting it to false after a timeout in _endRemoveTab().
Better approaches can be proposed in followups.

> >+              if (!isEndTab) { // keep tabs the same width
> >+                tab.style.MozTransition = "none";
> >+                tab.clientTop; // force style resolution to skip animation
> >+                tab.style.MozTransition = "";
> 
> Reference a bug # here.

Bug 649247 referenced.

Comment 247

[LegNeato]

http://hg.mozilla.org/mozilla-central/rev/8a67aa03761e

Comment 248

[Siddhartha Dugar [:sdrocking]]

When I move the pointer to the screen edge just above the gap between the tabs, resizing does occur. Please fix this.

Comment 249

[Philipp von Weitershausen [:philikon]]

(In reply to comment #248)
> When I move the pointer to the screen edge just above the gap between the tabs,
> resizing does occur. Please fix this.

Please file a new bug.

Comment 250

[bogas04]

Attachment: Tab scroll should adjust itself while closing tabs

Comment 251

[bogas04]

(In reply to comment #250)
> Created attachment 525653 [details]
> Tab scroll should adjust itself while closing tabs

If we are in tab scroll mode , the scrolled tabs are left un-scrolled and only a small portion of tabs is visible while deleting them , if we wish to delete the rest of the tabs which are on other end of tab scroller , we have to remove cursor from the tab bar , and then again close desired tabs , which i consider as serious problem with this feature.

Chrome doesn't scroll tabs , so it doesn't have to deal with it

Comment 252

[Siddhartha Dugar [:sdrocking]]

shouldn't resizing occur when user moves to app tabs?

Comment 253

[Can]

Tab resize occurs immediately when cursor leaves tab-bar, wherever pointer points then.

In Chrome, tab resize does not occur until cursor leaves tab-bar and url-bar both.

Shouldnt we do that like Chrome does?

Comment 254

[Daniel]

(In reply to comment #253)
> Tab resize occurs immediately when cursor leaves tab-bar, wherever pointer
> points then.
> 
> In Chrome, tab resize does not occur until cursor leaves tab-bar and url-bar
> both.
> 
> Shouldnt we do that like Chrome does?

we do?

the tabs don't resize when hovering the url-bar over here (win 7) and I think that is the intended behavior.

Comment 255

[Can]

(In reply to comment #254)
> (In reply to comment #253)
> > Tab resize occurs immediately when cursor leaves tab-bar, wherever pointer
> > points then.
> > 
> > In Chrome, tab resize does not occur until cursor leaves tab-bar and url-bar
> > both.
> > 
> > Shouldnt we do that like Chrome does?
> 
> we do?
> 
> the tabs don't resize when hovering the url-bar over here (win 7) and I think
> that is the intended behavior.

im using win7 too and latest aurora, tabs resize immediately after i leave tab-bar. I thought maybe an add-on caused that and restarted with all add-ons disabled, doesnt change the issue.

Comment 256

[Can]

(In reply to comment #255)
> (In reply to comment #254)
> > (In reply to comment #253)
> > > Tab resize occurs immediately when cursor leaves tab-bar, wherever pointer
> > > points then.
> > > 
> > > In Chrome, tab resize does not occur until cursor leaves tab-bar and url-bar
> > > both.
> > > 
> > > Shouldnt we do that like Chrome does?
> > 
> > we do?
> > 
> > the tabs don't resize when hovering the url-bar over here (win 7) and I think
> > that is the intended behavior.
> 
> im using win7 too and latest aurora, tabs resize immediately after i leave
> tab-bar. I thought maybe an add-on caused that and restarted with all add-ons
> disabled, doesnt change the issue.

i noticed that when i use default theme , tab resize does not occur until pointer leaves tab-bar and url-bar both, but when i use a persona or a theme, tab resize occurs when i only leave tab-bar.

Comment 257

[Daniel]

(In reply to comment #256)
> (In reply to comment #255)
> > (In reply to comment #254)
> > > (In reply to comment #253)
> > > > Tab resize occurs immediately when cursor leaves tab-bar, wherever pointer
> > > > points then.
> > > > 
> > > > In Chrome, tab resize does not occur until cursor leaves tab-bar and url-bar
> > > > both.
> > > > 
> > > > Shouldnt we do that like Chrome does?
> > > 
> > > we do?
> > > 
> > > the tabs don't resize when hovering the url-bar over here (win 7) and I think
> > > that is the intended behavior.
> > 
> > im using win7 too and latest aurora, tabs resize immediately after i leave
> > tab-bar. I thought maybe an add-on caused that and restarted with all add-ons
> > disabled, doesnt change the issue.
> 
> i noticed that when i use default theme , tab resize does not occur until
> pointer leaves tab-bar and url-bar both, but when i use a persona or a theme,
> tab resize occurs when i only leave tab-bar.

I can confirm, on both aurora and nightly...

follow up bug?

Comment 258

[Timothy Kross]

(In reply to comment #252)
> shouldn't resizing occur when user moves to app tabs?

Hmm, this is a tricky case. The app tabs are a static part of the UI, so it would make sense to resize when you hover over to them. But on the other hand, they are "just tabs" and it would add unnecessary differentiation between app tabs and normal tabs. I don't think we should resize when the mouse moves to app tabs.

Comment 259

[Siddhartha Dugar [:sdrocking]]

Tabs should not animate while resizing when "browser.tabs.animate" is set to false.

Comment 260

[Justin Dolske [:Dolske]]

This bug is fixed.

Please file new bugs for any remaining problems / suggestions, instead of commenting here. Otherwise they're likely to be missed.

Comment 261

[Gabriela [:gaby2300]]

Verified fixed in:
Mozilla/5.0 (Windows NT 6.1; rv:5.0a2) Gecko/20110513 Firefox/5.0a2

Comment 263

[firefoxqa]

I could notice that this feature has been implemented in Firefox 5; I must admit that I initially thought of a bug or performance issue when I first saw it in Google Chrome and now FF5 -- now, maybe it is because I use the middle button to close tabs, but I don't really find it convenient and would prefer all tabs to be instantly resized when one is closed, just as in FF previous versions.
Could anyone please let us know if there is an easy way to disable this new feature in FF5? (I couldn't see anything in the options or about:config so far)
Thanks in advance!

Comment 264

[DuMOHsmol]

Yes, I also want to know, if this feature can be disabled.

Comment 266

[Mark Straver]

Comment 263 and comment 264
I agree - Show us how to immediately have them resize? I prefer to have as much space for the tab labels as possible at all times, instead of wasting screen real estate on it.

Comment 267

[Asa Dotzler [:asa]]

This bug is resolved and Bugzilla is not a support forum. Please take your comments to the proper forum. Thank you.

Comment 268

[:aceman]

Probably better to file a bug for making it configurable.

Comment 269

[Mark Straver]

(In reply to comment #268)
> Probably better to file a bug for making it configurable.

OK, done so. Bug 667607

@Asa, I should probably have worded myself differently. Support forums wouldn't offer a solution to something that is hard-coded in this bug.

Comment 270

[Dorus Peelen]

I've opened Bug 668593 with my proposal on how to make tab closing a little nicer, and display more tab label during the process. Enjoy.

Comment 271

[zerotype]

I've filed Bug 669318 to deal with the fact that the tab resizing animation occurs even with browser.tabs.animate set to false.

Comment 272

[bogas04]

changing tab width using CSS (Bug 574654) now breaks this functionality

Comment 273

[rob64rock]

(In reply to bogas04 from comment #272)
> changing tab width using CSS (Bug 574654) now breaks this functionality

I also confirm...


(In reply to firefoxqa from comment #263)

(In reply to DuMOHsmol from comment #264)

(In reply to Mark Straver from comment #266)

Read Bug Comment:
https://bugzilla.mozilla.org/667607#c19

Comment 274

[Cork]

First commenting on a fixed and checked-in bug is normally not successful, as people will have moved on to new bugs. So looking in blocked bugs and search/file new ones is the way it should go.

The problem should have been fixed by bug 658729, and should (if my calculations is correct) have been included in firefox 6.

Comment 275

[u424036]

Dear Mozilla developers,

Can you please at least add an "about:config" option to turn off this utter **** and return to the pre-5.0 tab closing behavior.

Immediately after upgrading to 5.0 and noticing this **** I was already going to file it as a bug. But no, turns out it's the new great feature which Chrome has(?), so it must be totally the right thing to do!

Is there an official 'main' bug(number?) already, to track the request to have this removed or made configurable?

Comment 276

[nox]

I completely agree with Roman Mamedov. What the ****? I'm not using Chrome because I hate the way Google imposes stuff on users. Firefox has (had) a very very big advantage over other browsers and that's why most of the Firefox users are "loyal". That advantage is customizability. If you intend to take this away from users for the sake of imitating Chrome, then **** Chrome and **** you. What the **** is this **** feature doing on a bug tracking page anyway?

Comment 277

[Marco Castelluccio [:marco]]

You're breaking one or more bugzilla rules: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

If you want an option to alter this behaviour, please file a new bug.