68215 - onbeforeunload event

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement)

Description

[Keith Lesch]

This does not appear to be in the specification, so I will understand if this
RFE is turned down, but I will argue my case anyway.  :-)

When you have a large form on a web application, you want to catch people before
they unload a page (if they edited it).  If they have edited the form, you want
to make sure they meant to leave the page and it wasn't accidental, because they
may have made alot of changes.  A number of accidental keystrokes could cause a
new page to load and the previous form to be lost.  Therefore the onbeforeunload
is a perfect time to see if they made changes, and if so make sure they meant to
abandon those changes.

If anyone has an idea for how to implement this *without* implementing an
onbeforeunload event, please let me know.  I realize that I could overload all
links on the page with a javascript that could check for this, however that
would not cover the BACK button, [ALT + LEFT ARROW], Bookmarks, etc.

Comment 1

[Tim Powell]

I've had similar struggles and have worked to make leaving the page and
returning to it safer. The biggest thing to do is make sure that the page does
not expire. In addition, I've being arguing that Mozilla needs to maintain
state. In particular, hidden fields need to retain their values--see bug 55988.
Similarly, button labels should retain state--see bug 64415.

I agree that onbeforeunload would be very useful to have for web applications.
It'd be nice if we could use this in both IE and Mozilla.

Comment 2

[Keith Lesch]

Well, let's consider the situation that a user actually submits the form rather
than leaving the page without submitting it.  In this case, it is VERY important
that the page expires immediately.  If the user hits the back button, they will
get the old form and they can actually save over [erase] the changes they
originally made.  Similarly, if someone ELSE has edited the data that has been
loaded into the form, you do not want to give the user and "old" copy of the
data by restoring a cached page.  Therefore I have some issues with considering
use of the cache...

The other bugs you mention are also ones I have dealt with, but could work
around.  This one however could cause some upset users...  :-)   I have
generally been relying on cookies rather than hidden form elements...but I
realize that some people do not have the luxury of using them, or have much more
data hidden than would be logical to throw into a cookie.

Comment 3

[joki (gone)]

Futuring all RFEs

This bug has been marked "future" because the original netscape engineer
working on this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work
in some way -- please attach your concern to the bug for reconsideration.

Comment 4

[Tim Powell]

This bug is a non-trivial limitation of Mozilla. Because IE5 supports this 
event, safer and better DHTML applications can be created than in Mozilla. The 
onUnload event fires too late to be helpful. For example, you can't show 
a "You're about to lose your work. Are you sure you want to leave?" type of 
dialog that would let you not leave. The onBeforeUnload event makes this easy. 
I'd be really surprised if this isn't in future JavaScript versions, since it 
is so useful.

I'd imagine it'd be fairly easy to implement this. Just fire onBeforeUnload at 
the start of leaving the page and allow it to return and prevent unloading the 
page. (I'd imagine that onUnload fires at about the same time, but can't be 
interrupted.)

Proposing for Mozilla 1.0, since this is one item that's preventing me from 
creating DHTML applications that are as powerful as in IE. Also added the 
dataloss keyword, although this may be an abuse of it, it's certainly true that 
the lack of this event in Mozilla prevents web applications from saving users 
from dataloss.

Comment 5

[Blake Ross]

Wondering what jst and hixie think about this...

Comment 6

[Johnny Stenback (:jst)]

If someone comes up with a patch for this I'd accept the change (assuming the
fix itself is acceptable), but I have no time to spend on this. Does IE support
setting this event both as element.unbeforeunload=... and <body
onbeforeunload=...>? Can this be abused to block people from leaving a page?

Comment 7

[Tim Powell]

Here's the relevant microsoft documentation about this event.
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeunload.
asp

Some key points:
* This is supported for both inline HTML <body onbeforeunload="handler()"> and 
event.onbeforeunload = handler();
* You can't block the page unload without forcing a dialog box to appear. You 
can return a string to the onbeforeunload event which is shown in a dialog box. 
You have a chance to try to persuade or warn people from leaving the page, but 
can't prevent it.
* The dialog generated by IE when returning a string to this event has the 
following format: Title says "Microsoft Internet Explorer". The dialog 
says "Are you sure you want to navigate away from this page?", a blank line, 
the text string you supply, and the text "Press OK to continue, or Cancel to 
stay on the current page." The dialog has OK and Cancel buttons.

Comment 8

[Hixie (not reading bugmail)]

Sounds good; go for it. Someone should propose it to the W3C DOM WG.

Comment 9

[Madhur Bhatia]

QA contact updated

Comment 10

[Niels Aufbau]

Onbeforeunload should not be implemented:

1. Web pages primarily use this event to work around a bug in Internet Explorer
that prevents it from reliably saving form data (eg in textareas) in session
history.  Mozilla does not have this bug, so Mozilla users should not be subject
to extra dialogs put up by web page authors trying to prevent dataloss in IE.

