Closed Bug 837486 Opened 11 years ago Closed 11 years ago

Reposition pinned tabs when resetting tab strip underflow

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 22
Tracking Flags:
Tracking Status
firefox20 --- unaffected
firefox21 + verified
firefox22 --- verified

People

(Reporter: Optimizer, Assigned: fryn)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

First tab is not properly placed.
56.56 KB, image/png
Details
tbird example (png)
62.05 KB, image/png
Details
patch v3
7.71 KB, patch
dao
: review+
bajaj
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Not really reliable STR but :

1) Open many tabs so that they overflow and the scrolling arrows appear after and before the tabs.
2) Close soem of the tabs so that they should no longer be overflowing.

Expected behavior:
The scrolling arrows disappear and that space is used by the tabs.

Actual behavior:
The scrolling arrows disappear, but the space is left empty. See the attachment.

May be its due to pinned tab and a recent regression as I never encountered this around 3-4 days back, and now in 2 days I have seen this thrice.

I am always using the latest nightly, and I open a lot of tabs such that this number of times my tabs overflow and then come down to normal state is much much greater than 3.
Blocks: 649654
Assignee: nobody → fyan
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: Tab strip does not reset after closing tabs to stop overflowing. → Reposition pinned tabs when resetting tab strip underflow
Version: unspecified → Trunk
But the pinned tab was at its correct position.
Attached patch patch (obsolete) — — Splinter Review 

        Attachment #709700 -
        Flags: review?(dao)

    
    

    
  
My bad, I didn't see that it was the pinned tab that was misaligned. Disregard comment 1.

PS:now this is 100% reproducible

    
    

    
  
The regression window is known here:
The landing of the patch in bug 649654 caused this.
Keywords: regressionwindow-wanted

    
    

    
  
Comment on attachment 709700 [details] [diff] [review]
patch

Review of attachment 709700 [details] [diff] [review]:
-----------------------------------------------------------------

(Review ping:)
We really should get this fixed before the next merge.
I think landing this small patch is assuredly preferable to backing out two other larger patches.

        Attachment #709700 -
        Flags: review?(dolske)

    
    

    
  

      
      
      
      

        Attached image
          
        tbird example (png)
        — 
      

    


  
FWIW here is a grab of it happening in tbird. Note I've had it happen to a pinned tab in FF as well (half of the leftmost pinned tab scrolled off to the left).

    
    

    
  
I'm seeing this, too, on OSX.  It is slightly different than the screen shot, because half of the leftmost pinned tab (more or less) is kind of pushed off the left side and not visible.  But I see the same gap between pinned and unpinned tabs.

    
    

    
  
We need to fix this soon.

Dao, since you're probably the person best suited to review, can you increase the priority of this request?

    
    

    
  
Comment on attachment 709700 [details] [diff] [review]
patch

>-#tabbrowser-tabs:not([overflow="true"])[using-closing-tabs-spacer] ~ #alltabs-button {
>-  visibility: hidden; /* temporary space to keep a tab's close button under the cursor */

This was the last external occurrence of the using-closing-tabs-spacer attribute. We should get rid of it.

>+#tabbrowser-tabs[overflow="true"] > .tabbrowser-arrowscrollbox > .scrollbutton-up,
>+#tabbrowser-tabs[overflow="true"] > .tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  visibility: visible;
> }

It's not at all self-explanatory what this code is doing. Add a comment and [collapsed=true] to the selectors for clarity.

