Fix event handler processing algorithm to match implementations #2398

domenic · 2017-02-24T20:42:22Z

Remove the special cancelation behavior for onmouseover. This was only
ever implemented by Gecko. Fixes Consider removing special cancellation behaviour for event handlers #423 in part.
Update the special cancelation and argument behavior for onerror to a
reasonable set of intersection semantics among current
implementations. This fixes Sort out the OnErrorEventHandler mess #2296, and fixes the rest of Consider removing special cancellation behaviour for event handlers #423. See
especially
Sort out the OnErrorEventHandler mess #2296 (comment).
Editorial: links directly to Web IDL's definition of "invoke" instead
of indirecting.
Editorial: clarifies that BeforeUnloadEvents always have type
"beforeunload", instead of making that part of the conditional.

Tests:

Follows whatwg/html#2398; see also whatwg/html#2296.

domenic · 2017-02-24T23:07:20Z

FYI for anyone watching, this PR morphed from something about only the mouseover behavior to cover both mouseover and error and a number of editorial fixes; separate PRs ended up just being a bunch of conflicts. I've edited the OP to reflect this change.

foolip

Same nit twice, and I think that a test like https://github.com/w3c/web-platform-tests/pull/5014/files#r104584793 would be able to tease out whether the type matters or not.

No need to wait for more review if I'm just confused/wrong.

foolip · 2017-03-07T02:30:50Z

source

+    handling</var> be false.</p>
+
+    <p class="note">In this case, <var>E</var>'s <code data-x="dom-Event-type">type</code> will
+    always be <code data-x="event-error">error</code>.</p>


I wasn't able to figure out why this holds true. Synthetic events can reach this code path, right?

A synthetic event with a different name cannot trigger onerror though.

I do distinctly remember asking myself the same question @foolip asked, and then only later rediscovering that the reason was because of what @annevk said. In fact I think I did that multiple times. Let me try to make it clearer in both notes.

foolip · 2017-03-07T02:33:01Z

source

-      <p class="note">The <span data-x="event handler IDL attributes">event handler IDL
-      attribute</span>'s type is <code>OnBeforeUnloadEventHandler</code>, and the <var>return
+      <p class="note">This can only occur when <var>E</var>'s <code
+      data-x="dom-Event-type">type</code> is <code data-x="event-beforeunload">beforeunload</code>


Here too I don't see why this holds with synthetic events.

It doesn't.

This should be checking the type of the callback, not the type of the event, if it wants the guarantees about return type.

domenic · 2017-03-07T17:39:32Z

I added a clarification commit and also updated the tests in response to review feedback. I'd appreciate a look at the clarification commit especially before merging.

foolip

Thanks, my confusion is now resolved. Seems obvious in hindsight, but I'm happy I've not been the only one to wonder.

Some minimal tweaking around "producing" perhaps, but no need to ask for another round of review if you're confident about it.

foolip · 2017-03-08T09:02:18Z

source

+
+    <p class="note">In this case, <var>E</var>'s <code data-x="dom-Event-type">type</code> will
+    always be <code data-x="event-error">error</code>, since the only <span data-x="event
+    handlers">event handler</span> that produces <code>ErrorEvent</code> instances is <code


What does it mean for an event handler to produce an ErrorEvent instance? This bit I failed to notice before is that we're inside an algorithm that only applies to event handlers, where the event type is part of the name.

This and the other note would make perfect sense to me if only that connection is called out.

Indeed. Event handlers consume, not produce event instances. The thing that produces them is event constructors, and they can construct any event interface with any event type.

foolip · 2017-03-08T09:06:23Z

source

+      data-x="dom-Event-type">type</code> is <code data-x="event-beforeunload">beforeunload</code>
+      and when the <span data-x="event handler IDL attributes">event handler IDL attribute</span>'s
+      type is <code>OnBeforeUnloadEventHandler</code>, since the only <span data-x="event
+      handlers">event handler</span> that produces <code>BeforeUnloadEvent</code> instances is <code


Here too, it seems like something else is producing the instance:
https://html.spec.whatwg.org/#prompt-to-unload-a-document

domenic · 2017-03-08T15:55:52Z

Do you have a better word for the connection? I went with "producing" because it's not well-defined. Maybe "the only event handler that is called with BeforeUnloadEvent instances is..."?

* Remove the special cancelation behavior for onmouseover. This was only ever implemented by Gecko. Fixes #423 in part. * Update the special cancelation and argument behavior for onerror to a reasonable set of intersection semantics among current implementations. This fixes #2296, and fixes the rest of #423. See especially #2296 (comment). * Editorial: links directly to Web IDL's definition of "invoke" instead of indirecting. * Editorial: clarifies that BeforeUnloadEvents always have type "beforeunload", instead of making that part of the conditional.

Follows whatwg/html#2398.

