Closed Bug 173703 Opened 17 years ago Closed 17 years ago

Closing a tab left to the active tab using middle-click displays a blank screen

Categories

(SeaMonkey :: Tabbed Browser, defect, major)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: emaijala+moz, Assigned: jag+mozilla)

References

Details

(Keywords: regression)

Attachments

(2 files, 8 obsolete files)

Using trunk build 2002100808 using Modern theme. Open a new browser window. Open
two tabs. Load something to both of them. Activate the rightmost tab. Middle
click on the left tab to close it. Tab bar disappears and a blank window is
displayed. Pressing Ctrl-T will bring tab bar back and the content of the
remaining tab can be seen again. If I uncheck "Hide the tab bar when only one
tab is open" the tab bar remains visible but the content doesn't.
I'm also seeing this in 2002100920 (Modern theme) and Phoenix 20021010 (default
theme). In addition to the previously described behavior, URLs typed in the
Location bar are not loaded after the window is blanked (until you switch to
another tab).
QA Contact: sairuh → pmac
*** Bug 174623 has been marked as a duplicate of this bug. ***
Mozilla 2002101508 on WinNT4.

Steps to reproduce:

1) open two or more tabs
2) select the last one
3) open the JS console and clear all possible warnings/errors
4) close a tab with a middle-click on the tab (but not the last one)
result: the current tab turns gray
expected result: it should select the right tab and display it.

Now, here are the errors I see on my JS console:
Error: newBrowser has no properties
Source File:
chrome://multiviews/content/bindings/tabbrowser.xml#tabbrowser.updateCurrentBrowser()
Line: 6

Error: _content has no properties
Source File: chrome://communicator/content/contentAreaUtils.js
Line: 45

Error: oldBrowser has no properties
Source File:
chrome://multiviews/content/bindings/tabbrowser.xml#tabbrowser.removeTab()
Line: 30

note: you might need to close the activate/selected tab to see all three.
CC'ing SPAM
I see pretty much the same behaviour in build 2002101504 On Win2K with Modern.
It appears that if at the time a tab is closed, if the active tab is loading, it
renders blank. If there were more than two tabs open originally, it is possible
to switch tabs (and return) and see the content. This also works if you open a
new tab, switch to it, and return to the first tab.
On the other hand, if you minimize and restore the window, it is still blank.
note: you might need to change "middlemouse.contentLoadURL" on Linux to close a
tab with a mouse middle-click.
i hit this yesterday (using a 20021012 build on win2k). seems pretty serious to
me - fixable if you know what's happened, but if not, completely breaks the
browser...
Severity: normal → major
Attached patch Proposed patch (obsolete) — Splinter Review
I'm not sure why catching the tabpanel's onselect event isn't working, so this
catches the tabs' onselect event instead.
Keywords: patch, review
Attachment #103189 - Flags: review+
Blocks: 1.2
Attached patch Extra check (obsolete) — Splinter Review
Found a case which I wasn't fixing before :-(
Attachment #103189 - Attachment is obsolete: true
I don't have CVS access right now so I just add it like this:

file: tabbrowser.xml 
814             this.mTabContainer.removeChild(oldTab);
815             this.mPanelContainer.removeChild(oldBrowser);
816 +           this.mPanelContainer.selectedIndex = newIndex;
817 +           this.selectedTab = this.mTabContainer.childNodes[newIndex];
This is the change we wanna make. Don't preventBubble on select events from
tabpanels, don't move the onselect (not just yet anyway).
Attachment #103226 - Attachment is obsolete: true
Oh, my change to the fix for this in phoenix is that I set
mTabContainer.selectedIndex directly instead of going through this.selectedTab
(which ends up doing the same, but with more work).
Jag,

who are you fooling?

if (newIndex == index) { 
and 
if (this.mCurrentBrowser == newBrowser)
are almost the same things, but the latter is the slower one, because you need
to jump to another method first. 

Also note that you need to set .selectedIndex directly, or you end up with yet
another bug report ;) I'vre learned that the hard way, because we're using my
patch for some time already :-)
I'm not fooling anyone. I'm just saying which patch was applied to Phoenix. I'm
fine with moving the test back to where it was, was gonna ask hyatt about why he
made that change.

