This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 19713 - Processing of event handler return value is ambiguous
Summary: Processing of event handler return value is ambiguous
Status: RESOLVED FIXED
Alias: None
Product: WHATWG
Classification: Unclassified
Component: HTML (show other bugs)
Version: unspecified
Hardware: Other other
: P3 normal
Target Milestone: Unsorted
Assignee: Ian 'Hixie' Hickson
QA Contact: contributor
URL: http://www.whatwg.org/specs/web-apps/...
Whiteboard:
Keywords:
Depends on: 23489
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-26 05:46 UTC by contributor
Modified: 2013-10-14 14:41 UTC (History)
3 users (show)

See Also:


Attachments

Description contributor 2012-10-26 05:46:47 UTC
Specification: http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html
Multipage: http://www.whatwg.org/C#events
Complete: http://www.whatwg.org/c#events

Comment:
Processing of event handler return value is ambiguous

Posted from: 173.48.30.113 by bzbarsky@mit.edu
User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/19.0 Firefox/19.0
Comment 1 Boris Zbarsky 2012-10-26 05:55:21 UTC
For example, this part:

  If the event type is mouseover
    If return value is a boolean with the value true, then cancel the event.

This doesn't really define what "is a boolean" is.  So it's not clear what should happen if the return value is the string "something".

I would suggest relying on WebIDL here.  In particular, make the IDL look somewhat like this:

  [TreatNonCallableAsNull]
  callback EventHandlerNonNull = boolean (Event event);
  typedef EventHandlerNonNull? EventHandler;

  [TreatNonCallableAsNull]
  callback BeforeUnloadEventHandlerNonNull = DOMString? (Event event);
  typedef BeforeUnloadEventHandlerNonNull? BeforeUnloadEventHandler;

and define the onbeforeunload attribute as taking a BeforeUnloadEventHandler.  Then the processing model can say:

  If the event type is mouseover
    If return value is true, then cancel the event.
  If the Event object E is a BeforeUnloadEvent object
    If return value is not null and the Event object E's returnValue
    attribute's value is the empty string, then set the returnValue attribute's
    value to return value.
  Otherwise
    If return value is false, then cancel the event.

That should be completely unambiguous in weird cases like the onbeforeunload handler returning { toString: function() { return "hey, look at that" } }, which brings up a dialog in browsers but arguably shouldn't per current spec's "if the value is a string" bit.
Comment 2 Boris Zbarsky 2012-11-02 17:28:50 UTC
So the proposal in comment 1 doesn't work as-is because no return value would get treated as "false" for EventHandlerNonNull.

We could do "boolean?" as the return type, and say that a non-null false is needed.  That would still mean that returning "0" cancels the event; not sure whether that's desirable.
Comment 3 Boris Zbarsky 2012-11-02 17:33:41 UTC
But even if we decide that we can't have 0 preventing default, we should clearly define what "is a boolean" actually means here... in non-ECMAScript terms.  :(
Comment 4 Boris Zbarsky 2012-11-02 19:22:39 UTC
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=345521 sounds like we really don't want 0 to prevent default, unfortunately.
Comment 5 Ian 'Hixie' Hickson 2012-11-02 21:39:23 UTC
"Is a JavaScript boolean with the value false"?
Comment 6 Boris Zbarsky 2012-11-26 04:53:58 UTC
The return value is a WebIDL value...

It may be that the best we can do here is keep using IDL "any" and just make sure that this is "boolean" in the WebIDL sense, and rely on http://dev.w3.org/2006/webapi/WebIDL/#es-any to do the right thing for us.

I still think beforeunload should have DOMString? as return value, though.
Comment 7 Ian 'Hixie' Hickson 2013-02-06 23:38:10 UTC
bz: Using special WebIDL callbacks for beforeunload doesn't work because BeforeUnloadEvent objects can be fired for any event name, not just 'beforeunload'.

But I've tried to fix it... let me know (and reopen) if it's still problematic.
Comment 8 contributor 2013-02-06 23:38:21 UTC
Checked in as WHATWG revision r7702.
Check-in comment: Be more explicit about what this means.
http://html5.org/tools/web-apps-tracker?from=7701&to=7702
Comment 9 Boris Zbarsky 2013-02-08 16:58:58 UTC
> because BeforeUnloadEvent objects can be fired for any event name, not just
> 'beforeunload'.

I don't follow this.  Can you explain?

As far as I can tell the spec as currently written doesn't match implementations very well (not that interop for beforeunload is all that great).
Comment 10 Ian 'Hixie' Hickson 2013-03-28 20:28:34 UTC
What do you think the spec _should_ say?

The options I see are:

1. only have this magic behaviour for the beforeunload events fired as part of the "prompt to unload a document" algorithm,

2. have this magic behaviour for all BeforeUnloadEvent events

3. have this magic behaviour for all BeforeUnloadEvent events named 'beforeunload'

Is there a fourth option I'm missing?

Option 1 seems bad because it means the event dispatch mechanism has a dependency on the otherwise unrelated unload logic.

Option 3, as far as I can tell, needs a few more cycles to do something less useful than option 2.

That leaves option 2, which is what the spec says.

I agree that interop here isn't so hot, which is why I haven't really attempted to precisely match any UA, but obviously if there's a compat need, or if there's more interop than I realise, or if there's a fourth better option, or something, then we should change this.
Comment 11 Boris Zbarsky 2013-03-29 00:09:38 UTC
> What do you think the spec _should_ say?

What Gecko implements at this point is as follows:

1)  onbeforeunload has the type BeforeUnloadEventHandler on window/body/frameset,
    defined like so:

   callback BeforeUnloadEventHandlerNonNull = DOMString? (Event event);
   typedef BeforeUnloadEventHandlerNonNull? BeforeUnloadEventHandler;

2)  When invoking a BeforeUnloadEventHandlerNonNull, if the return value is not
    null call preventDefault() on the event object.  Then if the event is a
    BeforeUnloadEvent and its returnValue is empty, set its returnValue
    to the return value of the handler.

In particular, if the handler returns the number 42, or { toString: function() { return "hey there"; } } , we will show a prompt whereas current spec will not, as far as I can tell.  Furthermore, returning non-null from the handler will call preventDefault on the event, whereas the spec doesn't do that.  Those seem to be the main web-page detectable differences between what the spec says and Gecko does.
Comment 12 Ian 'Hixie' Hickson 2013-05-04 00:31:07 UTC
So that would be:

4. have 'beforeunload' events' return values be coerced to DOMString or null (unlike other events which aren't coerced at all), cancel if it's null, and if it's also a BeforeUnloadEvent event, additionally set the event object's returnValue based on the returned string if it's not null.

Is that right?
Comment 13 Boris Zbarsky 2013-05-04 01:17:31 UTC
That's what Gecko implements right now, I believe, yes.
Comment 14 Ian 'Hixie' Hickson 2013-06-07 20:58:14 UTC
Thanks for your patience with me on this, as always.
Comment 15 contributor 2013-06-07 21:01:16 UTC
Checked in as WHATWG revision r7933.
Check-in comment: Bring onbeforeunload handling closer to compatibility with the Web
http://html5.org/tools/web-apps-tracker?from=7932&to=7933