Closed Bug 509412 Opened 15 years ago Closed 15 years ago

tab close buttons disappear when closing the first tab

Categories

(Firefox :: Tabbed Browser, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3.7a1
Tracking Flags:
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: yekshemash, Assigned: wgianopoulos)

References

(
URL
)

Details

(Keywords: verified1.9.2)

Attachments

(1 file, 2 obsolete files)

patch v2.1 - fix whitespace
1.88 KB, patch
dao
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1) Gecko/20090806 Namoroka/3.6a1 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1) Gecko/20090806 Namoroka/3.6a1 (.NET CLR 3.5.30729)

Sometimes it's impossible to close inactive tab without switching to it. The button reappears when opening new tab or resizing ff window.
Usually this happens when at least 3 tabs are open an the first

Reproducible: Always

Steps to Reproduce:
1. Three tabs open (not blank)
2. Close the first one.

Actual Results:  
The second tab lacks it's close button

Expected Results:  
There should be [x] button ;)
Actually bug happens also with blank tabs and with more than just 3 tabs..
Confirming bug.  Also happens under Linux.  OS -> All
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Flags: blocking-firefox3.6?
Version: unspecified → Trunk
I believe what is happening here is that we are hitting this code in tabbrowser.xml:

      <method name="adjustTabstrip">
        <body><![CDATA[
          // modes for tabstrip
          // 0 - activetab  = close button on active tab only
          // 1 - alltabs    = close buttons on all tabs
          // 2 - noclose    = no close buttons at all
          // 3 - closeatend = close button at the end of the tabstrip
          switch (this.mCloseButtons) {
          case 0:
            this.setAttribute("closebuttons", "activetab");
            break;
          case 1:
            if (this.firstChild.getBoundingClientRect().width > this.mTabClipWidth)
              this.setAttribute("closebuttons", "alltabs");
            else
              this.setAttribute("closebuttons", "activetab");
            break;
 
when this.firstChild is still set to the tab being closed and so the bounding rectangle width is zero.

If that is what is really happening, it would seem the correct fix would be to do this after firstChild gets updated to point to the new first tab.
You're probably right, Bill. Looks like adjustTabstrip in _endRemoveTab should be moved to after this.tabContainer.removeChild(aTab);. Care to create a patch?
Attached patch patch v1 (obsolete) — — Splinter Review 
This seems to correct the issue.  I also tried the case where I open enough tabs so that the close buttons become hidden and tested that when i had closed the first tab enough times the close buttons appeared as they should.

Before seeing your comment, I had created another patch which seemed to also fix the issue.

In that case I moved this code all the way to the bottom of the method where there already was a test for the window being closed, so it was less code, but a much more dramatic change in the order of performing functions.
Assignee: nobody → bill
Status: NEW → ASSIGNED
Attachment #394648 - Flags: review?(dao)
Comment on attachment 394648 [details] [diff] [review]
patch v1

>             // Remove the tab ...
>             this.tabContainer.removeChild(aTab);
>+
>+            // Update Tab close button and scroll buttons state
>+            if (!aCloseWindow) {
>+              this.tabContainer.adjustTabstrip();
>+              // workaround for bug 345399
>+              this.tabContainer.mTabstrip._updateScrollButtonsDisabledState();
>+            }
> 
>             // ... and fix up the _tPos properties immediately.
>             for (let i = aTab._tPos; i < this.mTabs.length; i++)
>               this.mTabs[i]._tPos = i;

// Remove the tab ...
// ... and fix up the _tPos properties immediately.

these comments are connected. So can you insert your code after the _tPos updates?

_updateScrollButtonsDisabledState doesn't need to be moved down at all, afaict. And you should use this._windowIsClosing instead of aCloseWindow.
Summary: 'Close tab' button not shown → tab close buttons disappear when closing the first tab
Comment on attachment 394648 [details] [diff] [review]
patch v1

>+
>+            // Update Tab close button and scroll buttons state
>+            if (!aCloseWindow) {

Hmm, looking at the code above this more closely, I wonder if the if here should really be

if (!this._windowIsClosing) {
(In reply to comment #7)
> (From update of attachment 394648 [details] [diff] [review])
> >+
> >+            // Update Tab close button and scroll buttons state
> >+            if (!aCloseWindow) {
> 
> Hmm, looking at the code above this more closely, I wonder if the if here
> should really be
> 
> if (!this._windowIsClosing) {

Guess i should have checked for new comments before I added this. ;-)

Nice that you agree on this change though.  New patch on the way.

        Attachment #394648 -
        Attachment is obsolete: true

        Attachment #394657 -
        Flags: review?(dao)

        Attachment #394648 -
        Flags: review?(dao)

    
    

    
  


  

        Attachment #394657 -
        Attachment is obsolete: true

        Attachment #394657 -
        Flags: review?(dao)

    
  

        Attachment #394658 -
        Flags: review?(dao)

    
  

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

    
    

    
  
Comment on attachment 394658 [details] [diff] [review]
patch v2.1 - fix whitespace

Thanks!

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/3fff281f265f
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7
Version: Trunk → 3.5 Branch

    
  

        Attachment #394658 -
        Flags: approval1.9.2?

    
    

    
  
Verified fixed on all platforms with builds like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090904 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20090904044723

For me this is reproducible all the time for older builds:
1. Open three tabs.
2. Activate the second tab
3. Close the inactive first tab
=> Last tab lacks the close button

Can we get an automated test please? That should be easy to test.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Target Milestone: Firefox 3.7 → Firefox 3.7a1
Flags: blocking-firefox3.6? → blocking-firefox3.6+

    
  
Keywords: checkin-needed

    
  

        Attachment #394658 -
        Flags: approval1.9.2?

    
    

    
  
Verified fixed on 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090909 Namoroka/3.6a2pre ID:20090909040930

Bill, would you mind creating an automated test?
Keywords: verified1.9.2

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: