Closed Bug 103452 Opened 23 years ago Closed 22 years ago

javascript window.close should close tab, not complete browser window

Categories

(SeaMonkey :: Tabbed Browser, defect, P2)

Product:
SeaMonkey
Component:
Tabbed Browser
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: duncan, Assigned: jag+mozilla)

References

(
URL
)

Details

(Keywords: dataloss, Whiteboard: [ADT2 rtm][verified on trunk])

Attachments

(1 file, 6 obsolete files)

On window.close(), only close the tab, not the whole window
2.66 KB, patch
jag+mozilla
: review+
jag+mozilla
: superreview+
scc
: approval+
Details | Diff | Splinter Review 
i think javascript:self.close() should just close the current tab, when
using them, and not the whole browser window.

see the testcase url for a testcase
Confirming.  This could make tabs really annoying.  :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
New error on console after the window closes:

Error: _content is not defined
Source File: chrome://navigator/content/navigator.js
Line: 948

What would really help is seeing an object model of the new tab browser feature.
*** Bug 104711 has been marked as a duplicate of this bug. ***
*** Bug 104887 has been marked as a duplicate of this bug. ***
*** Bug 104888 has been marked as a duplicate of this bug. ***
Summary: javascript self.close should close tab, not complete browser window → javascript window.close should close tab, not complete browser window
Target Milestone: mozilla1.0 → mozilla1.0.1
*** Bug 108793 has been marked as a duplicate of this bug. ***
*** Bug 109745 has been marked as a duplicate of this bug. ***
*** Bug 111185 has been marked as a duplicate of this bug. ***
*** Bug 111851 has been marked as a duplicate of this bug. ***
QA Contact: blakeross → sairuh
This bug can cause a lot of dataloss. Adding keyword, changing prority to
critical and nominating for 0.9.9. (Also changing platform to All - please
change it back if I'm wrong.)
Severity: normal → critical
Keywords: dataloss, mozilla0.9.9
Hardware: PC → All
*** Bug 118342 has been marked as a duplicate of this bug. ***
*** Bug 119844 has been marked as a duplicate of this bug. ***
*** Bug 120077 has been marked as a duplicate of this bug. ***
Reassigning to new component owner.
Assignee: hyatt → jaggernaut
Status: ASSIGNED → NEW
voted and adding myself to cc ...
How common is the use of this function?  clearing target for re-triage by new
owner.
Target Milestone: mozilla1.0.1 → ---
This function is actually quite common. It is very noticable if you use Right
Click --> open in new tab often because you then quite often get windows which
the author intended to be new and chromeless and therefore entitled to close in
a tab. I have noticed this in my on-line banking service and sevaral other places.
I guess it's common enough that I can spend some time on this.

jst, is there an easy way for me to hook into window.close() and have it execute
some XUL js which can tell it to not proceed?
Look at GlobalWindowImpl::Close().
*** Bug 122946 has been marked as a duplicate of this bug. ***
*** Bug 123035 has been marked as a duplicate of this bug. ***
-> 1.0
Target Milestone: --- → mozilla1.0
nsbeta1+ per nav triage team
Keywords: nsbeta1 → nsbeta1+
My latest patch in http://bugzilla.mozilla.org/show_bug.cgi?id=113798
includes a hack for this too. But it is just a hack.
*** Bug 125126 has been marked as a duplicate of this bug. ***
*** Bug 127388 has been marked as a duplicate of this bug. ***
*** Bug 127838 has been marked as a duplicate of this bug. ***
Blocks: 128632
*** Bug 129665 has been marked as a duplicate of this bug. ***
*** Bug 129856 has been marked as a duplicate of this bug. ***
ADT2 per ADT triage team.
Whiteboard: [ADT2]
*** Bug 131411 has been marked as a duplicate of this bug. ***
*** Bug 131570 has been marked as a duplicate of this bug. ***
Uh oh, got bitten by this...
*** Bug 132766 has been marked as a duplicate of this bug. ***
Attached patch Patch extracted from bug 113798 (obsolete) — — Splinter Review 
Author: smaug@jippii.fi

Extracted patch from a set of tabbrowser patches. Categorized as hack by
author.
I just found out that I took the parts from an old patch. I'll try to make a new
one.

        Attachment #75781 -
        Attachment is obsolete: true

    
    

    
  
We shouldn't accept this patch. We could potentially be overriding a user
installed close function, something we don't want to do.

    
    

    
  
Alright, I'll bite: what's a 'user installed close function'?

    
    

    
  
jag wrote:
>We shouldn't accept this patch. We could potentially be overriding a user
>installed close function, something we don't want to do.

Yep, that was the reason for calling the patch a hack.
But if we change the close function at the beginning
of the loading then user could override it later?

    
    

    
  
Trudelle:

The patch basically does the following:

function myfunc () {
  // do something cool
}
window.close = myfunc;

This means that calling window.close() or close() now calls myfunc instead of 
the default built-in close-window function.

The point is that the page author can do something like:

function close () {
 // this is my close function
}

This sets window.close and this will either override our function definition or 
be overriden by our definition depending on which happens first.  The former 
outcome may be OK, the latter is unacceptable.

    
    

    
  
Can't we just set window.close to call the same code as CTRL+W does at the moment?

    
    

    
  
I think, the problem is more global. Window had been opened not via JavaScript,
must not have been closed by scripting without confirmation. Tab interface
intensifies situation, but root of all evils is located not in tabs.

NS4 and MSIE never close non-scripted windows without user's consent, and I
would expect same behavior of Mozilla.

    
    

    
  
Yes, consent is nice, but in my case, I'm looking at something in one tab, and I
open a tab to my banking application. I expect the close function to close the
tab, not the window, because I am using tabbed browsing.

If this is ambigious, like you THINK that window.close SHOULD close the window,
with all tabs in it, then there needs to be a pref to make the rest of us happy
that says, window.close closes the tab and not the window.

When going to tabbed browsing, NS4 and IE are not fair comparisons, since they
are not doing tabbed browsing. But I would argue that anything, even beyond
window.close, but any window action, should apply only to the tab and not the
window, meaning all tabs. Tabs should be considered their own window in one
windows,mac,Unix/Linix icon-saving friendly interface.

It's a larger can of worms to open, but one that should definitely be
considered. Maybe that's beyond the scope of THIS bug, which has caused data
loss for me in other tabs when window.close was called. But it still needs to be
thought out. Of course, someone wanting to pop-up a window probably doesn't want
to land in a tab, because they may be sizing that window. See where this can go?

    
    

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

    
    

    
  
OK, I think, this bug should be splitted to 2 separate threads:

1. javascript window.close should close tab, not complete browser window -
according description of #103452

2. javascript window.close closes non-scripted windows/tabs without confirmation
- this sould be separate bug thread, I have opened #133904 for this issue.

See http://bugzilla.mozilla.org/show_bug.cgi?id=133904

    
    

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

    
    

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

    
    

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

    
    

    
  
Hmmm, looks like quite a conundrum has arisen over this...

Consider: window.close() should not close a non-scripted window. I know this 
has been moved to another bug, but part of the problem might lie in how 
window.close() is treated.

Consider: window.close() should close the current tab in tab-mode and the 
current window in window-mode. This automatically suggests two distinct 
behaviors for the window.close() function. The question is, do we incorporate 
both functions statically, or can we reassign the function name itself to a 
different piece of code for each window and/or tab?

It might be worthwhile to consider having the window.close() function be a 
function pointer, for the purposes of abstraction. Thus, in window-mode, each 
window has its own window.close() function pointer (if scripted) which might 
point to one static function, then in tab-mode to have the window.close() 
function point to another static function so tabs are treated differently. 
Then, for both window- and tab-mode another function could be devised which 
asks the user's permission before closing the window. I'm not sure about any 
security risks concerning this, as I haven't looked at the code, but might it 
be possible to allow the pointer reassignment to be done only by the parent 
thread, and to declare any overt reassignments of window.close() within script 
to be illegal?

(I'm racking up quite a bill for talk today :-)

    
    

    
  
> and to declare any overt reassignments of window.close() within script 
> to be illegal?

This would likely break webpages that don't ever use window.close() but want to
define a variable or function called "close".

    
    

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

    
    

    
  
Hm. I suppose you are right at that.
I haven't even looked at the code. In fact, I haven't looked at any code in a 
good long while, being more concerned with system design lately, but I did do a 
little research on the problem.

It might be possible to have window.close() follow the current thread upwards 
to the parent. The parent thread could then determine the correct course of 
action for the window.close() code. I'm not sure how this could be implemented, 
since I haven't even looked at what is really going on, but it's an idea at 
least.

::Ah! I love the smell of microchips in the morning! Wait a minute...::

    
    

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

    
    

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

    
    

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

    
    

    
  
As a work around to the whole window closing, the following pref might be semi
useful. Of course with it, no windows will close with Window.close...

user_pref("capability.policy.default.Window.close", "noAccess");

No, It's nto a sulution, but may atleast help some of you.

    
  
Priority: -- → P2

    
    

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

    
    

    
  
This is in response to comment 50. It seems clear enough to me. Isn't window
mode the same as tab mode, but with only one tab ? If that's the case then just
close the current tab, and if there are no more tabs in the window, close the
window. That seems entirely consistent to me.

    
    

    
  
Exactly as stated in previous comment. We have implemented "close document"
gesture in Mozgest this way. Here is the code we use:

function closeDoc(){
  if (getBrowser().mTabContainer.childNodes.length > 1)
    getBrowser().removeCurrentTab();
  else window.setTimeout("window.close()", 10); 
}

Hope this helps.

    
  
Keywords: mozilla1.0

    
    

    
  
Until this bug 103452 is fixed, bug 32571 can not be considered completely
fixed, since window.close still closes windows it doesn't own - in other tabs.
Blocks: 32571

    
    

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

    
    

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

    
  
Whiteboard: [ADT2] → [ADT2 rtm]

    
    

    
  
voting for the bug and for solutions as suggested in #c59 anc #c60.

    
    

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

    
  
Keywords: mozilla0.9.9, 
          
            mozilla1.0 → mozilla1.0.1

    
    

    
  
I _very_ strongly disagree that this bug can wait till version 1.0.1 - it is a
_critical_ security fix!!

Not to mention it's an extremely simple fix!

    
    

    
  
It's simple?  Did I miss something?

    
    

    
  
Ariel Shkedi: This bug is still targeted to M1, but non-existing patch couldn't
be included to soon released M1. I suppose, that mozilla1.0 and mozilla0.9.9
keywords are redundant for this bug. BTW Jag has about 249 bugs and this bug is
one of the five with highest priority.

    
    

    
  
But it seems difficult/risky.  Unless someone has a flash of insight, this one
is probably not going to be fixed for MachV/1.0.1

    
    

    
  
Maybe, just deny window.close() for non-chrome scripts as a palliative?

    
    

    
  
Or deny window.close for any window that wasn't opened in javascript? (If that's
possible...)

This would be an ideal temporary measure since window.close is usually used in a
'Close' button on a javascript-generated popup window. Heck, you could get the
same effect by only allowing window.close when there's a URL bar visible! (since
most popup windows hide the URL bar).