So which selectedIndex are you talking about (the change I made I'm guessing)
and what bug does using .selectedTab cause?
Jag, do you remember that nasty bug where people can't close tabs? Bingo :-)
Gotta love answers that only hint but don't make clear ;-)
Attached patch Patch based on HJ's patch (obsolete) — Splinter Review
I modified HJ's patch to make the link toolbar patch (bug 102905) work, because

I don't want its select event firing before the call to updateCurrentBrowser().
Doh, I forgot about the 'Side Navigation Bar', because I never use it ^_~
It's because I had patched my build to fix the site navigation bar that I didn't
realize the effect on the tabbrowser :-/ P.S. I assume that means that although
having a working tabbrowser you won't review the site navigation bar patch?
Comment on attachment 103557 [details] [diff] [review]
Patch based on HJ's patch

What would need to be fixed in the site navigation toolbar to not have to have
this quirky ordering in tabbrowser?
Jag is right, we shouldn't have to change the ordening. Please read
http://bugzilla.mozilla.org/show_bug.cgi?id=102905#c71 because that will solve
both problems at once.
*** Bug 175267 has been marked as a duplicate of this bug. ***
So the right fix is to listen for select events from <tabpanels> instead of
<tabs>. New patch coming up.
Attached patch The best of both worlds. (obsolete) — Splinter Review
Attachment #103403 - Attachment is obsolete: true
Attachment #103557 - Attachment is obsolete: true
"The best of both worlds." 

That's not exactly true. First of all, if (newIndex == index) is only true when
you close the tab at the direct left side of the selected tab. So why call
updateCurrentBrowser if it returns right away because of your if
(this.mCurrentBrowser == newBrowser) return; in it? The question is, do we need
to call updateCurrentBrowser there, yes or no? I don't think so.
Hmmm... I think we still need to fire it. It's a bit complicated, so bear with me:

For the case where a tab before the currently selected one is removed,
selectedIndex changes on <tabs> since we want the selected to remain the same,
but since the tab still had the selected attribute set to true, the rest of the
setter is skipped. Then we set selectedIndex on <tabpanels>, which sees that the
val is different than what it currently has, and thus fires an event on
<tabpanels>, which causes us to call updateCurrentBrowser. So far, so good.

For the case where a tab after the currently selected one is removed,
.selectedIndex remains the same for both <tabs> and <tabpanels>, and both
functions skip their body and thus skip firing events, meaning
updateCurrentBrowser isn't called. Also good, since we didn't need to update the
rest of the UI.

For the case where the currently selected tab is removed, selectedIndex on
<tabs> changes, the new tab doesn't have |selected| set, so we fix all that up,
set selectedIndex on <tabpanels>, but unless the removed (and selected) tab was
the last tab, selectedIndex didn't change for <tabpanels>, so we skip that code
and don't fire the event (for the last tab we actually do fire the event and
everything's fine). Then we get back into the selectedIndex setter on <tabs>,
which fires the event on <tabs>. Then we set selectedIndex on <tabpanels>, which
by now should have no effect (since it was already done from the <tabs>
selectedIndex setter, in this case). So for this one case, where currentIndex ==
index && currentIndex != l - 1, we still need to call updateCurrentBrowser.
That test could also be written as |index == currentIndex && index == newIndex|.

It's kinda messy, isn't it.
From the index < currentIndex case:

> Then we set selectedIndex on <tabpanels>, which sees that the
> val is different than what it currently has, and thus fires an event on
> <tabpanels>, which causes us to call updateCurrentBrowser. So far, so good.

Actually, we really don't need to call updateCurrentBrowser here, since the
currentBrowser didn't change.

So, erh... It's a mess, like I said :-)

Adding the guard in updateCurrentBrowser will at least help us do a little less
work for this case.
Btw, (index == newIndex) when you close the tab at the direct left side of the
selected tab, but also when you close the selected tab and it's not the last tab
(since we select the tab to the right).

Also, the thing to realize is that even though the two indices might be the
same, newBrowser might not yet be the same as mCurrentBrowser (since it's
updateCurrentBrowser that fixes up mCurrentBrowser, and it might not have been
called, as shown in the third case).

I think we might be fine if we fix <tabpanels>' selectedIndex setter to fire in
the same manner <tabs>' selectedIndex setter does.
Comment on attachment 103226 [details] [diff] [review]
Extra check

> It's kinda messy, isn't it.

Which is why I want to update the current browser when the current <tab>
changes, not when the tabpanel selectedIndex changes. You still need to set the
tabpanel selectedIndex manually, in case the current <tab> doesn't change
(although this attachment could still be improved using the
this.mTabContainer.selectedIndex = newIndex; optimization).
Attachment #103226 - Attachment is obsolete: false
Attachment #103226 - Flags: needs-work+
*** Bug 176286 has been marked as a duplicate of this bug. ***
OS: All since Linux is affected too
OS: Windows 2000 → All
*** Bug 176475 has been marked as a duplicate of this bug. ***
Builds 2002102108, 2002102322
Additionally, if I load a bookmark in the "blanked" tab it'll open a new browser
WINDOW every time :-/
Instead, _typing_ a URL will NOT open a new window.
Is this something that is 1.2 material?  It's on the list and it has a patch.
I bet we'll get tons of obscure bug reports if this is left broken. 
CCing a few people because this is a highly visible regression not present in
earlier releases.
*** Bug 176521 has been marked as a duplicate of this bug. ***
Hey, when was this introduced? What if we backout the two most recent patches
for tabbox.xml?
jag, could this be what you want?
Alternatively, just fix it :)
*** Bug 177982 has been marked as a duplicate of this bug. ***
           if (next && next != startTab) {
-            this.selectedItem = next;
             next.focus();
-            document.commandDispatcher.advanceFocusIntoSubtree(next);
+            this.selectedItem = next;
           }

