Bug 169789 – Returning false in event handler must not cancel CustomEvent of type beforeunload

• Component: DOM
• Status: RESOLVED DUPLICATE

[Domenic Denicola]

The newly-introduced test at http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html named

> Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable

does not pass in Safari Tech Preview 25. Everything else does (except the test "Returning a string must not cancel the event: BeforeUnloadEvent with type "click", cancelable" which is fine since it's best if Safari doesn't implement createEvent("beforeunload"); see https://github.com/whatwg/dom/issues/362.)

[Chris Dumez]

(In reply to comment #0)
> The newly-introduced test at
> http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/
> beforeunload-canceling.html named
> 
> > Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable
> 
> does not pass in Safari Tech Preview 25. Everything else does (except the
> test "Returning a string must not cancel the event: BeforeUnloadEvent with
> type "click", cancelable" which is fine since it's best if Safari doesn't
> implement createEvent("beforeunload"); see
> https://github.com/whatwg/dom/issues/362.)

I am not clear yet on what part of the spec says so.

I found:
https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (step 5)

which says:
If return value is not null, then:
    1. Set E's canceled flag.
    ...

So if you return false, we cancel the event.

[Chris Dumez]

(In reply to comment #1)
> (In reply to comment #0)
> > The newly-introduced test at
> > http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/
> > beforeunload-canceling.html named
> > 
> > > Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable
> > 
> > does not pass in Safari Tech Preview 25. Everything else does (except the
> > test "Returning a string must not cancel the event: BeforeUnloadEvent with
> > type "click", cancelable" which is fine since it's best if Safari doesn't
> > implement createEvent("beforeunload"); see
> > https://github.com/whatwg/dom/issues/362.)
> 
> I am not clear yet on what part of the spec says so.
> 
> I found:
> https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (step 5)
> 
> which says:
> If return value is not null, then:
>     1. Set E's canceled flag.
>     ...
> 
> So if you return false, we cancel the event.

Oh, it is a CustomEvent, not a BeforeUnloadEvent. I get it now.

[Chris Dumez]

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > The newly-introduced test at
> > > http://w3c-test.org/html/browsers/browsing-the-web/unloading-documents/
> > > beforeunload-canceling.html named
> > > 
> > > > Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable
> > > 
> > > does not pass in Safari Tech Preview 25. Everything else does (except the
> > > test "Returning a string must not cancel the event: BeforeUnloadEvent with
> > > type "click", cancelable" which is fine since it's best if Safari doesn't
> > > implement createEvent("beforeunload"); see
> > > https://github.com/whatwg/dom/issues/362.)
> > 
> > I am not clear yet on what part of the spec says so.
> > 
> > I found:
> > https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (step 5)
> > 
> > which says:
> > If return value is not null, then:
> >     1. Set E's canceled flag.
> >     ...
> > 
> > So if you return false, we cancel the event.
> 
> Oh, it is a CustomEvent, not a BeforeUnloadEvent. I get it now.

So:

"""
Otherwise
If return value is false, then set E's canceled flag.

If we've gotten to this "Otherwise" clause because E's type is beforeunload but E is not a BeforeUnloadEvent object, then return value will never be false, since in such cases return value will have been coerced into either null or a DOMString.
"""

From https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (step 5)

[Chris Dumez]

Created attachment 304716 [details]
WIP Patch

[Chris Dumez]

Created attachment 304755 [details]
Patch

[Chris Dumez]

Created attachment 304953 [details]
Patch

[Chris Dumez]

Tried to clarify the code a bit based on Darin's feedback offline.

[Geoffrey Garen]

Comment on attachment 304953 [details]
Patch

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

> Source/WebCore/bindings/js/JSEventListener.cpp:184
> +            // OnBeforeUnloadEventHandler returns a DOMString. Therefore, the value returned by JS should get serialized to a
> +            // String and cannot be False below.
> +            // See https://html.spec.whatwg.org/#onbeforeunloadeventhandler.
> +            bool isOnBeforeUnloadEventHandler = m_isAttribute && event->type() == eventNames().beforeunloadEvent;
> +            if (isOnBeforeUnloadEventHandler)
> +                return;

Why do we use event->type() == eventNames().beforeunloadEvent here but is<BeforeUnloadEvent>(*event) above?

[Darin Adler]

Comment on attachment 304953 [details]
Patch

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

>> Source/WebCore/bindings/js/JSEventListener.cpp:184
>> +                return;
> 
> Why do we use event->type() == eventNames().beforeunloadEvent here but is<BeforeUnloadEvent>(*event) above?

To state the obvious, we have to check is<BeforeUnloadEvent> above, because it guards a cast and the code uses data that is not stored in an event that just happens to be named "beforeunload" but was not generated by the DOM with the appropriate underlying class.

So whatever we decide, we must not change that to just checking the event type.

[Darin Adler]

Comment on attachment 304953 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSEventListener.cpp:184
>>> +                return;
>> 
>> Why do we use event->type() == eventNames().beforeunloadEvent here but is<BeforeUnloadEvent>(*event) above?
> 
> To state the obvious, we have to check is<BeforeUnloadEvent> above, because it guards a cast and the code uses data that is not stored in an event that just happens to be named "beforeunload" but was not generated by the DOM with the appropriate underlying class.
> 
> So whatever we decide, we must not change that to just checking the event type.

OK, I remember now, this is exactly what the change is all about! You can have a custom event of type "beforeunload" and it has to work. I believe this is covered by the test cases.

[Lucas Forschler]

Mass move bugs into the DOM component.

[Anne van Kesteren]

*** This bug has been marked as a duplicate of bug 168382 ***