Surely it's possible to implement some sort of temporary "fudge" that improves
things in most cases, until the bug is fixed for real. I admit the URL bar fudge
would be technically horrendous, but at least it would appear to work 95% of the
time as far as end-users are concerned.

    
    

    
  
It would be great if window.close closed a tab instead of a window. And closing 
of a tab, if it were the only tab, would close the window. For me, I have a 
logout button on a home banking site. Logout calls window.close; the problem is, 
it's usually in another tab, not it's own window.

    
    

    
  
It has been said that window.close should close the tab instead of the window;
it seems that this is because one could lose data in other tabs. But then, to be
consistent, I would say: it should only close the page(*) or perhaps a group of
pages belonging to the same site, because one could lose data that were in the
same tab, accessible with the Back button.

(*) a bit like clicking on Back (but one can't forward afterwards), and the tab
or window is closed if it was the first page.

    
    

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

    
  

        Attachment #75856 -
        Attachment is obsolete: true

        Attachment #75856 -
        Flags: needs-work+

    
    

    
  
Vincent Lefevre: that's not really a problem (that closing the tab/window looses
data from 'back' button). Because if bug 32571 were fixed only windows that were
opened via javascript (or chrome) will close automatically. Other windows will
give you a dialog and ask you if you want them to close.

    
    

    
  