2. I often intentionally close a browser window after beginning to fill out a
form, and I believe that other users do the same.  (general dialog fatigue)

3. If the web page author really wanted to save the data in an emergency
situation where the user has closed the window, she would stuff the data into a
cookie (which works in all browsers) rather than relying on an IE-only event.

4. Internet Explorer does not care how hard it is for users to leave pop-up
hell.  I hope Mozilla does, because the main reason I'm here is
dom.disable_open_during_load.  

5. The proposed dialog would be especially dangerous because, from the point of
view of someone trying to leave pop-up hell, the "safe" option is "yes".  What
will happen if a site pops up an installation dialog as you try to close it? 
(specific dialog fatigue)

I recommend wontfixing this bug.

Comment 11

[Jeffrey Steele]

Without this functionality, I cannot support Moz / Netscape for our Web-based 
intranet application...  We are writing an Application, and as such, need more 
control over the user's action... This event allows for greater flexability on 
our part, and can prevent data loss (what if the amount of data present on the 
page exceeds the max cookie size???).  In our application this event is user 
configurable, if they do not like the pop-up warning, they can turn it off...

Just my 2cents, but again, without this, we cannot offer a choice in browsers 
to our clients (which means IE only).  And I'd really like to be able to offer 
platform independance...

Comment 12

[Andrew Hagen]

This bug is important to enterprise users who want to use Mozilla or Netscape as
a data entry tool. 

Once implemented, there should be a user-configurable option to turn this on,
which enterprise users could use. The default setting should be "off." That
would likely keep everyone happy.

Comment 13

[Hixie (not reading bugmail)]

We could simply add this to the prefs in the popup blocking prefs panel.

Comment 14

[Andrew Hagen]

That's a good idea. I second the motion.

Comment 15

[Jesse Ruderman]

Onbeforeunload may become unnecessary when bug 48333 is fixed.

Comment 16

[Tim Powell]

Thanks for the pointer, Jesse. Bug 48333 addresses a different problem. It 
would not make up for the lack of the onbeforeunload event, but may help some 
marginal cases. Onbeforeunload handles leaving the page, not just closing the 
window. Onbeforeunload is also under web-developer control and therefore can be 
more informative.

Comment 17

[z-moz]

I vote for adding this feature as well. Just because Mozilla correctly 
remembers form information doesn't mean this event isn't useful. Many users may 
not understand that if a form is not submitted it's contents won't be saved. 

Take a webmail application for example. If someone is composing a message, and 
decides to click over to their inbox to reference a different message, then 
clicks the compose tab again (rather than using the "Back" button), they may be 
surprised that the message they were working on is gone. 

I also agree that you should be able to turn it off in browser preferences if 
it annoys you. 

Comment 18

[Charles Manske]

The editor group is working on embedding technology and will definitely need
this feature. There are people like Xopus and Bitflux XML editor groups that
are trying to mozilla and really need this feature.
Reassigning to saari to get it on the radar.

Comment 19

[saari (gone)]

adding to editor embedding meta bug
http://bugzilla.mozilla.org/show_bug.cgi?id=157128

Comment 20

[Christian Stocker]

I agree with Charles, we really need this feature. Otherwise people will loose
data from time to time (hitting back without intention et al.) and not using
Mozilla as an application. But most is said anyway in this thread, so please
implement it :)

chregu

Comment 21

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 22

[Jesse Ruderman]

Tim: I think the dialog from bug 48333 will also appear if you navigate away
from the page accidentally by hitting "Back".  It would be strange to have the
dialog appear later, such as when you click a link (removing the page with the
form from session history) or close the window with another page visible.  I
still think onbeforeunload will be unnecessary for preventing dataloss once bug
48333 is fixed.

I agree with Niels (comment 10) that it's important to avoid adding dialogs that
are likely to be used on malicious porn/pop-up sites but have "yes" (or whatever
the first button is) as the "safest" answer.  Here, "safest" means "most likely
to get me out this annoying site, which has established itself as annoying by
popping up this dialog, and back to what I was doing before", and "what I was
doing before" means "looking at porn".

Comment 23

[Skewer]

Attachment: onBeforeUnload testcase (and sample application)

Comment 24

[Skewer]

I'm adding the URL of the MSDN spec for this (which is as authoritative as it
gets since this is a MS creation). Try to ignore their goofy syntax; a better
sample of the way to use this code is in attachment 116514 [details]. By the way, it is
not adequately stated in the MS reference, but a return value of bool true will
stifle the dialog. Bool false is displayed in the dialog as string "false". As
an implementor I would instead remove the author-specified text from the dialog
and just have the built-in message appear.

A few design notes that I would add:

