Bug 3402

Summary: unload event doesn't fire
Product: WebKit Reporter: Brad <brkemper>
Component: DOMAssignee: Maciej Stachowiak <mjs>
Status: NEW ---    
Severity: Major CC: ap, bugs-webkit, darin, emacemac7, eric, george.zhu, ian, jsp, kingsinsear6, laszlo.gombos, rwlbuis, slewis
Priority: P2    
Version: 312.x   
Hardware: Mac   
OS: OS X 10.3   
Attachments:
Description Flags
Mondo test case
none
Bug3402 test page
none
Bug3402 test page (unzipped)
none
Initial patch
none
Patch rniwa: review-

Description Brad 2005-06-09 21:13:14 PDT
most people are familiar with attaching javascript  to the loading of a page by having an onLoad 
parameter in the body tag of a page. There is also a parameter called onUnload that is supposed to 
work the same way, except it is supposed to fire when the page is closed (such as when a window is 
closed). In Safari it doesn't.  Non of the JavaScript code that is in the onUnload parameter of a body tag 
will be executed when the window is closed. But it should.
Comment 1 Jesse Pelton 2005-06-29 06:09:27 PDT
Created attachment 2701 [details]
Mondo test case

Safari is not alone in handling unload events unevenly.  This test case uses
multiple script-based approaches to setting an unload event handler on window
and body objects.  I tested with IE 6, Firefox 1.0.4, Opera 8.0, Konqueror 3.4,
and Safari 2.0.  Only Opera supported all the tested methods, but it invokes
body unload handlers multiple times.  Unfortunately, there is no single method
that works for all browsers.  I think that the ideal would be for all browsers
to support all the tested methods so script authors don't have to work out
which method works where, so I'll send this test case to the developers of all
the
browsers I tested.

Safari is, from my point of view, the worst offender.  All the other tested
browsers support setting an unload event on the window object (as allowed in
JavaScript since at least version 1.3), but Safari does not.  While neither
HTML 4.01 nor DOM 2 specifies an unload event for the window object, I think it
should nonetheless be supported: Netscape's JavaScript specification is the de
facto standard for the window object (since it's outside the domain of the W3C
specs), and other browsers implement it.
Comment 2 Brad 2005-06-29 09:07:50 PDT
That is a very useful table in the attachment. In all the variants shown, the onunload handler is set 
within a JavaScript block. One other case however, is having the handler built into the body tag as a 
parameter, like this:

<body onUnload="someFunction()"> 

