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

Status: NEW

(Firefox :: Tabbed Browser, defect)

Description

[ithinc]

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

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

Confirmed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110413 Firefox/6.0a1

Comment 2

[Frank Yan (:fryn)]

Tab resizing does not occur, but the tab bar shifts due to the change in presence of scroll buttons.

Comment 3

[Frank Yan (:fryn)]

Attachment: 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.

Comment 4

[ithinc]

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

[Frank Yan (:fryn)]

(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.

Comment 6

[ithinc]

(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.

Comment 7

[ithinc]

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

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

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!

Comment 9

[JP Rosevear [:jpr]]

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

[Frank Yan (:fryn)]

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.

Comment 11

[Frank Yan (:fryn)]

Attachment: patch updated to tip

Comment 12

[Dão Gottwald [: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?

Comment 14

[ithinc]

Attachment: patch on top of bug 821859

Comment 15

[Frank Yan (:fryn)]

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.

Comment 16

[Frank Yan (:fryn)]

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

[Frank Yan (:fryn)]

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.

Comment 18

[ithinc]

(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

[Frank Yan (:fryn)]

(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

[Frank Yan (:fryn)]

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.

Comment 21

[ithinc]

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.

Comment 22

[ithinc]

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

Comment 23

[Frank Yan (:fryn)]

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.

Comment 24

[Frank Yan (:fryn)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5294a43582dd

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5294a43582dd

Comment 26

[Frank Yan (:fryn)]

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.

Comment 27

[Frank Yan (:fryn)]

Attachment: patch that is not ready and must be improved not to modify browser.css at runtime and not to cause bug 851436