1. This event should not fire in popup ads. We need to block that from happening
before it starts. In fact, I think onBeforeUnload and onUnload should both be
blockable in popup ads. However, be careful in noting that there may be useful
applications for both functions in a JavaScript window genuinely created by the
site's author. There are a number of ways to estimate whether a dialog is a
popup window or part of the site's content, and most of the rules that currently
apply to the way Mozilla blocks popups could apply the same to any unload rules.

2. The dialog presented by onBeforeUnload should have [JavaScript Application]
in the title just like all other JavaScript dialogs.

3. It would be very annoying to load a page with frames (including iframes and
similar devices) where several frames have this event. I think there should only
be one dialog in this case, but I am unsure what should be done if each
specifies different text for the dialog. One thing that should definitely be
done in the testing of any patches to this bug is a stress test of, say, 99
frames, each utilizing the onBeforeUnload event. Naturally, IE bugs you with a
dialog for every frame.

Being the in-control programmer I am, I would rather have an unload-type event
that lets me silently control whether or not to cancel unloading (for instance,
I could create a dialog that in one sweep says, Save, Don't Save, or Cancel, and
just return false to cancel the unload). Of course for security reasons we must
take a similar warning dialog approach to IE to prevent abuse
(onBeforeUnload="return false;" in a popup ad? Ugh!). But for more secure
applications such as app property dialogs, it may be appropriate to implement
this differently.

Also, I won't change anything just yet, but I think either critical severity or
RFE in the title should be made consistent with the other. I don't think this is
critical severity, but it does address a serious problem with web forms and
probably deserves higher priority than an enhancement. I'm thinking, -RFE
-critical +normal?

Comment 25

[Daniel Veditz [:dveditz]]

Resetting severity to "new feature" level, and removing incorrect dataloss
keyword. Web apps might lose data if they're relying on this non-standard and
non-universal event, but that's not a *Mozilla* dataloss bug.

Comment 26

[Johnny Stenback (:jst)]

Taking, and optimistically setting to Mozilla 1.4a.

Comment 27

[Mitchell Stoltz (not reading bugmail)]

For security/annoyance reasons, this feature should be controlled by a pref, and
should be disabled by default.

Comment 28

[Daniel Veditz [:dveditz]]

If it's disabled by default there's almost no point in implementing it. Most of
our annoyances are on by default because most people stick to reputable
non-annoying sites, and those who don't will eventually find the scripting pref
panel to stop all the other annoyances.

To some extent this conflicts with bug 48333 -- if that were implemented some
people would still want this one because it's more flexible, but no one is going
to want to see two dialogs. But if we fix this one in favor of 48333 then only
websites who bother with this non-standard event will be protected.

Comment 29

[Andrew Hagen]

This feature should be implemented and disabled by default. The feature is
needed for intranet and corporate applications, as mentioned by comment 11. It
is not needed for Internet applications. In fact, if it were enabled by default,
it would lead to pop-up misery. 

Bug 48333 should also be implemented and does not conflict with this bug. A
pref, perhaps the same one for onbeforeunload, should control whether it is
enabled. By default, the pref for 48333 should be enabled. 

This combination would give corprate managers maximum control over Mozilla as it
runs on their computers, and would give regular users maximum protection from
data loss of unsaved form data resulting from premature exits without creating
pop-up misery.

Comment 30

[Mitchell Stoltz (not reading bugmail)]

> Most of our annoyances are on by default because most people stick to reputable
> non-annoying sites, and those who don't will eventually find the scripting pref
> panel to stop all the other annoyances.

I disagree - the annoyances that are on by default are so because they have
become de facto standards, and to disable them would break a lot of pages. I
don't believe onBeforeUnload is widely used yet, and we should not encourage its
use.

Comment 31

[Skewer]

onBeforeUnload would be worthless and a waste of time if it is disabled by
default. The whole notion of a mechanism designed to *protect* data being
*disabled* by default is comical. People won't turn it on until *after* they've
had a devastating loss of data. If someone takes the time to implement this it
should most definitely be enabled by default.

Annoyance? It has the potential to be annoying, but you need to take a realistic
view on this:

1. Internet Explorer has supported onBeforeUnload since version 4. It has an
overwhelming majority of market share. Yet there has not been any significant
abuse of this scripting event. I challenge anyone to show me an existing,
normal-traffic site that abuses onBeforeUnload.
2. As I suggested, a system similar to the popup blocker would be effective in
blocking any abuse that ever does occur.
3. Whether or not the Mozilla project implements a certain functionality does
not significantly encourage web developers to add it to their pages. Developers
will do whatever Microsoft's latest machine supports. Mozilla did not make
favicon.ico popular.
4. Just because something is annoying and non-standard does not mean it doesn't
have a place in Mozilla. Marquee, anyone?

Comment 32

[Mitchell Stoltz (not reading bugmail)]

> 2. As I suggested, a system similar to the popup blocker would be effective in
> blocking any abuse that ever does occur.
If the blocker was checked in simultaneously with the OnBeforeUnload patch, this
might be OK.