bzbarsky · 2017-03-09T19:16:15Z

source

+    handling</var> be false.</p>
+
+    <p class="note">In this case, <var>E</var>'s <code data-x="dom-Event-type">type</code> will
+    always be <code data-x="event-error">error</code>, since the only <span data-x="event


This is not true. Simple testcase:

<script> window.onclick = function() { alert('hey'); } window.dispatchEvent(new ErrorEvent("click")); </script>

There seems to be some confusion here between event types (which are intimately related to which on* attribute we're dealing with here) and event interfaces, which are completely unrelated.

You really do want to check for both ErrorEvent and the event type being "error" and the current target being the right sort, however you do that.

The way Gecko does this is that the "onerror" property has the type "OnErrorEventHandler" only on Window and WorkerGlobalScope, and the equivalent of this algorithm checks the type of the callback we have. Explicitly saying to check the current target and having it be OnErrorEventHandler everywhere is probably black-box identical to that, though.... That's worth double-checking.

bzbarsky · 2017-03-09T19:18:49Z

source

+
+    <p class="note">In this case, <var>E</var>'s <code data-x="dom-Event-type">type</code> will
+    always be <code data-x="event-error">error</code>, since the only <span data-x="event
+    handlers">event handler</span> that produces <code>ErrorEvent</code> instances is <code


Indeed. Event handlers consume, not produce event instances. The thing that produces them is event constructors, and they can construct any event interface with any event type.

bzbarsky · 2017-03-09T19:28:12Z

source

-      <p class="note">The <span data-x="event handler IDL attributes">event handler IDL
-      attribute</span>'s type is <code>OnBeforeUnloadEventHandler</code>, and the <var>return
+      <p class="note">This can only occur when <var>E</var>'s <code
+      data-x="dom-Event-type">type</code> is <code data-x="event-beforeunload">beforeunload</code>


It doesn't.

This should be checking the type of the callback, not the type of the event, if it wants the guarantees about return type.

bzbarsky · 2017-03-09T19:34:10Z

source

-  the value is instead used to determine whether or not to prompt about unloading the document.</p>
+  <div class="note">
+   <p>The return value of the function affects whether the event is canceled or not: <span
+   w-nodev>as described above, </span>if the return value is false, the event is canceled.</p>


Span should end right after comma?

bzbarsky · 2017-03-09T19:36:09Z

Argh. I just missed you merging this. :(

Anyway, what got merged is totally nonsensical, unfortunately, @foolip was right. Please fix.

bzbarsky · 2017-03-09T20:35:19Z

@domenic see above.

domenic · 2017-03-09T20:37:47Z

Thank you for your vigilance @bzbarsky. I'm really embarassed I missed this because we actually do have tests for the new Event("error", ...) case in web-platform-tests/wpt#5014.

bzbarsky · 2017-03-09T20:40:08Z

Right, the tests all make sense. It's the spec that doesn't. ;)

domenic · 2017-03-09T20:42:32Z

I guess we don't have tests for the new ErrorEvent("click", ...) case. I'll be sure to do those as a follow-up.

domenic · 2017-03-09T20:49:41Z

For the beforeunload case though, I can't find a counterexample, because it's impossible to create BeforeUnloadEvent instances. The UA is the only one that does so, and the only event handler that will see them is onbeforeunload.

Follows whatwg/html#2398; see also whatwg/html#2296.

This is a follow-up to 7163372, which forgot about the case of new ErrorEvent("click", ...), as noted in #2398 (comment).

bzbarsky · 2017-03-09T20:55:03Z

Oh, BeforeUnloadEvent has no constructor? I wonder why not. Do we want to depend on it not growing one?

domenic · 2017-03-09T20:57:19Z

Probably because it's kind of an evil event? Meh, not a great reason.

I'll update #2428 to check both but note that the check is technically redundant.

domenic · 2017-03-09T20:59:25Z

Oh, wait, lol, createEvent("beforeunload") will work, I think... more tests to write.

This is a follow-up to 7163372, which forgot about the case of ErrorEvents with a non-"error" event type, or BeforeUnloadEvents with a non-"beforeunload" event type, as noted in #2398 (comment).

domenic · 2017-03-15T20:44:39Z

Here are all the tests relevant to the event handler processing algorithm:

Implementation status:

Firefox nightly confirmed passes everything after https://bugzilla.mozilla.org/show_bug.cgi?id=1345996
Chrome:
- Does not do error correctly: https://bugs.chromium.org/p/chromium/issues/detail?id=685276
- Does not do beforeunload correctly: https://bugs.chromium.org/p/chromium/issues/detail?id=692203
Edge:
- Does not pass 5th arg to onerror: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7203307/
- Does not treat synthetic ErrorEvents correctly: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11286304/
- Does not do beforeunload correctly: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11286419/
Safari: tiiiny bug https://bugs.webkit.org/show_bug.cgi?id=169789

foolip · 2017-03-21T10:38:14Z

Is this ready for review again? The use of "producing" is gone, so that issue's gone.

domenic · 2017-03-21T10:49:04Z

It got merged, then fixed up in #2428 :)

foolip · 2017-03-21T13:13:16Z

Oops, thanks :)

