New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onbeforeunload return value handling is broken #2297

Closed
bzbarsky opened this Issue Jan 25, 2017 · 9 comments

Comments

4 participants
@bzbarsky
Collaborator

bzbarsky commented Jan 25, 2017

https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm step 4 says:

If the event type is beforeunload
...
If the return value is null, then cancel the event.

which means that this testcase:

onbeforeunload = function() {};

should cancel the event, and hence lead to a beforeunload prompt. That's not how browsers actually behave.

The way Gecko behaves, based on code inspection, is that the "cancel the event" steps happen only if the return vaue is NOT null (and also if the event is an actual BeforeUnloadEvent, for what it's worth).

When this is fixed, please fix the html/webappapis/scripting/events/event-handler-processing-algorithm.html test in web platform tests, which was written based on the current spec text and fails in every browser I've tried it in.

I should note, that based on the results of that test and the behavior of browsers on this testcase:

onbeforeunload = function() {
  return "";
}

(which leads to a beforeunload prompt in Firefox, Chrome, and Safari) it sounds like the Gecko behavior of only doing special things here for actual BeforeUnloadEvent instances is not so Gecko-specific.

@anu0012

This comment has been minimized.

Show comment
Hide comment
@anu0012

anu0012 Jan 28, 2017

@bzbarsky Hey I'd like to contribute. Can you please guide me?

anu0012 commented Jan 28, 2017

@bzbarsky Hey I'd like to contribute. Can you please guide me?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jan 28, 2017

Collaborator

That should probably be a question for someone who actually edits the HTML spec, not me. I just report bugs in it...

@zcorpan you added the "good first bug" tag, so want to do the guiding?

Collaborator

bzbarsky commented Jan 28, 2017

That should probably be a question for someone who actually edits the HTML spec, not me. I just report bugs in it...

@zcorpan you added the "good first bug" tag, so want to do the guiding?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jan 29, 2017

Collaborator

And just to be clear, this could use actual useful tests, not just the little bit in html/webappapis/scripting/events/event-handler-processing-algorithm.html...

Collaborator

bzbarsky commented Jan 29, 2017

And just to be clear, this could use actual useful tests, not just the little bit in html/webappapis/scripting/events/event-handler-processing-algorithm.html...

@anu0012

This comment has been minimized.

Show comment
Hide comment
@anu0012

anu0012 Jan 29, 2017

@bzbarsky I need to change the return value of both functions and test it according to Web Platform Tests description. Am I right ?

anu0012 commented Jan 29, 2017

@bzbarsky I need to change the return value of both functions and test it according to Web Platform Tests description. Am I right ?

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jan 30, 2017

Collaborator

You need to test what browsers actually do, then change the spec to reflect it, then change the tests to test the spec...

Collaborator

bzbarsky commented Jan 30, 2017

You need to test what browsers actually do, then change the spec to reflect it, then change the tests to test the spec...

@zcorpan

This comment has been minimized.

Show comment
Hide comment
@zcorpan

zcorpan Jan 30, 2017

Member

My understanding from the OP was that the fix for the standard would be to change

If the return value is null, then cancel the event.

to

If the return value is not null, then cancel the event.

But it seems this needs more investigation, so I'll remove the label again. @anu0012 you're still welcome to work on this of course! You can ask in https://wiki.whatwg.org/wiki/IRC for help.

Member

zcorpan commented Jan 30, 2017

My understanding from the OP was that the fix for the standard would be to change

If the return value is null, then cancel the event.

to

If the return value is not null, then cancel the event.

But it seems this needs more investigation, so I'll remove the label again. @anu0012 you're still welcome to work on this of course! You can ask in https://wiki.whatwg.org/wiki/IRC for help.

@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Jan 30, 2017

Collaborator

To be clear, I believe that is in fact the correct fix for the standard, and I have done some basic testing in some browsers, but I don't 100% remember whether I tested an explicit null return (as opposed to undefined, say) in Edge and whatnot.

Collaborator

bzbarsky commented Jan 30, 2017

To be clear, I believe that is in fact the correct fix for the standard, and I have done some basic testing in some browsers, but I don't 100% remember whether I tested an explicit null return (as opposed to undefined, say) in Edge and whatnot.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Feb 10, 2017

Member

I found another interesting spec mismatch, which is that both Gecko and Blink do not allow CustomEvent("beforeunload") to go down this path. Per spec I believe it should. So I think we at least need another guard.

Member

domenic commented Feb 10, 2017

I found another interesting spec mismatch, which is that both Gecko and Blink do not allow CustomEvent("beforeunload") to go down this path. Per spec I believe it should. So I think we at least need another guard.

domenic added a commit that referenced this issue Feb 11, 2017

Fix onbeforeunload event handler processing
There are two particular fixes:

- null should *not* cancel the event; this was flipped
- CustomEvents with type "beforeunload" should not get this special
  behavior (especially since they could be non-cancelable)

This also modernizes the surrounding section, style-wise.

Fixes #2297.
@bzbarsky

This comment has been minimized.

Show comment
Hide comment
@bzbarsky

bzbarsky Feb 11, 2017

Collaborator

Right, see the "and also if the event is an actual BeforeUnloadEvent, for what it's worth" bit in the original issue description...

Collaborator

bzbarsky commented Feb 11, 2017

Right, see the "and also if the event is an actual BeforeUnloadEvent, for what it's worth" bit in the original issue description...

domenic added a commit that referenced this issue Feb 14, 2017

Fix onbeforeunload event handler processing
There are two particular fixes:

* null should *not* cancel the event. This has been flipped ever since
  it was introduced in 7612afe. It
  appears the mistake occurred between
  https://www.w3.org/Bugs/Public/show_bug.cgi?id=19713#c11 (includes
  "not") and https://www.w3.org/Bugs/Public/show_bug.cgi?id=19713#c12
  (which elides it).
* Only applies the special behavior to BeforeUnloadEvents, not to any
  Event with the name "beforeunload". This is especially important since
  such events could be non-cancelable, in which case the previous text
  could potentially cancel them anyway.

This also modernizes the surrounding section, style-wise.

Fixes #2297.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment