Open
Bug 649654
Opened 10 years ago
Updated 7 years ago
When tab bar underflow occurs and tab strip was scrolled to the beginning, resize tabs such that they can be closed in succession
Categories
(Firefox :: Tabbed Browser, defect)
Firefox
Tabbed Browser
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox5 | - | --- |
People
(Reporter: tabutils+bugzilla, Unassigned)
References
Details
Attachments
(1 file, 3 obsolete files)
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110413 Firefox/6.0a1 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110413 Firefox/6.0a1 See the following steps. Reproducible: Always Steps to Reproduce: 1. Open enough tabs to overflow the tab bar 2. Select the first tab 3. Close all tabs from the first tab with mouse clicking the "x" button Actual Results: Tab resizing happens immediately when the tab bar underflows Expected Results: Tab resizing should not happen before mouse moves off the tab bar
Comment 1•10 years ago
|
||
Confirmed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110413 Firefox/6.0a1
Status: UNCONFIRMED → NEW
Ever confirmed: true
![]() |
||
Comment 2•10 years ago
|
||
Tab resizing does not occur, but the tab bar shifts due to the change in presence of scroll buttons.
Summary: Resizing happens immediately when tab bar underflows → Handle tab bar underflow more gracefully when repeatedly closing the first tab with the mouse
![]() |
||
Updated•10 years ago
|
Assignee: nobody → fryn
Status: NEW → ASSIGNED
Summary: Handle tab bar underflow more gracefully when repeatedly closing the first tab with the mouse → Handle tab bar underflow more gracefully when tab strip was scrolled to the beginning
![]() |
||
Comment 3•10 years ago
|
||
Fixes this and a few other cases. Also, gives up on trying to special case tabs with owners for now, since it only solved a few edge cases.
Attachment #525930 -
Flags: review?(gavin.sharp)
Attachment #525930 -
Flags: review?(dao)
Comment on attachment 525930 [details] [diff] [review] patch >diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css >+.tabbrowser-tabs[locked] > .tabbrowser-tab:not([pinned])[fadein] { >+ max-width: 100px; >+} Why not use "100px" as tabTempMaxWidth this time? >diff --git a/browser/base/content/tabbrowser.css b/browser/base/content/tabbrowser.css >+.tabbrowser-arrowscrollbox > .scrollbutton-up[spacer], >+.tabbrowser-arrowscrollbox > .scrollbutton-down[spacer] { >+ visibility: hidden; >+} It could be better to set style.visibility directly for this.
![]() |
||
Comment 5•10 years ago
|
||
(In reply to comment #4) > Comment on attachment 525930 [details] [diff] [review] > patch > > >diff --git a/browser/base/content/browser.css b/browser/base/content/browser.css > >+.tabbrowser-tabs[locked] > .tabbrowser-tab:not([pinned])[fadein] { > >+ max-width: 100px; > >+} > > Why not use "100px" as tabTempMaxWidth this time? That would require a more expensive loop and more complex unlocking. I intentionally only use the style.maxWidth loop in non-overflow mode, so the loop will be run at most (tab container width divided by 100) times, which in maximized mode on larger monitors would be at most 20 times. > >diff --git a/browser/base/content/tabbrowser.css b/browser/base/content/tabbrowser.css > >+.tabbrowser-arrowscrollbox > .scrollbutton-up[spacer], > >+.tabbrowser-arrowscrollbox > .scrollbutton-down[spacer] { > >+ visibility: hidden; > >+} > > It could be better to set style.visibility directly for this. Perhaps.
(In reply to comment #5) > That would require a more expensive loop and more complex unlocking. > I intentionally only use the style.maxWidth loop in non-overflow mode, so the > loop will be run at most (tab container width divided by 100) times, which in > maximized mode on larger monitors would be at most 20 times. Instead I suggest you always use ".tabbrowser-tabs[locked] > .tabbrowser-tab:not([pinned])[fadein]" rule and then set style.maxWidth on this rule dynamically. No need to loop the tabs more. You need to make a reference to this rule but it is merely one-time cost. A lazy getter should be OK.
For example: gBrowser.mTabContainer.__defineGetter__("_tabSizingRule", function() { delete this._tabSizingRule; for (let i = 0, styleSheet; styleSheet = document.styleSheets[i]; i++) { if (styleSheet.href == "chrome://browser/content/tabbrowser.css") { for (let j = 0, cssRule; cssRule = styleSheet.cssRules[j]; j++) { if (cssRule.selectorText == ".tabbrowser-tabs[locked] > .tabbrowser-tab:not([pinned])[fadein]") { return this._tabSizingRule = cssRule; } } } }); Then you need only to set style.maxWidth on _tabSizingRule and set/unset "locked" attribute on the tabContainer.
Comment 8•10 years ago
|
||
Seems like a good polish fix for a handy user experience feature that Frank's been working on for a while and managed to get into mozilla-aurora. I would suggest that if the fix takes on central, we carry this one over to mozilla-aurora for "stabilization". I *believe* that this is the correct marking for that goal, but be unafraid to correct me!
tracking-firefox5:
--- → ?
Comment 9•10 years ago
|
||
Unless UI team feels strongly on this, we would not stop beta for this. If UI team feels strongly, can be re-nomed for discussion. Also patch review is not complete for risk assessment.
![]() |
||
Comment 10•10 years ago
|
||
Comment on attachment 525930 [details] [diff] [review] patch Dão, could you take a look at this? Let me know if I can do anything to help you review it.
Attachment #525930 -
Flags: review?(gavin.sharp)
![]() |
||
Comment 11•10 years ago
|
||
Attachment #525930 -
Attachment is obsolete: true
Attachment #525930 -
Flags: review?(dao)
Attachment #534318 -
Flags: review?(dao)
Comment 12•10 years ago
|
||
(In reply to comment #3) > Created attachment 525930 [details] [diff] [review] [review] > patch > > Fixes this and a few other cases. > > Also, gives up on trying to special case tabs with owners for now, since it > only solved a few edge cases. Could you please move unrelated changes to separate patches & bugs?
![]() |
||
Updated•10 years ago
|
Summary: Handle tab bar underflow more gracefully when tab strip was scrolled to the beginning → When tab bar underflow occurs and tab strip was scrolled to the beginning, resize tabs such that they can be closed in succession
Updated•10 years ago
|
Attachment #534318 -
Flags: review?(dao)
![]() |
||
Updated•9 years ago
|
Assignee: fryn → nobody
Status: ASSIGNED → NEW
Reporter | ||
Comment 14•8 years ago
|
||
Attachment #699771 -
Flags: review?(fyan)
![]() |
||
Updated•8 years ago
|
Attachment #534318 -
Attachment is obsolete: true
![]() |
||
Comment 15•8 years ago
|
||
Comment on attachment 699771 [details] [diff] [review] patch on top of bug 821859 Review of attachment 699771 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.css @@ -52,5 @@ > > -.tabbrowser-tabs:not(:hover) > .tabbrowser-arrowscrollbox > .closing-tabs-spacer { > - transition: width .15s ease-out; > -} > - We need to keep this block in some form and the class on the <xul:spacer/>. Without any animation, unlocking the tabs is jarring. min-width doesn't animate from "none", so the above two blocks will need to be changed to something like: .closing-tabs-spacer { min-width: 0; pointer-events: none; } .tabbrowser-tabs:not(:hover) > .tabbrowser-arrowscrollbox > .closing-tabs-spacer { transition: min-width 150ms ease-out; } ::: browser/base/content/tabbrowser.xml @@ +2831,5 @@ > var tabs = document.getBindingParent(this); > + > + if (tabs.hasAttribute("dontresize") || tabs.hasAttribute("using-closing-tabs-spacer")) { > + tabs.mTabstrip._scrollButtonUp.style.visibility = "visible"; > + tabs.mTabstrip._scrollButtonDown.style.visibility = "hidden"; These should either both be "visible" or "hidden". I slightly prefer "visible" despite the visual weight, but I'm open to reasons for the latter.
Attachment #699771 -
Flags: review?(fyan) → review-
![]() |
||
Comment 16•8 years ago
|
||
Comment on attachment 699771 [details] [diff] [review] patch on top of bug 821859 Review of attachment 699771 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +3150,2 @@ > this.tabbrowser.addEventListener("mousemove", this, false); > window.addEventListener("mouseout", this, false); Could you unindent by 2 spaces the entire above block? I don't mind the "hg blame" churn.
![]() |
||
Comment 17•8 years ago
|
||
Comment on attachment 699771 [details] [diff] [review] patch on top of bug 821859 Review of attachment 699771 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ +2831,5 @@ > var tabs = document.getBindingParent(this); > + > + if (tabs.hasAttribute("dontresize") || tabs.hasAttribute("using-closing-tabs-spacer")) { > + tabs.mTabstrip._scrollButtonUp.style.visibility = "visible"; > + tabs.mTabstrip._scrollButtonDown.style.visibility = "hidden"; I realized that the "down" scroll button stays in its enabled state here. Let's just set both to "hidden" then.
Reporter | ||
Comment 18•8 years ago
|
||
(In reply to Frank Yan (:fryn) from comment #17) > I realized that the "down" scroll button stays in its enabled state here. > Let's just set both to "hidden" then. We may set its disabled state correctly and make them "visible".
![]() |
||
Comment 19•8 years ago
|
||
(In reply to ithinc from comment #18) > (In reply to Frank Yan (:fryn) from comment #17) > > I realized that the "down" scroll button stays in its enabled state here. > > Let's just set both to "hidden" then. > > We may set its disabled state correctly and make them "visible". That's fine too. Leaving both hidden might even feel cleaner. I think it's fine to leave both as hidden and move on. Now that these patches fix the major remaining edge cases/issues for clicking in the same place to close tabs quickly, bug 649216 seems like a reasonable next step, if you're up for it.
![]() |
||
Comment 20•8 years ago
|
||
ithinc, are you happy with this patch after including the changes according to my review comments? If so, I can change the review to a conditional r+, create an updated diff, and push it.
Reporter | ||
Comment 21•8 years ago
|
||
Not so quick. I'm not free in the recent days. I'll address your comments a little later. If you're willing to update the patch by yourself, that's fine for me. Thanks.
Reporter | ||
Comment 22•8 years ago
|
||
Forgot to say, I have a few little changes to add. I'll upload a new patch this weekend.
![]() |
||
Comment 23•8 years ago
|
||
Comment on attachment 699771 [details] [diff] [review] patch on top of bug 821859 Review of attachment 699771 [details] [diff] [review]: ----------------------------------------------------------------- The requested changes are minor and have been discussed and agreed upon. This is really a conditional r+. (In reply to ithinc from comment #22) > Forgot to say, I have a few little changes to add. I'll upload a new patch > this weekend. It's been a few weeks, and I'd rather not wait on more changes, especially if they grow the scope of this bug. Feel free to file a followup with any changes you have in mind when you have time.
Attachment #699771 -
Flags: review- → review+
![]() |
||
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5294a43582dd
Assignee: nobody → ithinc
Status: NEW → ASSIGNED
![]() |
||
Updated•8 years ago
|
Target Milestone: --- → Firefox 21
Comment 25•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5294a43582dd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
![]() |
||
Comment 26•8 years ago
|
||
Backed out in bug 851436 due to causing regression and mostly due to being built on top of bug 821859, which we learned is not a performant approach. https://hg.mozilla.org/mozilla-central/rev/7fbba1340a23 Bug 851436 comment 11 by Dão Gottwald is worth repeating here: > If someone wants to give bug 649654 another shot later > on, avoiding the problems it caused the first time, that's of course always > an option.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
||
Updated•8 years ago
|
Assignee: ithinc → nobody
Status: REOPENED → NEW
Target Milestone: Firefox 21 → ---
![]() |
||
Comment 27•8 years ago
|
||
Attachment #699771 -
Attachment is obsolete: true
Attachment #726312 -
Flags: review-
You need to log in
before you can comment on or make changes to this bug.
Description
•