Closed Bug 465086 Opened 16 years ago Closed 14 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 5
Tracking Status
blocking2.0 --- -
status2.0 --- wanted

People

(Reporter: matthias.winkelmann+mozbugs, Assigned: fryn)

References

(Depends on 6 open bugs, Blocks 2 open bugs)

Details

(Keywords: polish, Whiteboard: [parity-chrome][bugday-20110513])

Attachments

(1 file, 36 obsolete files)

20.22 KB, patch
Details | Diff | Splinter Review
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
See Google Chrome for an excellent implemetation of this behaviour.
Whiteboard: [parity-chrome]
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.
Keywords: polish
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.
CC'ing Alex from the UX team who could give us possible solutions.
Keywords: uiwanted
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 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.
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.
cc'ing Dao since I think he has been looking into potential tab animations as well.
Blocks: 565707
Blocks: cuts-tabs
No longer blocks: 565707
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....
(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.
blocking2.0: --- → ?
(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?
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.
Keywords: uiwanted
Attached patch patch v1 (obsolete) — Splinter Review
Addresses non-overflow and overflow cases. Help wanted with robustly detecting when the mouse cursor leaves the tab toolbar.
Assignee: nobody → fyan
Status: NEW → ASSIGNED
Attachment #459686 - Flags: feedback?(dolske)
Something to consider: should we show the tab close icon longer (always?). Maybe we could also decrease the minimal tab size a bit.
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #459686 - Attachment is obsolete: true
Attachment #459750 - Flags: review?(dao)
Attachment #459686 - Flags: feedback?(dolske)
(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
Attachment #459750 - Flags: review?(dao)
Attachment #459750 - Attachment is obsolete: true
Attachment #459910 - Flags: review?(dao)
Attachment #459910 - Flags: ui-review?(limi)
Doesn't block, but I want it bad. Get it reviewed and ask for approval. You will get it.
blocking2.0: ? → -
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.
Attachment #459910 - Flags: review?(dao) → review-
(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.
(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.
(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/)
(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.
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #459910 - Attachment is obsolete: true
Attachment #461305 - Flags: ui-review?(limi)
Attachment #461305 - Flags: review?(dao)
Attachment #459910 - Flags: ui-review?(limi)
(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?
(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.
(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...
(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.
I'd love to get this into beta 4.
blocking2.0: - → ?
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?
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.
blocking2.0: ? → -
(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.
Attachment #461305 - Attachment is obsolete: true
Attachment #465096 - Flags: ui-review?(limi)
Attachment #465096 - Flags: review?(dao)
Attachment #461305 - Flags: ui-review?(limi)
Attachment #461305 - Flags: review?(dao)
Attachment #465096 - Flags: ui-review?(limi)
Attachment #465096 - Flags: ui-review?(beltzner)
Attachment #465096 - Flags: review?(gavin.sharp)
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
Attachment #465096 - Flags: ui-review?(beltzner) → ui-review+
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.
Attachment #465096 - Flags: review?(dao) → review-
Attachment #465096 - Flags: review?(gavin.sharp)
Attachment #465096 - Attachment is obsolete: true
Attachment #469397 - Flags: review?(dao)
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.
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.
Attachment #469397 - Attachment is obsolete: true
Attachment #469441 - Flags: review?(dao)
Attachment #469397 - Flags: review?(dao)
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.
(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.
(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 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
Attachment #469441 - Flags: review?(dao)
Is this feature in danger yet because of upcoming freezes?
(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?
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.
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.
Attachment #469441 - Attachment is obsolete: true
Attachment #474459 - Flags: review?
(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.
Blocks: 595694
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.
Attachment #474459 - Flags: review? → review?(dao)
(In reply to comment #51) By the way, thanks for all the testing, Michael :)
Attachment #474459 - Attachment is obsolete: true
Attachment #482612 - Flags: review?(dao)
Attachment #474459 - Flags: review?(dao)
While this isn't a blocker, it's a big win for users if we can get a review on it, Dao.
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)
Also, have you noticed Bug 606727? It will lead to tab flicking. _handleNewTab is unnecessarily called when transitionend event happens.
Severity: enhancement → normal
Depends on: 606727
Attachment #482612 - Attachment is obsolete: true
Attachment #488003 - Flags: review?(dao)
Attachment #482612 - Flags: review?(dao)
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.
And gBrowser.visibleTabs is not much reliable. We'd better count "boxObject.width > 0 && flex > 0 && not removing" tabs.
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.
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.
(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?
Attachment #488003 - Flags: review?(gavin.sharp)
(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.
(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.
Whiteboard: [parity-chrome] → [parity-chrome][target-betaN]
I think this got missed from triage. It's annoying. It's a chrome parity thing. And we have a patch.
blocking2.0: - → ?
my bad, missed the blocking2.0 minus in the history. Disregard!
blocking2.0: ? → ---
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?
blocking2.0: --- → ?
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.
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!
blocking2.0: ? → -
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).
Attachment #488003 - Flags: review?(dao)
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.
Attachment #499208 - Flags: review?(gavin.sharp)
Attachment #499208 - Flags: feedback?(fryn)
Patch part 1, version 12 removes the unused field.
Attachment #499208 - Attachment is obsolete: true
Attachment #499327 - Flags: review?(gavin.sharp)
Attachment #499327 - Flags: feedback?(fryn)
Attachment #499208 - Flags: review?(gavin.sharp)
Attachment #499208 - Flags: feedback?(fryn)
Try server tests were successful.
Patrick just demo'ed this, it's sweet. Many thumbs up. /be
Patch part 1, version 13 makes the method even simpler.
Assignee: fryn → pwalton
Attachment #499327 - Attachment is obsolete: true
Attachment #499440 - Flags: review?(gavin.sharp)
Attachment #499440 - Flags: feedback?(fryn)
Attachment #499327 - Flags: review?(gavin.sharp)
Attachment #499327 - Flags: feedback?(fryn)
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.
Attachment #499443 - Flags: review?(gavin.sharp)
Attachment #499443 - Flags: feedback?(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. :)
Attachment #499440 - Flags: feedback?(fryn) → feedback+
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.
Patch part 1, version 14 addresses feedback comments.
Attachment #499440 - Attachment is obsolete: true
Attachment #499453 - Flags: review?(gavin.sharp)
Attachment #499440 - Flags: review?(gavin.sharp)
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.
Attachment #499443 - Attachment is obsolete: true
Attachment #499454 - Flags: review?(gavin.sharp)
Attachment #499443 - Flags: review?(gavin.sharp)
Attachment #499443 - Flags: feedback?(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.
Attachment #499453 - Flags: feedback-
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 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, ""); }
Attachment #499454 - Flags: feedback-
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.
(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 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 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.
(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.
(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.
Patch part 1, version 15 addresses feedback. The spacer is gone now!
Attachment #499453 - Attachment is obsolete: true
Attachment #499794 - Flags: review?(gavin.sharp)
Attachment #499794 - Flags: feedback?(fryn)
Attachment #499453 - Flags: review?(gavin.sharp)
Patch part 2, version 3 addresses feedback. Review requested.
Attachment #499454 - Attachment is obsolete: true
Attachment #499795 - Flags: review?(gavin.sharp)
Attachment #499795 - Flags: feedback?(fryn)
Attachment #499454 - Flags: review?(gavin.sharp)
(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.
(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;
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;
(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.
(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.
(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.
New version of the patch brings back the spacer and only uses it: no max-width hacks necessary! Thanks ithinc :)
Attachment #499794 - Attachment is obsolete: true
Attachment #499795 - Attachment is obsolete: true
Attachment #499844 - Flags: review?(gavin.sharp)
Attachment #499844 - Flags: feedback?(fryn)
Attachment #499794 - Flags: review?(gavin.sharp)
Attachment #499794 - Flags: feedback?(fryn)
Attachment #499795 - Flags: review?(gavin.sharp)
Attachment #499795 - Flags: feedback?(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.
Attachment #499844 - Flags: feedback?(fryn) → feedback-
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.
(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 } }
(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.
(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?
This logic maybe works better, and is more simple. if closing non-last tab paddingEnd += tab.width; else paddingEnd -= scrollSize - scrollPosition - scrollClientSize;
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.
Attachment #499844 - Attachment is obsolete: true
Attachment #500133 - Flags: review?(gavin.sharp)
Attachment #500133 - Flags: feedback?
Attachment #499844 - Flags: review?(gavin.sharp)
Attachment #500133 - Flags: feedback? → feedback?(fryn)
Mozilla is all about "good enough" when it is time to ship. /be
(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?
When closing the last tab, we need a fixed spacer; When closing a non-last tab, we need a growing spacer.
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; }
In the mean time, capture and stopPropagation the underflow event, and resend it when mouse moves out of the tab bar.
(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).
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?
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 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/.
Attachment #500133 - Flags: feedback?(fryn) → feedback-
Nevermind about the toolkit/ bit. I see that it's just adding an anonid.
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.
Attachment #500133 - Attachment is obsolete: true
Attachment #500250 - Flags: review?(gavin.sharp)
Attachment #500250 - Flags: feedback?(fryn)
Attachment #500133 - Flags: review?(gavin.sharp)
Oh, forgot to mention: if the patch isn't doing anything for you, make sure you're recompiling toolkit/ as well as browser/
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.
Attachment #500250 - Flags: feedback?(fryn) → feedback-
> 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.
(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.
(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.
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.
(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.
Version 19 addresses the feedback comments.
Attachment #500250 - Attachment is obsolete: true
Attachment #500282 - Flags: review?(gavin.sharp)
Attachment #500282 - Flags: feedback?(fryn)
Attachment #500250 - Flags: review?(gavin.sharp)
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.
(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.
(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 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.
(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 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.
(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;}
(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.
(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.
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.
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.
(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 :)
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.
(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.
(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 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.
Attachment #500282 - Flags: feedback?(fryn) → feedback+
Attachment #488003 - Flags: review?(gavin.sharp)
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.
The landing of bug 572160 bitrotted this patch. Please update to tip, and link to tryserver builds for ease of review and UI feedback.
Attachment #488003 - Attachment is obsolete: true
I'm happy to do the UI review on this once there is a tryserver build.
Patch version 20 updates to trunk. Try server build forthcoming!
Attachment #500282 - Attachment is obsolete: true
Attachment #502184 - Flags: ui-review?(limi)
Attachment #502184 - Flags: review?(gavin.sharp)
Attachment #500282 - Flags: review?(gavin.sharp)
(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?
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.
(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.)
* 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.
(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.
Attachment #502184 - Flags: ui-review?(limi)
Attachment #502184 - Flags: review?(gavin.sharp)
Attached patch Proposed patch, version 21. (obsolete) — Splinter Review
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.
Attachment #502184 - Attachment is obsolete: true
Attachment #507000 - Flags: ui-review?(limi)
Attachment #507000 - Flags: review?(gavin.sharp)
Attachment #507000 - Flags: feedback?(fryn)
Baking on try server. I'll have a try build ready when it's done.
Attached patch Proposed patch, version 22. (obsolete) — Splinter Review
Patch version 22 fixes some RTL issues and cleans up the animation a bit.
Attachment #507000 - Attachment is obsolete: true
Attachment #507023 - Flags: ui-review?(limi)
Attachment #507023 - Flags: review?(gavin.sharp)
Attachment #507023 - Flags: feedback?(fryn)
Attachment #507000 - Flags: ui-review?(limi)
Attachment #507000 - Flags: review?(gavin.sharp)
Attachment #507000 - Flags: feedback?(fryn)
(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 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|.
Attachment #507023 - Flags: feedback?(fryn) → feedback-
(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.
Attachment #507023 - Flags: ui-review?(limi)
Attachment #507023 - Flags: review?(gavin.sharp)
(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
(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!
(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. :)
(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.
(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
(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.
(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.
Attached patch Proposed patch, version 23. (obsolete) — Splinter Review
Patch version 23 fixes the RTL issue and the Panorama bug, as well as I can tell. It also mitigates papercut (A) a bit.
Attachment #507023 - Attachment is obsolete: true
Attachment #507306 - Flags: feedback?(fryn)
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.
blocking2.0: - → ?
Whiteboard: [parity-chrome][target-betaN] → [parity-chrome][softblocker][d?]
Please don't add [softblocker] to bugs that don't block. It breaks simple sw:softblocker queries.
Whiteboard: [parity-chrome][softblocker][d?] → [parity-chrome][d?]
IIRC, Beltzner said not to use [d?] anymore.
Whiteboard: [parity-chrome][d?] → [parity-chrome]
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.
blocking2.0: ? → -
status2.0: --- → wanted
Working on a test case now. Limi, can you do ui-r on this?
(In reply to comment #172) > Working on a test case now. Limi, can you do ui-r on this? Yes, more than happy to.
Attached patch Proposed patch, version 24. (obsolete) — Splinter Review
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/pwalton@mozilla.com-b54200a778b0 IMO this is nearing the end of the cycle; reviews and feedback would be much appreciated.
Attachment #507306 - Attachment is obsolete: true
Attachment #507721 - Flags: ui-review?(limi)
Attachment #507721 - Flags: feedback?(fryn)
Attachment #507306 - Flags: feedback?(fryn)
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
(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 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.
Attachment #507721 - Flags: feedback?(fryn) → feedback+
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.
(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".
(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.
Attached patch Proposed patch, version 25. (obsolete) — Splinter Review
Patch version 25 addresses feedback and adds in the ensureElementIsVisible() again. Review and UI review requested.
Attachment #507721 - Attachment is obsolete: true
Attachment #507954 - Flags: ui-review?(limi)
Attachment #507954 - Flags: review?(gavin.sharp)
Attachment #507721 - Flags: ui-review?(limi)
(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 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.
Attachment #507954 - Flags: ui-review?(limi) → ui-review-
To force RTL mode for testing in Firefox, you can use Ehsan's awesome extension: https://addons.mozilla.org/addon/force-rtl/
(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 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.
Attachment #507954 - Flags: ui-review- → ui-review+
Comment on attachment 507954 [details] [diff] [review] Proposed patch, version 25. Yup, seems to work great once the RTL stuff is initialized properly.
Attachment #507954 - Flags: review?(gavin.sharp) → review?(dolske)
I am told that this is no longer possible for Firefox 4. This will be released as an addon.
(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?
(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.
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.
How about preffing it off for Firefox 4? Would someone review it in that case?
Attachment #507954 - Flags: review?(dolske) → review?(gavin.sharp)
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.
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.
(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.
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.
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
(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.
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
I suggest you guys fix the depended bug 606727, bug 622266 first.
(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.
No longer depends on: 606727
(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.
(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.
Attached patch Proposed patch, version 26. (obsolete) — Splinter Review
Patch version 26 unlocks the tabs when the pointer leaves the tab strip.
Attachment #507954 - Attachment is obsolete: true
Attachment #510468 - Flags: review?(gavin.sharp)
Attachment #507954 - Flags: review?(gavin.sharp)
(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.
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.
(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?
(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.
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.
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.
(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.
Attachment #510468 - Flags: review?(dao)
Could I get a review on this?
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.
(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.
Apps tabs can still be closed by middle click, though.
(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.
(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.
Attached patch Proposed patch, version 27. (obsolete) — Splinter Review
Patch version 27 addresses review comments.
Attachment #510468 - Attachment is obsolete: true
Attachment #523471 - Flags: review?(gavin.sharp)
Attachment #510468 - Flags: review?(gavin.sharp)
Attachment #510468 - Flags: review?(dao)
Attachment #523471 - Flags: review?(dao)
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.
Attachment #523471 - Flags: review?(gavin.sharp) → review+
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.)
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.
Attachment #524295 - Flags: review?(gavin.sharp)
Attachment #524295 - Flags: review?(dolske)
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.
(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?
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. :)
Attachment #524295 - Flags: review?(gavin.sharp)
Attachment #524295 - Flags: review?(dolske)
Attached patch patch v29 (obsolete) — Splinter Review
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=fyan@mozilla.com&rev=dc4af5e171cf This final version is running on tryserver: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=fyan@mozilla.com&rev=40bd7086613b I'll link to builds when they are available.
Assignee: pwalton → fryn
Attachment #523471 - Attachment is obsolete: true
Attachment #524295 - Attachment is obsolete: true
Attachment #524805 - Flags: review?(gavin.sharp)
Attachment #524805 - Flags: review?(dolske)
Attachment #524295 - Attachment description: unbitrotted, improved version of my original approach → patch v28 - unbitrotted, improved version of my original approach
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?
> >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.
(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.
Attached patch patch v30 (obsolete) — Splinter Review
(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.
Attachment #524805 - Attachment is obsolete: true
Attachment #524885 - Flags: review?(gavin.sharp)
Attachment #524885 - Flags: review?(dolske)
Attachment #524805 - Flags: review?(gavin.sharp)
Attachment #524805 - Flags: review?(dolske)
(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.
(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 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?
(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.
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.
(In reply to comment #236) That's how the feature is supposed to behave.
(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?
(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.
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 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?
Attached patch patch v31 (obsolete) — Splinter Review
(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.
Attachment #524885 - Attachment is obsolete: true
Attachment #525264 - Flags: review?(gavin.sharp)
Attachment #524885 - Flags: review?(gavin.sharp)
Attachment #524885 - Flags: review?(dolske)
(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?
Blocks: 649216
Blocks: 649218
Attached patch patch v32 (obsolete) — Splinter Review
(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.
Attachment #525264 - Attachment is obsolete: true
Attachment #525275 - Flags: review?(gavin.sharp)
Attachment #525264 - Flags: review?(gavin.sharp)
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.
Attachment #525275 - Flags: review?(gavin.sharp) → review+
(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.
Attachment #525275 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 649251
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2
Version: unspecified → Trunk
When I move the pointer to the screen edge just above the gap between the tabs, resizing does occur. Please fix this.
(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.
Target Milestone: Firefox5 → Firefox 3
Target Milestone: Firefox 3 → Firefox 5
Depends on: 649441
No longer depends on: 649561
(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
Depends on: 649624
Depends on: 649627
Depends on: 649628
Depends on: 649654
Depends on: 649671
Blocks: 649671
No longer depends on: 649671
Blocks: 649627
No longer depends on: 649627
shouldn't resizing occur when user moves to app tabs?
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?
(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.
(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.
(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.
(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?
(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.
Depends on: 652320
Depends on: 652424
Tabs should not animate while resizing when "browser.tabs.animate" is set to false.
Attachment #525653 - Attachment is obsolete: true
Depends on: 652735
Depends on: 649561
Summary: When closing a tab, other tabs should not resize until cursor leaves tab toolbar → When closing a tab, other tabs should not resize until cursor leaves toolbar region
This bug is fixed. Please file new bugs for any remaining problems / suggestions, instead of commenting here. Otherwise they're likely to be missed.
Depends on: 653053
Depends on: 655797
Depends on: 655802
Verified fixed in: Mozilla/5.0 (Windows NT 6.1; rv:5.0a2) Gecko/20110513 Firefox/5.0a2
Status: RESOLVED → VERIFIED
Whiteboard: [parity-chrome] → [parity-chrome][bugday-20110513]
Depends on: 658729
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!
Yes, I also want to know, if this feature can be disabled.
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.
This bug is resolved and Bugzilla is not a support forum. Please take your comments to the proper forum. Thank you.
Probably better to file a bug for making it configurable.
(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.
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.
I've filed Bug 669318 to deal with the fact that the tab resizing animation occurs even with browser.tabs.animate set to false.
changing tab width using CSS (Bug 574654) now breaks this functionality
(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
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.
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?
No longer blocks: 649671
Depends on: 649671
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?
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.
Depends on: 1197582
Depends on: 1494944
Depends on: 1816236
Depends on: 1823413
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: