The default bug view has changed. See this FAQ.

When tab bar underflow occurs and tab strip was scrolled to the beginning, resize tabs such that they can be closed in succession

NEW
Unassigned

Status

()

Firefox
Tabbed Browser
6 years ago
3 years ago

People

(Reporter: ithinc, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5-)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Blocks: 465086
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

6 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

6 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

6 years ago
Created attachment 525930 [details] [diff] [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.
Attachment #525930 - Flags: review?(gavin.sharp)
Attachment #525930 - Flags: review?(dao)
(Reporter)

Comment 4

6 years ago
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

6 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.
(Reporter)

Comment 6

6 years ago
(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.
(Reporter)

Comment 7

6 years ago
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.
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

6 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.
tracking-firefox5: ? → -

Comment 10

6 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

6 years ago
Created attachment 534318 [details] [diff] [review]
patch updated to tip
Attachment #525930 - Attachment is obsolete: true
Attachment #525930 - Flags: review?(dao)
Attachment #534318 - Flags: review?(dao)
(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

6 years ago
Duplicate of this bug: 666913

Updated

6 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

6 years ago
Attachment #534318 - Flags: review?(dao)

Updated

5 years ago
Assignee: fryn → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

4 years ago
Depends on: 821859
(Reporter)

Comment 14

4 years ago
Created attachment 699771 [details] [diff] [review]
patch on top of bug 821859
Attachment #699771 - Flags: review?(fyan)

Updated

4 years ago
Attachment #534318 - Attachment is obsolete: true

Comment 15

4 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

4 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

4 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

4 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

4 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

4 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

4 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

4 years ago
Forgot to say, I have a few little changes to add. I'll upload a new patch this weekend.

Comment 23

4 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

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5294a43582dd
Assignee: nobody → ithinc
Status: NEW → ASSIGNED

Updated

4 years ago
Target Milestone: --- → Firefox 21
https://hg.mozilla.org/mozilla-central/rev/5294a43582dd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 837486

Comment 26

4 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

4 years ago
Assignee: ithinc → nobody
Status: REOPENED → NEW
Target Milestone: Firefox 21 → ---

Comment 27

4 years ago
Created attachment 726312 [details] [diff] [review]
patch that is not ready and must be improved not to modify browser.css at runtime and not to cause bug 851436
Attachment #699771 - Attachment is obsolete: true
Attachment #726312 - Flags: review-

Updated

4 years ago
No longer depends on: 821859
You need to log in before you can comment on or make changes to this bug.