> 4. Just because something is annoying and non-standard does not mean it doesn't
> have a place in Mozilla. Marquee, anyone?
No, but we have the flexibility to add or not add any nonstandard feature, and
for each one we must weigh its usefulness against its potential downsides. In
this case, I think the downside is significant. We are certainly under no
obligation to implement things just because IE does.

Any functionality which could be used to stop the user from leaving a page would
be obnoxious.

Comment 33

[Samir Gehani]

adt: nsbeta1-

Comment 34

[Janick Bernet]

I don't really get this. What's the big downside of implementing onBeforeUnload.
If the user doesn't want it (and this should be the default case) it could be
turned off (or on) in preferences. It's just that I don't really see a big
downisde in implementing and I don't think that implementation would be too
difficult.

Comment 35

[Martijn Korse]

"Any functionality which could be used to stop the user from leaving a page 
would be obnoxious."

That is soooo short-sighted. There are loads of features in Mozilla that could 
have an adverse effect on viewing webpages, but all can be switched on or off. 
The same thing could be done with this functionality! The first time this 
functionality will be triggered you could even display a window which explains 
where it can be turned on or off. So at max it will only be annoying once - and 
that you can hardly call a downside!

I really don't understand the resistance against this functionality....

Another option: release it as a plug-in. As pointed out this functionality can 
be especially usefull in browser-based intranet applications that deal with 
large forms.

Btw, It's been over 2,5 years since the original post. Is (the) Mozilla (team) 
still reading this topic? Or are we talking to ourselves?
I dare any developper to contact me on icq (#6863110) to debat the 
implementation of this feature, as i'm sure i can convince him/her in a 
dialogue.

Comment 36

[Johnny Stenback (:jst)]

For the record, I've been convinced that adding support for this into Mozilla is
the right thing to do, and now it's simply a matter of someone finding the time
to do it.

Comment 37

[RP]

The way IE handles onbeforeunload is very interesting and safe.
All it does is shows a dialog box with OK and Cancel along with a system
message 'augmented' by a custom programmable message.
Clicking OK keeps the user on the same page and does not unload the page.
Clicking CANCEL unloads the page and lets the user continue with his desirecd 
action.

So I do not see any malicious website being able to abuse it.
CANCEl button will always do what the user really wants.

This event will be a great feature to the web programming community.

Comment 38

[Jesse Ruderman]

RP: That is not a true statement about IE for Windows.  In IE for Windows,
clicking OK does what the user wants (leave the page) and clicking Cancel does
not.  And that is a security problem (see comment 22).

Comment 39

[William J. Edney]

To All -

I must (very strongly) add my voice to the growing list of those who support
this feature. The ability to 'give the user a second chance' to make sure that
they want to leave a particular page is critical. Because, in my world, 'leaving
a page' is equivalent to 'quitting your running Web application and flushing
whatever data was cached in the client JS'. My clients are using Mozilla as an
application delivery platform, not a porn browser (for that, IE works better ;-)).