Assuming resolution on approach, this seems like something we should go after in
1.0.1, any chance of an updated patch? minusing for now, please re-nominate if a
patch appears with consensus.
Keywords: mozilla1.0.1 → mozilla1.0.1-

    
    

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

    
    

    
  


  
This is part of one way of fixing this bug. This makes the implementation of
window.close() fire a DOMWindowClose event that event listeners in chrome code
can capture and if a listener prevents the default action of this event the
window won't be closed. The remaining part is to add the listener for this
event in the tab UI code...

    
    

    
  
I'm a little bit worried about jst's patch. I don't know a great deal about the
mozilla source, but I'd like to know: if the user is looking at tab 2 (for the
sake of argument), and tab 4 fires off a window.close(), could the patch result
in tab 2 being closed, instead of tab 4?

I could be wrong. I hope I'm wrong. But closing the wrong tab is only marginally
better than closing the whole window. Yes, I'm being obvious... I'll shut up
now. :-)

    
    

    
  
The window in which the event is fired is the one that will be closed,
independent of if the tab is the one that's currently displayed or not. And yes,
if tab 2 has permissions (and somehow got a reference to the other tab, which I
believe is impossible) to call tab 4's window.dispatchEvent() tab 2 could cause
tab 4 to close, but that's no different than tab 2 calling tab 4's
window.close() directly. IOW this patch does *not* let malicous scripts do
anything they can't already do.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        chrome part of patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #86009 -
        Attachment description: content part of patch → chrome part of patch

    
    

    
  
