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

Status: VERIFIED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Ere Maijala (slow)]

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.

Comment 1

[Ryan Grove]

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

Comment 2

[A. Craig West]

*** Bug 174623 has been marked as a duplicate of this bug. ***

Comment 3

[HJ]

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.

Comment 4

[HJ]

CC'ing SPAM

Comment 5

[A. Craig West]

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.

Comment 6

[HJ]

note: you might need to change "middlemouse.contentLoadURL" on Linux to close a
tab with a mouse middle-click.

Comment 7

[Michael Lefevre]

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

Comment 8

[[e-mail]]

Attachment: Proposed patch

I'm not sure why catching the tabpanel's onselect event isn't working, so this
catches the tabs' onselect event instead.

Comment 9

[[e-mail]]

Attachment: Extra check

Found a case which I wasn't fixing before :-(

Comment 10

[HJ]

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];

Comment 11

[jag (Peter Annema)]

Attachment: Patch that was applied to Phoenix (slightly modified)

This is the change we wanna make. Don't preventBubble on select events from
tabpanels, don't move the onselect (not just yet anyway).

Comment 12

[jag (Peter Annema)]

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

Comment 13

[HJ]

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 :-)

Comment 14

[jag (Peter Annema)]

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?

Comment 15

[HJ]

Jag, do you remember that nasty bug where people can't close tabs? Bingo :-)

Comment 16

[jag (Peter Annema)]

Gotta love answers that only hint but don't make clear ;-)

Comment 17

[[e-mail]]

Attachment: Patch based on HJ's patch

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

Comment 18

[HJ]

Doh, I forgot about the 'Side Navigation Bar', because I never use it ^_~

Comment 19

[[e-mail]]

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 20

[jag (Peter Annema)]

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?

Comment 21

[HJ]

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.

Comment 22

[Daniel de Wildt]

*** Bug 175267 has been marked as a duplicate of this bug. ***

Comment 23

[jag (Peter Annema)]

So the right fix is to listen for select events from <tabpanels> instead of
<tabs>. New patch coming up.

Comment 24

[jag (Peter Annema)]

Attachment: The best of both worlds.

Comment 25

[HJ]

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

Comment 26

[jag (Peter Annema)]

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.

Comment 27

[jag (Peter Annema)]

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.

Comment 28

[jag (Peter Annema)]

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 29

[[e-mail]]

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

Comment 30

[R.K.Aa.]

*** Bug 176286 has been marked as a duplicate of this bug. ***

Comment 31

[R.K.Aa.]

OS: All since Linux is affected too

Comment 32

[[e-mail]]

Attachment: Alternative - only update when closing the current tab

Comment 33

[Pratik]

*** Bug 176475 has been marked as a duplicate of this bug. ***

Comment 34

[Mike Neman]

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.

Comment 35

[Christopher Blizzard (:blizzard)]

Is this something that is 1.2 material?  It's on the list and it has a patch.

Comment 36

[Ere Maijala (slow)]

I bet we'll get tons of obscure bug reports if this is left broken. 

Comment 37

[Arthur]

CCing a few people because this is a highly visible regression not present in
earlier releases.

Comment 38

[Bill Mason]

*** Bug 176521 has been marked as a duplicate of this bug. ***

Comment 39

[HJ]

Hey, when was this introduced? What if we backout the two most recent patches
for tabbox.xml?

Comment 40

[[e-mail]]

Attachment: Alternatively, make select events more selective :-) (untested)

jag, could this be what you want?

Comment 41

[Ere Maijala (slow)]

Alternatively, just fix it :)

Comment 42

[R.K.Aa.]

*** Bug 177982 has been marked as a duplicate of this bug. ***

Comment 43

[jag (Peter Annema)]

           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?

Comment 44

[[e-mail]]

Attachment: After much consultation on irc...

Comment 45

[HJ]

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.

Comment 46

[jag (Peter Annema)]

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 47

[HJ]

Comment on attachment 105181 [details] [diff] [review]
After much consultation on irc...

agreed

Comment 48

[HJ]

Comment on attachment 105181 [details] [diff] [review]
After much consultation on irc...

r=HJ thanks Neil

Comment 49

[Bill Mason]

*** Bug 178462 has been marked as a duplicate of this bug. ***

Comment 50

[jag (Peter Annema)]

*** Bug 176298 has been marked as a duplicate of this bug. ***

Comment 51

[Jean-Pierre Melkonian]

Same bug On Mac OS 9.1, Build 2002110408

Platform to ALL ??

Comment 52

[Bill Mason]

Platform -> All per comment 51

Comment 53

[[e-mail]]

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;

Comment 54

[[e-mail]]

Attachment: Overkill?

You don't want me to move stuff around like this do you?

Comment 55

[Ere Maijala (slow)]

Come on...

Comment 56

[[e-mail]]

Attachment: Split line and added comments

Comment 57

[jag (Peter Annema)]

Comment on attachment 105325 [details] [diff] [review]
Split line and added comments

sr=jag

Comment 58

[HJ]

A quick note, selectedTab, that's not always the right one. We're should I look
for that?

Comment 59

[jag (Peter Annema)]

*puzzled look* Your note's too quick.

Comment 60

[[e-mail]]

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.

Comment 61

[jag (Peter Annema)]

So ... is your last patch okay, or not?

Comment 62

[HJ]

Comment on attachment 105325 [details] [diff] [review]
Split line and added comments

Neil, lets get this thing going, r=hj

Comment 63

[[e-mail]]

jag: yes, I could only reproduce HJ's bug with my mischained assignment.

Comment 64

[[e-mail]]

Fix was checked in by timeless.

Comment 65

[jag (Peter Annema)]

Since this is a regression, drivers may want this in 1.2. Can you ask them?

Comment 66

[Daniel Steinberger]

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 67

[Asa Dotzler [:asa]]

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

Comment 68

[Bill Mason]

Fix verified on trunk build 2002111108 PC/Win98.
Leaving status as RESOLVED pending verification of fix on 1.2 branch.

Comment 69

[Mike Neman]

Fix verified on build 2002111122 PC/linux too.

Comment 70

[[e-mail]]

I was going to wait until someone marked the bug as verified...

Comment 71

[Jean-Pierre Melkonian]

Works now on Mac OS 9.1 Build 2002111108

Comment 72

[Dash McElroy]

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.

Comment 73

[Michael Lefevre]

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 :)

Comment 74

[Michael Lefevre]

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

Comment 75

[[e-mail]]

*** Bug 174141 has been marked as a duplicate of this bug. ***

Comment 76

[Torben]

*** Bug 175445 has been marked as a duplicate of this bug. ***

Comment 77

[Torben]

*** Bug 179473 has been marked as a duplicate of this bug. ***

Comment 78

[Torben]

*** Bug 177873 has been marked as a duplicate of this bug. ***

Comment 79

[Christopher Blizzard (:blizzard)]

This has been checked into the 1.2 branch.

Comment 80

[Torben]

*** Bug 180881 has been marked as a duplicate of this bug. ***

Comment 81

[Patty Mac]

Verified on in a build labelled 1.3a, netscape trunk build: 2002-12-10-08-TRUNK)

Comment 82

[u60234]

*** Bug 186523 has been marked as a duplicate of this bug. ***

Comment 83

[u60234]

*** Bug 187695 has been marked as a duplicate of this bug. ***