Let me be the first, however, to offer a case of beer for the first person to
fix this and get r=,sr= and trunk checkin. I have done this before
(http://bugzilla.mozilla.org/show_bug.cgi?id=88049) and it worked out well :-).

Cheers,

- Bill

Comment 40

[Gabriel Wicke]

Another vote from me- i lost three hour's typing work in Epoz 0.74 embedded in
Plone2 by pressing <ALT>-A (which followed some access-keyed link in the Plone UI). 
Alt-A is the default 'select all' shortcut in Mozilla Composer on Linux, so this
case is not that unlikely.
The reason for pressing alt-a ironically was a security measure- i wanted
to copy the whole text in case something went wrong on save...

Comment 41

[Jason Sutter]

We're currently using onbeforeunload in Blogger, to ensure folks don't
accidently loose a post. It's great. Users are glad that it's there when they
need it and don't mind it when they didn't really want to save.

I'd love it if we could provide the same failsafe for Mozilla users - sooner
rather than later.

Comment 42

[Johnny Stenback (:jst)]

Attachment: Implement onbeforeunload.

This should do it, for seamonkey. Something similar to what's done in
XUL/XBL/JS here is also needed for firebird. More on that later.

Comment 43

[[e-mail]]

Comment on attachment 140707 [details] [diff] [review]
Implement onbeforeunload.

>+      <method name="onWindowClose">
>+        <parameter name="event"/>
>+        <body>
>+          <![CDATA[
>+            if (!event.windowClosePrevented && this.docShell.contentViewer &&
>+                !this.docShell.contentViewer.permitUnload()) {
>+                // Unloading was not permitted by the user, cancel the
>+                // window close.
>+              event.preventDefault();
>+              event.stopPropagation();
>+            }
>+          ]]>
>+        </body>
>+      </method>
Whether you want to check the current tab or all tabs, you would probably find
it easier to do that in the navigator's WindowIsClosing().

>+          // Add a close listener to let us prevent closing a window
>+          // if a tab isn't ready want to be closed.
>+          var me = this;
>+          function handler(event) {
>+            me.onWindowClose(event)
>+          }
>+          window.addEventListener("close", handler, true);
Alternatively if you rename onWindowClose to handleEvent and add
nsIDOMEventListener to the implementation list you can simply add the element
as the event listener.

Comment 44

[Johnny Stenback (:jst)]

> Whether you want to check the current tab or all tabs, you would probably find
> it easier to do that in the navigator's WindowIsClosing().

I don't want such code to be specific to navigator, anyone using tabs should
have the same unctionality. (and I know we don't share code between SeaMonkey
and other apps today, but that's a different issue which will hopefully be
solved some way, some day).

> >+          var me = this;
> >+          function handler(event) {
> >+            me.onWindowClose(event)
> >+          }
> >+          window.addEventListener("close", handler, true);
> Alternatively if you rename onWindowClose to handleEvent and add
> nsIDOMEventListener to the implementation list you can simply add the element
> as the event listener.

I kinda like this idea. I'll attempt do this...

Comment 45

[Peter Van der Beken [:peterv]]

Comment on attachment 140707 [details] [diff] [review]
Implement onbeforeunload.

Personally I'd just name the event nsBeforeUnloadEvent and NS_BEFORE_UNLOAD.

> Index: dom/src/events/nsJSEventListener.cpp
> ===================================================================

> -NS_IMETHODIMP 
> -nsJSEventListener::GetEventTarget(nsIScriptContext**aContext, 
> +NS_IMETHODIMP
> +nsJSEventListener::GetEventTarget(nsIScriptContext**aContext,

Want to add a space before aContext?

> Index: dom/src/jsurl/nsJSProtocolHandler.cpp
> ===================================================================

> @@ -531,34 +533,58 @@ nsJSChannel::InternalOpen(PRBool aIsAsyn

>          if (loadFlags & LOAD_DOCUMENT_URI) {
> -            // We're loaded as the document channel. Stop all pending
> -            // network loads.
> +            // We're loaded as the document channel. If we go on,
> +            // we'll blow away the currend document. Make sure that's

current

> Index: xpfe/global/resources/content/bindings/tabbrowser.xml
> ===================================================================

> @@ -1396,18 +1416,26 @@
>        <property name="userTypedValue"
>                  onget="return this.mCurrentBrowser.userTypedValue;"
>                  onset="return this.mCurrentBrowser.userTypedValue = val;"/>
>  
>        <constructor>
>          <![CDATA[
>            this.mCurrentBrowser = this.mPanelContainer.firstChild;
>            this.mCurrentTab = this.mTabContainer.firstChild;
>            this.mTabBox.handleCtrlTab = !/Mac/.test(navigator.platform);
> +
> +          // Add a close listener to let us prevent closing a window
> +          // if a tab isn't ready want to be closed.

s/want//

And I like Neil's suggestion.

Comment 46

[Peter Van der Beken [:peterv]]

You also wanted me to whine about event.returnValue which we don't support and
might make us incompatible with IE code (though it seems related to window.event).

Comment 47

[Peter Van der Beken [:peterv]]

Ah, and please change

+            if (!event.windowClosePrevented && this.docShell.contentViewer &&
+                !this.docShell.contentViewer.permitUnload()) {

to |if (!("windowClosePrevented" in event) ...|

Comment 48

[Brendan Eich [:brendan]]

What's that about window.event?!?

jst, how about a new patch to sr?

/be

Comment 49

[Johnny Stenback (:jst)]

Argh, my build is messed up, beyond recovery, and I'm in the middle of
implementing what Neil suggested. No new patch until tomorrow :-(

Brendan, you can still sr if you want, just don't look at the changes in JS,
XUL, or XBL files (which are very few).

Comment 50

[Peter Van der Beken [:peterv]]

Brendan: IE has a returnValue property on the event, so instead of returning a
string from the event handler one has to set window.event.returnValue to a
string. The example in
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeunload.asp
shows this. It might make sense to support it on the event object that we pass
to  handleEvent (so without implementing window.event ;-)), but I haven't really
thought it through.

Comment 51

[Johnny Stenback (:jst)]

(In reply to comment #50)
> Brendan: IE has a returnValue property on the event, so instead of returning a
> string from the event handler one has to set window.event.returnValue to a
> string. The example in

Correction, one does not have to set event.returnValue, one *can* set
event.returnValue if one doesn't want to return the string.

Comment 52

[Johnny Stenback (:jst)]

Attachment: Updated patch.

This should fix the issues that peterv pointed out, though some of those were
in code that's no longer around due to changes on the trunk.

This also changes how the whole tab thing works, it turned out that I don't
need an onclose handler in the tab code at all since I ended up following
Neil's other suggestion as well due to the fact that our tab code is rather
cumbersome.

Comment 53

[Brendan Eich [:brendan]]

Comment on attachment 141065 [details] [diff] [review]
Updated patch.

>@@ -2401,19 +2414,23 @@ nsHTMLDocument::Close()
> 
> nsresult
> nsHTMLDocument::WriteCommon(const nsAString& aText,
>                             PRBool aNewlineTerminate)
> {
>   nsresult rv = NS_OK;
> 
>   if (!mParser) {
>     rv = Open();
>-    if (NS_FAILED(rv)) {
>+
>+    // If Open() fails, or if it didn't create a parser (as it won't
>+    // if the user chose to not discard the current document through
>+    // onunload), don't write anything.

Comment should read "... user chose not to discard the current document through
on*before*unload....", right?

Here in nsGlobalWindow.cpp:

>   // Ask the content viewer whether the toplevel window can close.
>   // If the content viewer returns false, it is responsible for calling
>   // Close() as soon as it is possible for the window to close.
>   // This allows us to not close the window while printing is happening.
> 
>   nsCOMPtr<nsIContentViewer> cv;
>   mDocShell->GetContentViewer(getter_AddRefs(cv));
>   if (cv) {
>     PRBool canClose;
>-    cv->RequestWindowClose(&canClose);
>-    if (!canClose)
>+
>+    rv = cv->RequestWindowClose(&canClose);
>+    if (NS_SUCCEEDED(rv) && !canClose)
>+      return NS_OK;
>+
>+    rv = cv->PermitUnload(&canClose);
>+    if (NS_SUCCEEDED(rv) && !canClose)
>       return NS_OK;

Seems odd to do these in this order, instead of the reverse.  Necessary to
match IE or something?

>@@ -528,34 +530,58 @@ nsJSChannel::InternalOpen(PRBool aIsAsyn
[snip]
>+            nsCOMPtr<nsIDocShell> docShell(do_GetInterface(cb));
>+            if (docShell) {
>+                nsCOMPtr<nsIContentViewer> cv;
>+                docShell->GetContentViewer(getter_AddRefs(cv));
>+
>+                if (cv) {
>+                    PRBool okToUnload;
>+
>+                    if (NS_SUCCEEDED(cv->PermitUnload(&okToUnload)) &&
>+                        !okToUnload) {
>+                        // There's gotta be a better error...
>+                        rv = NS_ERROR_FAILURE;

Is this benign, or does it get suppressed later?

>+                    }
>+                }
>+            }
> 
>-            rv = StopAll();
>+            if (NS_SUCCEEDED(rv)) {
>+                rv = StopAll();
>+            }
>         }
> 
>         if (NS_SUCCEEDED(rv)) {
>             // This will add mStreamChannel to the load group.
> 
>             if (aIsAsync) {
>                 rv = mStreamChannel->AsyncOpen(aListener, aContext);
>             } else {
>                 rv = mStreamChannel->Open(aResult);
>             }
>         }
>-    } else {
>+    }
>+
>+    if (NS_FAILED(rv)) {
>         // Propagate the failure down to the underlying channel...
>         mStreamChannel->Cancel(rv);

Seems like it gets propagated, but I suppose it has to.  Maybe it should become
the magic DOM error for "javascript:void 0" and similar URLs, which do not turn
into new documents, therefore do not unload the current doc.

Looks great otherwise -- can you get ben to take a bug on Firefox?  sr=me with
good answers to the questions here.

/be

Comment 54

[Johnny Stenback (:jst)]

(In reply to comment #53)
> >+    // If Open() fails, or if it didn't create a parser (as it won't
> >+    // if the user chose to not discard the current document through
> >+    // onunload), don't write anything.
> 
> Comment should read "... user chose not to discard the current document through
> on*before*unload....", right?

Yeah, duh. Fixed.

> >+    rv = cv->RequestWindowClose(&canClose);
> >+    if (NS_SUCCEEDED(rv) && !canClose)
> >+      return NS_OK;
> >+
> >+    rv = cv->PermitUnload(&canClose);
> >+    if (NS_SUCCEEDED(rv) && !canClose)
> >       return NS_OK;
> 
> Seems odd to do these in this order, instead of the reverse.  Necessary to
> match IE or something?

Nope, this was an oversight on my part. I changed the order of those calls, I
like it better that way. Ideally the other call wouldn't even exist, but that's
another story alltogether...

...
> Seems like it gets propagated, but I suppose it has to.  Maybe it should become
> the magic DOM error for "javascript:void 0" and similar URLs, which do not turn
> into new documents, therefore do not unload the current doc.

Yeah, that's a good idea. I changed it to NS_ERROR_DOM_RETVAL_UNDEFINED.

> Looks great otherwise -- can you get ben to take a bug on Firefox?  sr=me with
> good answers to the questions here.

Thanks! I'll either fix this for Firefox myself, or get ben to do it.

Comment 55

[Johnny Stenback (:jst)]

Attachment: Same thing for Firefox.

This gives Firefox the same functionality in its chrome code, i.e. the ability
to fire the onbeforeunload event when closing windows and tabs, and at the same
time, it adds the warning about closing a window with multiple tabs open.

Comment 56

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 141265 [details] [diff] [review]
Same thing for Firefox.

Can we get a getter for tabCount or something like taht added to the tabbrowser
instead of using browser.mTabContainer.childNodes directly from browser.js?
With that, r/sr=[e-mail]

Comment 57

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 141265 [details] [diff] [review]
Same thing for Firefox.

Can we get a getter for tabCount or something like taht added to the tabbrowser
instead of using browser.mTabContainer.childNodes directly from browser.js?
With that, r/sr=[e-mail]

Comment 58

[Johnny Stenback (:jst)]

Attachment: No more mTabContainer!

Comment 59

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 141277 [details] [diff] [review]
No more mTabContainer!

r=[e-mail]

(marking on the new patch)

Comment 60

[Johnny Stenback (:jst)]

Last patch checked in.

Comment 61

[noririty]

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

Comment 62

[Steve Wardell]

Tested Firefox 20040220 build on XP and the closing multiple tabs notification
looks good, thou the unBeforeUnload testcase doesn't seem to work. Was that part
of Firefox or just Seamonkey?

Comment 63

[Jesse Ruderman]

Steve: works for me.  Note that you have to type text into the box in the
testcase for it to try to stop you from leaving the page.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040221
Firefox/0.8.0+

Comment 64

[Martin Honnen]

I have written two test cases which fire the onbodybeforeunload handler with IE,
one test case uses script to set document.body.onbeforeunload, the second test
uses script to set window.onbeforeunload. Both test cases don't fire the handler
with Mozilla 1.7a (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a)
Gecko/20040219) thus I am reopening the bug as I think if Mozilla tries to
implement an IE feature it should do so as far as possible and it seems odd that
you can currently set the onbeforeunload event handler with HTML but not with
script.
The test cases are at
http://home.arcor.de/martin.honnen/mozillaBugs/onbeforeunload/onbeforeunloadDocumentBody.html
http://home.arcor.de/martin.honnen/mozillaBugs/onbeforeunload/onbeforeunloadWindow.html

And the IE documentation at
http://msdn.microsoft.com/library/default.asp?url=/workshop/author/dhtml/reference/events/onbeforeunload.asp
clearly indicates that you should be able to set object.onbeforeunload for
window and for <body> (document.body).

Comment 65

[Vaclav Dvorak]

(In reply to comment #64)
> I have written two test cases which fire the onbodybeforeunload handler with IE,
> one test case uses script to set document.body.onbeforeunload, the second test
> uses script to set window.onbeforeunload. Both test cases don't fire the handler
> with Mozilla 1.7a (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a)

I may be completely wrong, but don't you need to call
"the_object.addEventListener('eventname', eventhandler, true)" to set an event
handler, instead of just doing "the_object.eventname = eventhandler"? See
http://www.w3.org/2003/01/dom2-javadoc/org/w3c/dom/events/EventTarget.html
I suspect that what you do is non-standard and Mozilla doesn't implement it.

Comment 66

[Martin Honnen]

(In reply to comment #65)

> don't you need to call
> "the_object.addEventListener('eventname', eventhandler, true)" to set an event
> handler, instead of just doing "the_object.eventname = eventhandler"? See
> http://www.w3.org/2003/01/dom2-javadoc/org/w3c/dom/events/EventTarget.html
> I suspect that what you do is non-standard and Mozilla doesn't implement it.

Well, onbeforeunload is non-standard, it is implemented in Mozilla as IE
implements it and with IE you can set object.onbeforeunload for window and for
document.body and therefore I think Mozilla should support that too. And Mozilla
for instance allows setting window.onload and window.onunload, it is not that
for other event types Mozilla doesn't support setting object.oneventname.

Comment 67

[Martin Honnen]

For completeness I have made a test case using addEventListener called on the
window object:
http://home.arcor.de/martin.honnen/mozillaBugs/onbeforeunload/onbeforeunloadWindowAddEventListener.html
There Mozilla fires the listener and shows the confirmation dialog when the link
is clicked. It still seems odd that setting window.onbeforeunload doesn't work.

Comment 68

[Martin Honnen]

(In reply to comment #67)
> For completeness I have made a test case using addEventListener called on the
> window object:
>
http://home.arcor.de/martin.honnen/mozillaBugs/onbeforeunload/onbeforeunloadWindowAddEventListener.html
> There Mozilla fires the listener and shows the confirmation dialog when the link
> is clicked. 

I had a JavaScript bug in that test case which caused an error in the JavaScript
console, with the strange effect that with that error the confirmation dialog
appeared while it does no longer appear now with the bug fixed. I have put in a
debug message which shows that the function added with window.addEventListener
is called when the link is clicked but somehow Mozilla fails to bring up the
confirmation dialog. IE does bring up the confirmation dialog.

To compare Mozilla's and IE's implementation I have also written the test case at
http://home.arcor.de/martin.honnen/mozillaBugs/onbeforeunload/onbeforeunloadDocumentBodyAttribute.html
which works with both IE 6 and Mozilla 1.7a, it shows that both Mozilla and IE
reflect the onbeforeunload attribute set on the <body> element as the
window.onbeforeunload property in their object models, and that is a further
reason why I think that setting window.onbeforeunload as done in
http://home.arcor.de/martin.honnen/mozillaBugs/onbeforeunload/onbeforeunloadWindow.html
should work with both IE and Mozilla.

Comment 69

[Johnny Stenback (:jst)]

Attachment: Fix for window.onbeforeunload etc not working.

Comment 70

[Johnny Stenback (:jst)]

Comment on attachment 142787 [details] [diff] [review]
Fix for window.onbeforeunload etc not working.

I know I had this change in my tree when I was working on the original patch,
but apparently this part fell off the patch wagon w/o me noticing...

Comment 71

[Brendan Eich [:brendan]]

Comment on attachment 142787 [details] [diff] [review]
Fix for window.onbeforeunload etc not working.

D'oh!

/be

Comment 72

[Johnny Stenback (:jst)]

Fixed.

Comment 73

[unknown_kev_cat]

(In reply to comment #2)
>Well, let's consider the situation that a user
>actually submits the form [...]
>In this case, it is VERY important
> that the page expires immediately. 

This is *NOT* a good idea! Many poorly designed sites do data validation after
submition, and then say 'press the back button'. In this case if it expires
imedeatly i lose everything.... I know sites should not do this and it does.
Often thepage managed to get the form data expired anyway, but forcing it to do
this would not be good.

Comment 74

[Bill Mason]

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

Comment 75

[Jim Cook]

I think the discussion points lead one to believe that Microsoft's 
implementation of onBeforeUnload was used as the basis for this functionality. 
However, the nightly build (June 1) of Firefox doesn't accurately represent 
that reality.

In fact, trying the example quoted in the comments () does not work.

Part of the problem is that Firefox seems to require the following syntax:

<BODY onBeforeUnload="return confirmexit();">

However, the following does NOT work:

<BODY onBeforeUnload="confirmexit();">

Microsoft uses the latter syntax, AND sets the returnValue on its event 
object. So is cross-browser support on this function shot to hell already?

Also, using Mozilla/Firefox how is onBeforeUnload supposed to work when using 
the attachEvent() functionality. What is returned and how?

body.attachEvent('onbeforeunload', confirmexit);

Comment 76

[William J. Edney]

Jim -

Sorry for any confusion this caused. As one of the main instigators of this
functionality (still owing jst a case of beer over it, in fact), let me try to
explain:

This functionality was never intended to be cross-browser in the sense of
'exactly syntactically the same'. It was intended to provide the *same
functionality*. It could never actually provide the exact same syntax and the
reason for this quite simple:

IE uses a non-standard syntax for defining event handlers and return values.
Mozilla uses the standardized W3C event syntax. What is evident in your code
sample is some of these Microsoftisms.

Setting the 'returnValue' of the event object is Microsoft only. In this case,
since Mozilla supports using the older 'DOM Level 0' standard (which is
supported by Microsoft as well) you return the value that you want the event
handler to return. Hence the need for 'return confirmexit()'.

In your other case, the 'attachEvent()' method is Microsoft only as well, added
in IE 5 mostly to support behaviors. The W3C standard syntax, supported by
Mozilla, is:

body.addEventListener('beforeunload',confirmexit,false);

(Note how you leave off the 'on' and that you pass false here to not have the
handler placed on the body element as a 'capturing' handler - although it
probably doesn't matter much in this case).

Feel free to email me at: [e-mail] if you need further
assistance here.

Cheers,

- Bill

Comment 77

[William J. Edney]

Folks (especially jst ;-) -

Been working through this with James Cook and it indeed appears that the
expected DOM Level 2 syntax of:

window.addEventListener('beforeunload',beforeunloadfunc,true);

doesn't work for this event. The test case in Martin Honnen's comment #68
applies here. Here's the user agent string for the Mozilla version I'm using to
test (Mozilla 1.7RC2):

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040514 

Is there a reason for this or is it the case of a dropped bit?

Can we reopen the bug until this gets sorted out?

Cheers,

- Bill

Comment 78

[Brendan Eich [:brendan]]

Why not file a new bug?

/be

Comment 79

[Jim Cook]

New bug added in regards to the incomplete implementation of this feature 
request.

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