Um... that seems to have the exact problem described in comment 79.  That is, 
the tab that is closed is always the "current" tab, regardless of which tab 
actually fired the event.  We want to be checking the event target against the 
toplevel windows in all the tabs and closing the tab whose window matches the 
event target, no?

    
    

    
  
Yup, that's what we'd need to do...

    
    

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

    
    

    
  


  
So it looks like the value that DispatchEvent returns is the inverse of what it
should be, I'll talk to hewitt about this and file a new bug if necessary. In
the mean time I'm just inverting the check in jst's patch, and adding the
tabbrowser code to make this all work.

        Attachment #85860 -
        Attachment is obsolete: true

        Attachment #86009 -
        Attachment is obsolete: true

    
    

    
  


  
Attached the wrong file

        Attachment #86318 -
        Attachment is obsolete: true

    
    

    
  
Ignore that dump :-)

    
    

    
  
Comment on attachment 86322 [details] [diff] [review]
On window.close(), only close the tab, not the whole window

sr=jst for the tabbrowser.xml changes with the debug dump() removed.

    
    

    
  
sr=jag on the changes to nsGlobalWindow.cpp.

    
    

    
  
Comment on attachment 86322 [details] [diff] [review]
On window.close(), only close the tab, not the whole window

r=bryner

        Attachment #86322 -
        Flags: review+

    
    

    
  
Comment on attachment 86322 [details] [diff] [review]
On window.close(), only close the tab, not the whole window

sr=hewitt, but change noDefault to noPreventDefault, as we discussed

        Attachment #86322 -
        Flags: superreview+

    
    

    
  


  
It turns out the value returned is what the DOM says it should be, false if
preventDefault() was called, true if not. So it's really |executeDefault|. Imho
a renaming of those parameter names would be wise. Different bug though, new
patch with the var name changed for this patch and the dump removed.

    
  

        Attachment #86322 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 86328 [details] [diff] [review]
On window.close(), only close the tab, not the whole window

Carrying over r=hewitt, sr=jag, sr=jst

        Attachment #86328 -
        Flags: superreview+

        Attachment #86328 -
        Flags: review+

    
    

    
  
-> 1.0.1 (won't make 1.0), adt1.0.1 (want this for NS RTM), mozilla1.0.1 (want 
this in that release), fixed (trunk).
Status: NEW → RESOLVED
Closed: 22 years ago
Keywords: mozilla1.0.1- → adt1.0.1, 
          
            mozilla1.0.1
Resolution: --- → FIXED
Target Milestone: mozilla1.0 → mozilla1.0.1

    
    

    
  
I (and we) greatly appreciate your fixing this bug! But are you sure this can't
make it into 1.0?

The entire world will be looking at the release - and tabs is going to be at the
top of reviewers lists because it's a feature IE doesn't have.

A bug like this - which reviewers _are_ going to notice is going to be
embarrassing for mozilla. Especially because of bug 32571 it's going to look
like the browser just crashed! (And bug 32571 just noted a target of
mozilla1.2alpha!)

