Closed
Bug 245809
Opened 21 years ago
Closed 20 years ago
onBeforeUnload support not working with addEventListener
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mozilla, Assigned: jst)
References
Details
Attachments
(3 files)
|
595 bytes,
text/html
|
Details | |
|
648 bytes,
text/html
|
Details | |
|
20.34 KB,
patch
|
bzbarsky
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; NetCaptor 7.5.0 Gold; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040601 Firefox/0.8.0+
The recently implemented feature request (<a
href="http://bugzilla.mozilla.org/show_bug.cgi?id=68215">68215</a>) to support
an onbeforeunload event only seems to work in an embedded HTML attribute event
scenario. The implementation does not support a window.addEventListener() type
syntax shown in the example below.
Test
====
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0
Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html>
<head>
<script type="text/javascript">
//<![CDATA[
function unloading(e) {
return 'If you see this dialog, things must be looking
up!';
}
window.addEventListener('beforeunload', unloading, false);
//]]>
</script>
</head>
<body>
Just hit reload. If onBeforeUnload is working properly, you will see an alert
box saying so.
</body>
</html>
Reproducible: Always
Steps to Reproduce:
|
||
Comment 1•21 years ago
|
||
|
||
Comment 2•21 years ago
|
||
WFM: Firefox, branch build 20040607, Windows XP. I get an alert on the testcase.
Keywords: testcase
|
|
||
Comment 3•21 years ago
|
||
Jim, can you try the attached testcase?
The attached testcase WORKSFORME
when tried with Mozilla 1.8a2 build 2004060709 and with Firefox 0.8.0+,
rv:1.8a2, build 20040604; under XP Pro SP1a.
Keywords: testcase
|
||
Comment 4•21 years ago
|
||
Marking invalid, since the original testcase should in fact just unload silently
and the fixed testcase works as it should.
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → INVALID
|
||
Comment 5•21 years ago
|
||
If you check the MSDN document about onbeforeunload, you will notice IE does not
support window.attachEvent('onbeforeunload', fct). So maybe the real question
is: should Mozilla support addEventListener("beforeunload", ...) at all? Since
there is no way for an event listener to return a string it just doesn't make
much sense. Where in the updated testcase can I prevent the page from being
unloaded?
|
|
||
Comment 6•21 years ago
|
||
Apologies, I just did it a test with IE6 and it does support
window.attachEvent('onbeforeunload', fct). Sorry for the spam.
|
|
||
Comment 7•21 years ago
|
||
OK, to make up for it I've now found a real bug: no matter how many
onbeforeunload handler a page adds, IE6 executes only the first one. Mozilla,
OTOH, executes all. Users probably would not appreciate it if they had to
confirm several that they really want to leave the page...
|
|
||
Comment 8•21 years ago
|
||
*** Bug 251680 has been marked as a duplicate of this bug. ***
|
||
Comment 9•20 years ago
|
||
(In reply to comment #4)
> Marking invalid, since the original testcase should in fact just unload silently
> and the fixed testcase works as it should.
I don't believe this is invalid. Returning a string should result in a
confirmation prompt, NOT an alert box. Please look at Bug 251680 again, which
you labelled as a duplicate, for details.
However, I can see where the confusion is coming from since the description is
wrong: "If onBeforeUnload is working properly, you will see an alert box saying
so" is not correct, since it's not an alert box.
|
Assignee | |
Comment 10•20 years ago
|
||
Yeah, this is indeed not invalid. We do fire the event, but there's currently no
way for the event handler to pass text to the browser to include in the
beforeunload UI. IE does this by letting handlers set event.returnValue (which
is actually a more generic method in IE to return values from event handlers,
documented to be a boolean, but set to a string in their beforeunload event
samples, go figure). We can easily implement that too as the method to propagate
the string to include in the beforeunload UI from a beforeunload handler.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
|
|
Assignee | |
Comment 11•20 years ago
|
||
Attachment #176962 -
Flags: superreview?(peterv)
Attachment #176962 -
Flags: review?(bzbarsky)
|
|
||
Comment 12•20 years ago
|
||
Comment on attachment 176962 [details] [diff] [review]
Implement event.returnValue for events sent to beforeunload handlers.
All the licenses you added claim Netscape Communications Corporation as the
initial developer. That's pretty wrong in this case.... Use clean licenses
for new source files, please?
>Index: dom/public/nsIDOMClassInfo.h
> eDOMClassInfo_PopupBlockedEvent_id,
>+ eDOMClassInfo_BeforeUnloadEvent_id,
I thought we were adding to the end of the list now? We have comments to that
effect, certainly....
>Index: dom/public/idl/events/nsIDOMBeforeUnloadEvent.idl
>+ * handler
>+ */
>+ attribute DOMString returnValue;
Is that a tab? ;)
r=bzbarsky with that fixed, assuming the desired behavior is that a prompt come
up any time a string is returned from the method or set in event.returnValue,
right?
Attachment #176962 -
Flags: review?(bzbarsky) → review+
|
|
Assignee | |
Comment 13•20 years ago
|
||
(In reply to comment #12)
> (From update of attachment 176962 [details] [diff] [review] [edit])
> All the licenses you added claim Netscape Communications Corporation as the
> initial developer. That's pretty wrong in this case.... Use clean licenses
> for new source files, please?
Yeah, duh, fixed.
> >Index: dom/public/nsIDOMClassInfo.h
> > eDOMClassInfo_PopupBlockedEvent_id,
> >+ eDOMClassInfo_BeforeUnloadEvent_id,
>
> I thought we were adding to the end of the list now? We have comments to that
> effect, certainly....
Yeah, we were hoping to do that, but we haven't been very strict about it. It's
more whishful thinking than anything else that this would be binary compatible
from version to version... I moved this new id to the end tho, as the comments
suggest.
> >Index: dom/public/idl/events/nsIDOMBeforeUnloadEvent.idl
> >+ * handler
> >+ */
> >+ attribute DOMString returnValue;
>
> Is that a tab? ;)
It's not, in fact :) If you look at the indentation style used in the other idl
files in the directory you 'll see that "attribute" is always indented so that
you can write "readonly " in front of it so that the "attribute" and types line
up for readonly and writable attributes. Yeah, it looks weird in this file, but
oh well.
> r=bzbarsky with that fixed, assuming the desired behavior is that a prompt come
> up any time a string is returned from the method or set in event.returnValue,
> right?
Yeah, that's what we want, and that's what IE does.
Assignee: events → jst
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
||
Comment 14•20 years ago
|
||
Comment on attachment 176962 [details] [diff] [review]
Implement event.returnValue for events sent to beforeunload handlers.
What bz said.
>Index: dom/public/idl/events/nsIDOMBeforeUnloadEvent.idl
>===================================================================
>+interface nsIDOMBeforeUnloadEvent : nsIDOMEvent
>+{
>+ /*
Make that /**
>+ * Attribute used to pass back a return value from a beforeunload
>+ * handler
>+ */
>+ attribute DOMString returnValue;
>+};
>Index: dom/src/events/nsJSEventListener.cpp
>===================================================================
>+ // Set the text in the beforeUnload event as long as it wasn't
>+ // already set (through event.returnValue, which takes
>+ // presedence over a value returned from a JS function in IE)
precedence
>Index: content/events/src/nsDOMBeforeUnloadEvent.cpp
>===================================================================
>+nsDOMBeforeUnloadEvent::nsDOMBeforeUnloadEvent(nsPresContext* aPresContext,
>+ nsBeforePageUnloadEvent* aEvent)
>+ : nsDOMEvent(aPresContext, aEvent ? aEvent : new nsBeforePageUnloadEvent(NS_BEFORE_PAGE_UNLOAD))
Hmm, this won't catch OOM :-/.
Attachment #176962 -
Flags: superreview?(peterv) → superreview+
|
|
Assignee | |
Comment 15•20 years ago
|
||
(In reply to comment #14)
> Hmm, this won't catch OOM :-/.
Yeah, that's a problem with how our event construction code is set up. Nothing
I'm willing to do about that in this bug.
FIXED.
Status: NEW → RESOLVED
Closed: 21 years ago → 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•