I like it, but what's the reason for the above change?
Attachment #103226 - Attachment is obsolete: true
Attachment #103707 - Attachment is obsolete: true
Attachment #103965 - Attachment is obsolete: true
Attachment #104487 - Attachment is obsolete: true
Neil, why do you need the extra event handler if you can use onselect on the
tabbox? All select events run into this |if (event.originalTarget ==
this.mPanelContainer)| check, so that must slow things down a bit. This
shouldn't be necessary when the tabbox selects the right panel.
Attachment #105181 - Flags: superreview+
Comment on attachment 105181 [details] [diff] [review]
After much consultation on irc...

+	     this.mTabContainer.selectedIndex =
this.mPanelContainer.selectedIndex = newIndex;

Except for that line I like it :-)

Split that up and sr=jag
Comment on attachment 105181 [details] [diff] [review]
After much consultation on irc...

agreed
Attachment #105181 - Flags: review+
Comment on attachment 105181 [details] [diff] [review]
After much consultation on irc...

r=HJ thanks Neil
*** Bug 178462 has been marked as a duplicate of this bug. ***
Blocks: 175445
*** Bug 176298 has been marked as a duplicate of this bug. ***
Same bug On Mac OS 9.1, Build 2002110408

Platform to ALL ??
Platform -> All per comment 51
Hardware: PC → All
Comment on attachment 105181 [details] [diff] [review]
After much consultation on irc...

>+            this.mTabContainer.selectedIndex = this.mPanelContainer.selectedIndex = newIndex;

After yet more thought, I have seen the error of my ways, and will change/ask
whoever gets to check in the code to change it as follows, if nobody has any
objections:

// When the current tab is removed select a new tab
// This will fire all the right select events
this.mTabContainer.selectedIndex = newIndex;
// When removing a tab to the left of the current
// fix up the panel index without firing any events
this.mPanelContainer.selectedIndex = newIndex;
Attachment #105181 - Flags: needs-work+
Attached patch Overkill?Splinter Review
You don't want me to move stuff around like this do you?
Come on...
Comment on attachment 105325 [details] [diff] [review]
Split line and added comments

sr=jag
Attachment #105325 - Flags: superreview+
A quick note, selectedTab, that's not always the right one. We're should I look
for that?
*puzzled look* Your note's too quick.
Attachment #105181 - Attachment is obsolete: true
Comment on attachment 105181 [details] [diff] [review]
After much consultation on irc...

jag: I think I see what HJ means with my old patch as the chained assignment
set the selectedIndex properties in the wrong order; I should have ensures a
tab was selected before trying to fix the tabpanel index.
So ... is your last patch okay, or not?
Comment on attachment 105325 [details] [diff] [review]
Split line and added comments

Neil, lets get this thing going, r=hj
Attachment #105325 - Flags: review+
jag: yes, I could only reproduce HJ's bug with my mischained assignment.
Fix was checked in by timeless.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Since this is a regression, drivers may want this in 1.2. Can you ask them?
jag: this bug already IS on bug 174647 (make 1.2 not suck) dependecies... i
think they DO want it - and it should be easy to get a=
Comment on attachment 105325 [details] [diff] [review]
Split line and added comments

Yes, we do want this for 1.2. a=asa for checkin to the 1.2 branch (on behalf of
drivers).
Attachment #105325 - Flags: approval+
Fix verified on trunk build 2002111108 PC/Win98.
Leaving status as RESOLVED pending verification of fix on 1.2 branch.
Fix verified on build 2002111122 PC/linux too.
I was going to wait until someone marked the bug as verified...
Works now on Mac OS 9.1 Build 2002111108
Uhm - I hate to say it but I can reproduce this bug on build 2002111211 in
Win9x. I just updated an hour ago from 20021106?? (forgot what it was already)
and I ran into this bug.
Dash - ha... I ran into the same thing. there was a mix up with the new win32
1.2 branch builds - the 2002111211 build in the latest-trunk folder is actually
a 1.2 build, not a trunk (1.3a) build (which you can check in help:about), and
this fix is not in the 1.2 branch builds yet. unless you're seeing this in a
build labelled 1.3a, there is no problem :)
I notice this is still not fixed in the 1.2 branch, but it WFM with the latest
trunk (on win2k). what's the hold up with checking the fix into 1.2?

seems it was left as "resolved" pending verification on the branch, and not
checked into the branch pending being marked "verified". that's not gonna work...
*** Bug 174141 has been marked as a duplicate of this bug. ***
*** Bug 175445 has been marked as a duplicate of this bug. ***
*** Bug 179473 has been marked as a duplicate of this bug. ***
*** Bug 177873 has been marked as a duplicate of this bug. ***
This has been checked into the 1.2 branch.
*** Bug 180881 has been marked as a duplicate of this bug. ***
Verified on in a build labelled 1.3a, netscape trunk build: 2002-12-10-08-TRUNK)

*** Bug 186523 has been marked as a duplicate of this bug. ***
*** Bug 187695 has been marked as a duplicate of this bug. ***
Product: Core → SeaMonkey
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.