You have working code - can't you ask for an exception (if necessary) and
squeeze this in? I think this bug deserves an exception if one is needed.

    
    

    
  
Only bugs that will make this universe cease to exist would be able to delay
shipping of Mozilla 1.0 at this point, and even then it's a big if.

    
    

    
  
sairuh - can you pls verify this one on the trunk, and check for any
regresssions? thanks!
Blocks: 143047

    
    

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

    
    

    
  
If I call opener.close() from a window opened by code in a tab, the tabbed
window will become the top window not the one opener.close() called from.
New bug or result of this and so stay here?

    
    

    
  
If it is a bug (could one see it as a feature?), I would say it's a bug that's
exposed because we got this working. Could you file a new bug on it for further
investigation?

    
    

    
  
I note that no one ever responded to comment #42 which, as an
outsider, and as a (new) user, I find odd, since ctrl+W, despite the
mnemonic, executes the operation I expect and want to execute -- close
*tab* (naturally, a tabless window disappears), and the fact that this
bug exists indicates, I believe, improper (or absent) modelling of the
UI.  From a user perspective, tabs almost replace browser windows.  I
have already lost large amounts of data through habitually typing
alt+F4 to close a "window" which was actually a tab -- I'm trying to
break that habit, but it deserved at least a relnote.  Another very
serious consequence of this failure in UI modelling is that there is
no apparent way to open a bookmark into a tab -- this greatly reduces
the usefulness of tabs.  And, there are missing features that would
have been obviously desirable given a fully modelled UI -- such as being
able to drag and drop a tab from one window to another.

    
    

    
  
I'll respond then.  the code that CTRL+W does is to look at the tab that is
currently open and then close that.  It would suck if you loaded a page, it has
some JS that does stuff for a few minutes, you switch to a different tab, and
then we call the code that CTRL+W does which closes the currently active tab
(different than the tab that called window.close since you have switched tabs).

This bug is fixed.  Please take any other issues/requests to different/new bugs.
 Thanks

    
    

    
  
vrfy'd fixed using 2002.06.07.08-trunk bits on linux rh7.2.
Whiteboard: [ADT2 rtm] → [ADT2 rtm][verified on trunk]

    
    

    
  
Jim Balter: What was your point in comment 42? that's more or less what the
patch does. It requires cooperation between the window container and the tab
control since unlike keystroke events there's no way for the tab control to
intercept the user directly tickling the window object (although cf. the clever
initial attempt to override the .close() method on the prototype, ultimately
prevented by a recent security patch).

As to bookmarks, menu items have a very simple UI -- you click them or not. If
you want to use the bookmark menu you will have to open a tab first. If you have
items in your personal toolbar you can drag them to an empty bit of the tab area
and it will open a new tab. If you use the bookmark sidebar you can drag links
to an empty bit of the tab area to open a new tab.

It would be consistant if you could drag the icon on the tab, but for the open
tab you can drag the same icon up in the URL bar to the tab area of another
window to open it in a new tab. If you drag it to the content area itself it
will open in the existing tab. If you think this is something you want to do a
lot you probably want to uncheck the "Hide tab bar when only one tab is open" pref.

    
    

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

    
    

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

    
    

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

    
    

    
  
Comment on attachment 86328 [details] [diff] [review]
On window.close(), only close the tab, not the whole window

a=scc (on behalf of drivers) for checkin to the mozilla1.0 branch.

when you check this into the branch, please change the mozilla1.0.1+ keyword to
fixed1.0.1

        Attachment #86328 -
        Flags: approval+

    
  
Keywords: mozilla1.0.1 → mozilla1.0.1+

    
    

    
  
Marking verified per comment 103
Status: RESOLVED → VERIFIED

    
    

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

    
    

    
  
adt1.0.1+ added.
Keywords: adt1.0.1 → adt1.0.1+

    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
  
Please check this in asap and change the mozilla1.0.1 keyword to fixed1.0.1

    
  
Keywords: mozilla1.0.1+ → fixed1.0.1

    
    

    
  
verified on the branch using 2002.06.27.0x comm bits (all platforms).
Keywords: fixed1.0.1 → verified1.0.1

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

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

    
    

    
  
*** Bug 172347 has been marked as a duplicate of this bug. ***
Product: Core → SeaMonkey

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: