|Summary:||unload event doesn't fire|
|Component:||DOM||Assignee:||Maciej Stachowiak <mjs>|
|Severity:||Major||CC:||ap, bugs-webkit, darin, emacemac7, eric, george.zhu, ian, jsp, kingsinsear6, laszlo.gombos, rwlbuis, slewis|
|OS:||OS X 10.3|
Description Brad 2005-06-09 21:13:14 PDT
Comment 1 Jesse Pelton 2005-06-29 06:09:27 PDT
Comment 2 Brad 2005-06-29 09:07:50 PDT
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
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 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.