This is a follow-up to 7163372, which forgot about the case of ErrorEvents with a non-"error" event type, or BeforeUnloadEvents with a non-"beforeunload" event type, as noted in whatwg#2398 (comment).

domenic added normative change topic: events labels Feb 24, 2017

domenic added a commit to web-platform-tests/wpt that referenced this pull request Feb 24, 2017

Test for no special behavior for mouseover cancelation

074fb8f

Follows whatwg/html#2398.

domenic mentioned this pull request Feb 24, 2017

Test for no special behavior for mouseover cancelation web-platform-tests/wpt#5011

Merged

domenic force-pushed the rm-special-mouseover branch from 6d53911 to 8555352 Compare February 24, 2017 23:02

domenic changed the title ~~Remove special cancelation behavior for mouseover~~ Fix event handler processing algorithm to match implementations Feb 24, 2017

domenic added a commit to web-platform-tests/wpt that referenced this pull request Feb 24, 2017

Add various tests for error event specialness

346f244

Follows whatwg/html#2398; see also whatwg/html#2296.

domenic mentioned this pull request Feb 24, 2017

Add various tests for error event specialness web-platform-tests/wpt#5014

Merged

foolip approved these changes Mar 7, 2017

View reviewed changes

foolip approved these changes Mar 8, 2017

View reviewed changes

domenic added 3 commits March 9, 2017 14:24

Clarify why these types always match

ac26ec0

"produces" -> "is called with"

ce23bf2

domenic force-pushed the rm-special-mouseover branch from be3da0f to ce23bf2 Compare March 9, 2017 19:24

domenic merged commit 7163372 into master Mar 9, 2017

domenic deleted the rm-special-mouseover branch March 9, 2017 19:30

domenic added a commit to web-platform-tests/wpt that referenced this pull request Mar 9, 2017

Test for no special behavior for mouseover cancelation

9665faf

Follows whatwg/html#2398.

bzbarsky reviewed Mar 9, 2017

View reviewed changes

domenic added a commit to web-platform-tests/wpt that referenced this pull request Mar 9, 2017

Add various tests for error event specialness

f80f772

Follows whatwg/html#2398; see also whatwg/html#2296.

domenic added a commit that referenced this pull request Mar 9, 2017

Fix "special error event handling" for event handlers

06a40b6

This is a follow-up to 7163372, which forgot about the case of new ErrorEvent("click", ...), as noted in #2398 (comment).

domenic mentioned this pull request Mar 9, 2017

Fix event handler processing algorithm special cases #2428

Merged

pshaughn mentioned this pull request Dec 6, 2019

Remove obsolete special-case handling of Boolean return value from mouseover event servo/servo#25190

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix event handler processing algorithm to match implementations #2398

Fix event handler processing algorithm to match implementations #2398

domenic commented Feb 24, 2017 •

edited

Loading

domenic commented Feb 24, 2017

foolip left a comment

foolip Mar 7, 2017

annevk Mar 7, 2017

domenic Mar 7, 2017

foolip Mar 7, 2017

bzbarsky Mar 9, 2017

domenic commented Mar 7, 2017

foolip left a comment

foolip Mar 8, 2017

bzbarsky Mar 9, 2017

foolip Mar 8, 2017

domenic commented Mar 8, 2017

bzbarsky Mar 9, 2017

bzbarsky Mar 9, 2017

bzbarsky Mar 9, 2017

bzbarsky Mar 9, 2017

bzbarsky commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

domenic commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

domenic commented Mar 9, 2017

domenic commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

domenic commented Mar 9, 2017

domenic commented Mar 9, 2017

domenic commented Mar 15, 2017 •

edited

Loading

foolip commented Mar 21, 2017

domenic commented Mar 21, 2017

foolip commented Mar 21, 2017

Fix event handler processing algorithm to match implementations #2398

Fix event handler processing algorithm to match implementations #2398

Conversation

domenic commented Feb 24, 2017 • edited Loading

domenic commented Feb 24, 2017

foolip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Mar 7, 2017

foolip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

domenic commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

domenic commented Mar 9, 2017

domenic commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

domenic commented Mar 9, 2017

domenic commented Mar 9, 2017

domenic commented Mar 15, 2017 • edited Loading

foolip commented Mar 21, 2017

domenic commented Mar 21, 2017

foolip commented Mar 21, 2017

domenic commented Feb 24, 2017 •

edited

Loading

domenic commented Mar 15, 2017 •

edited

Loading