Theoretically, it would be handled the same as Variant 4 (body.onunload), but my experience shows 
that it does not fire at all in Safari when it is set that way and the window closes (at least in cases where 
the entire window is closed, not just the page).
Comment 3 Alexey Proskuryakov 2006-02-08 11:56:23 PST
The attached test case doesn't currently work because of bug 7143.
Comment 4 Alexey Proskuryakov 2006-04-05 09:41:02 PDT
FWIW, a current build of WebKit (http://nightly.webkit.org) has the same behavior as Firefox 1.5 - tests 1 and 5 (and only those) pass.
Comment 5 Jesse Pelton 2006-04-05 11:04:38 PDT
That's progress of a sort, and it's probably sufficient unless the goal is complete support for W3C standards.  At least there's now one variant (the first) that works for all the browsers indicated in the test case.  While it would be cool if Safari could support variants 2, 3 and 4, only Opera supports 3 and 4, even though those appear to be mandated by the DOM standard.  (It turns out that Opera does not support variant 2, contrary to what the test case says.  I was misled by the browser's caching strategy.  Reloading the page before running variant 2 does not result in the unload handler being called.)
Comment 6 Brad 2006-04-06 08:07:17 PDT
(In reply to comment #4)
> FWIW, a current build of WebKit (http://nightly.webkit.org) has the same
> behavior as Firefox 1.5 - tests 1 and 5 (and only those) pass.
> 

Actually, #5 would be closest to setting explicitly in the HTML attribute. I just tested to see if having it in the HTML works now, and in the latest version (Version 2.0.3 (417.9.2)) it does! That is indeed good news. I personally think that is the most important one (along with being able to to the analogous "setAttribute()" in pure JavaScript (variant 5). Along with variant 1, this is enough for me as a developer/designer, and I think covers compatibility with the vast number of sites that use an unLoad event.
Comment 7 Brad 2006-04-06 08:09:26 PDT
Oh, BTW, when I say "latest version (Version 2.0.3 (417.9.2)) ", I mean Safari, not WebKit. So this is already in production (yeah!).
Comment 8 George Zhu 2007-01-05 01:16:40 PST
Created attachment 12237 [details]
Bug3402 test page

Bug3402 test page
Comment 9 George Zhu 2007-01-05 01:19:51 PST
Comment on attachment 12237 [details]
Bug3402 test page

I test it with Safari 2.0.4(419.3), onunload event fail to activate while in Firefox and IE, it works well. Is it a regression?
Comment 10 Jesse Pelton 2007-01-05 06:27:37 PST
Created attachment 12240 [details]
Bug3402 test page (unzipped)

Unzipped the test case to simplify testing.
Comment 11 Jesse Pelton 2007-01-05 06:29:12 PST
George's test case works for me with the same version of Safari.
Comment 12 George Zhu 2007-01-28 23:16:54 PST
(In reply to comment #11)
> George's test case works for me with the same version of Safari.
> 

Hi, Jesse Pelton,
I finally find the difference, if I reload the document in Safari, yes, the event is activated. But if I click red close button on left top corner of the page, the event is not activate, which behaves differently from IE and Firefox...

So the problem can be describe as: onUnload event does not fire when user close the window.
Comment 13 Jesse Pelton 2007-01-29 05:12:11 PST
It might or might not be a regression.  My "mondo" test case, while it has its uses, ignores this case, and seems to have focused attention on the cases it does cover.
Comment 14 George Zhu 2007-01-29 21:54:02 PST
Ok, you are right, the problem mentioned here has already resolved, how to close it? 
On the other hand, I create a new bug 12469, you can find it here: http://bugs.webkit.org/show_bug.cgi?id=12469
Comment 15 Rob Buis 2008-05-08 12:58:47 PDT
Created attachment 21025 [details]
Initial patch

Just a first stab at this bug. In fact it only is meant for the third problem
in the Mondo testcase, using addEventListener to register an unload event listener and then testing that it is called. I used the Mondo testcase to verify that this now works using the patch, however I would like to get some early feedback to a) get to know whether the approach is sane and b) whether there is a need to support the 2nd and 4th variant (which are the same problem).
*If* the approach is sane there may be some other help elements that may use the same technique, ie. all that use setHTMLWindowEventListener should have this third problem.
Cheers,

Rob.
Comment 16 Darin Adler 2008-05-24 23:09:59 PDT
Comment on attachment 21025 [details]
Initial patch

The patch needs a test case for each of the events you're patching here; and we need to double check with other browsers behavior too.

Also, I don't think this is the correct approach. I don't think we want to add these listeners to both the body element and the window object. Instead we want to dispatch these events to both the window object and the body element.
Comment 17 Maciej Stachowiak 2008-05-24 23:50:03 PDT
If the event bubbles, then dispatching to the body will also dispatch to the window (effectively). I agree with darin that adding the event listeners twice does not seem right. The relevant events should either dispatch to the body and bubble, or dispatch twice (to the body, then the window).
Comment 18 Stephanie Lewis 2008-05-25 00:28:32 PDT
At the moment adding onunload events and onbeforeunload events as anything other than a windowEventListener means that the events will never be dispatched to the element.
Comment 19 Rob Buis 2011-10-11 05:41:06 PDT
Created attachment 110502 [details]
Patch
Comment 20 Rob Buis 2011-10-11 05:44:12 PDT
I was looking at this bug again and I don't think much is broken nowadays. The cases 2 and 4 of the mondo testcase actually work, provided a function object, not a string is handed over. Since the other implementations do not accept the string assignment either, but do accept the function object, I think it is correct to be strict here.
For case 3 I uploaded a new patch. If this seems like the right direction I can add some tests.
Cheers,

Rob.
Comment 21 Darin Adler 2011-10-11 09:10:22 PDT
Comment on attachment 110502 [details]
Patch

Is this change correct? Why no test case?
Comment 22 Ryosuke Niwa 2011-11-28 16:55:33 PST
Comment on attachment 110502 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110502&action=review

> Source/WebCore/ChangeLog:6
> +        Fix addEventListener("unload") for body elements by propegating to the window object.

I'm not sure if want to support document.body.addEventListener. It seems like it makes more sense to support document.body.setAttribute('onunload', ...).

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

We definitely need a test for this. r- because of this.