>       <method name="_unlockTabSizing">
>+        <parameter name="dontReposition"/>
>         <body><![CDATA[
>           this.tabbrowser.removeEventListener("mousemove", this, false);
>           window.removeEventListener("mouseout", this, false);
> 
>           this._closingTabsSpacer.style.minWidth = "";
>           this.removeAttribute("using-closing-tabs-spacer");
>           this.removeAttribute("dontresize");
> 
>-          if (this.hasAttribute("overflow") && this.mTabstrip._scrollbox.scrollWidth <= this.mTabstrip._scrollbox.clientWidth) {
>-            this.mTabstrip._scrollButtonUp.style.visibility = "";
>-            this.mTabstrip._scrollButtonDown.style.visibility = "";
>+          if (this.hasAttribute("overflow") &&
>+              this.mTabstrip._scrollbox.scrollWidth <= this.mTabstrip._scrollbox.clientWidth) {
>             this.removeAttribute("overflow");
>+            if (!dontReposition)
>+              this._positionPinnedTabs();
>           }
>         ]]></body>
>       </method>

The dontReposition argument feels a bit whacky. It's unclear what it would be good for just by looking at this method. I'd rather get rid of it and take the redundant _positionPinnedTabs call in what seems like a rare edge case.

        Attachment #709700 -
        Flags: review?(dao) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Thanks for the first review.

        Attachment #709700 -
        Attachment is obsolete: true

        Attachment #709700 -
        Flags: review?(dolske)

        Attachment #716795 -
        Flags: review?(dao)

    
    

    
  
Comment on attachment 716795 [details] [diff] [review]
patch v2

Review of attachment 716795 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, using :hover here causes other breakage.

        Attachment #716795 -
        Flags: review?(dao)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3
        — 
          — Splinter Review
      

    


  

        Attachment #716795 -
        Attachment is obsolete: true

        Attachment #716854 -
        Flags: review?(dao)

    
    

    
  
Comment on attachment 716854 [details] [diff] [review]
patch v3

>       <!-- Try to keep the active tab's close button under the mouse cursor -->
>       <method name="_lockTabSizing">
>         <parameter name="aTab"/>
>         <body><![CDATA[
>           var tabs = this.tabbrowser.visibleTabs;
>           if (!tabs.length)
>             return;
> 
>-          var isEndTab = (aTab._tPos > tabs[tabs.length-1]._tPos);
>-          var tabWidth = aTab.getBoundingClientRect().width;
>-
>-          // Locking is neither in effect nor needed, so let tabs expand normally.
>-          if (isEndTab && !this.hasAttribute("dontresize"))
>-            return;
>-
>-          // Let spacer grow to the maximum and lock it, then let tabs expand normally
>+          this.tabbrowser.addEventListener("mousemove", this, false);
>+          window.addEventListener("mouseout", this, false);
>+
>+          let isEndTab = (aTab._tPos > tabs[tabs.length-1]._tPos);
>+          let spacer = this._closingTabsSpacer;
>           if (isEndTab) {
>-            let spacer = this._closingTabsSpacer;
>+            if (!spacer.style.minWidth)
>+              spacer.style.minWidth = 0;

What's the purpose of the last two lines?

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #18)
> >+            if (!spacer.style.minWidth)
> >+              spacer.style.minWidth = 0;
> 
> What's the purpose of the last two lines?

It ensures that we handle underflow when repeatedly closing the last tab.

    
    

    
  
Are we letting this go to beta ?

    
    

    
  
Comment on attachment 716854 [details] [diff] [review]
patch v3

>+          let spacer = this._closingTabsSpacer;
>           if (isEndTab) {
>-            let spacer = this._closingTabsSpacer;

>+          let tabWidth = aTab.getBoundingClientRect().width;
>           if (!this.hasAttribute("dontresize")) {
>             this._delayResizingRule.style.setProperty("max-width", tabWidth + "px", "important");
>             this.setAttribute("dontanimate", "true");
>             this.setAttribute("dontresize", "true");
>             this.clientTop; // flush styles to skip animation; see bug 649247
>             this.removeAttribute("dontanimate");
>           }
> 
>           if (!this.mTabstrip._scrollButtonUp.disabled) {
>-            let spacer = this._closingTabsSpacer;

|spacer| isn't needed in all cases, so I'm not sure declaring it unconditionally is an improvement. Same applies to |tabWidth|.

        Attachment #716854 -
        Flags: review?(dao) → review+

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #25)
> |spacer| isn't needed in all cases, so I'm not sure declaring it
> unconditionally is an improvement. Same applies to |tabWidth|.

Thanks for the review. I pushed it with these declarations moved into their `if` blocks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d670756c497
Target Milestone: --- → Firefox 22

    
    

    
  
Comment on attachment 716854 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by: the landing of the patch in bug 649654
User impact if declined: pinned tabs will be positioned incorrectly in some cases, which is a bad glitch in primary UI
Testing completed: locally and now landed on m-c
Risk to taking this patch: minimal; fix is contained to tab strip
String or UUID changes made by this patch: none

        Attachment #716854 -
        Flags: approval-mozilla-aurora?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4d670756c497
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 716854 [details] [diff] [review]
patch v3

low risk uplift of a Fx21 regression causing a UI glitch in pinned tabs .

        Attachment #716854 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
  
Depends on: 851436

    
    

    
  
I could reproduce this fairly reliably before today. No longer reproducible with today's Aurora and Nightly builds. Marking this verified fixed but I'll keep an eye on this over the next few days.
Status: RESOLVED → VERIFIED

          status-firefox21:
          fixed → verified

          status-firefox22:
          fixed → verified

    
    

    
  
(In reply to Scoobidiver from comment #32)
> Backout from Aurora in bug 851436:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/bfb3ef111866

We backed out both the cause and fix.

          status-firefox21:
          affected → verified

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: