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