Rewrite event handlers #3850

TimothyGu · 2018-07-24T21:42:03Z

/infrastructure.html ( diff )
/obsolete.html ( diff )
/sections.html ( diff )
/webappapis.html ( diff )

TimothyGu · 2018-07-26T06:54:30Z

This is now ready to be reviewed.

annevk

This looks really great. I hope to have time next week for a more detailed review. What would help with that is a small write-up (that we can eventually use in the commit message) of what is being changed and why. As this seemingly changes a bit more than just the lack of removal of event listeners added through event handlers.

annevk · 2018-07-26T08:59:45Z

source

-  element, replace the generic <span>event handlers</span> with the same names normally supported by
-  <span>HTML elements</span>.</p>
+  <p>The <span>event handlers</span> of the <code>Window</code> object named by the
+  <span><code>Window</code>-reflecting body element event handler set</span>, exposed on the


body should be in <code> here, no?

It applies to frameset elements too, and I was trying to allude to the body element.

annevk · 2018-07-26T09:07:11Z

source

+   element, then return <var>eventTarget</var>.</p></li>
+
+   <li><p>If <var>name</var> is not the name of an attribute of the <code>WindowEventHandlers</code>
+   interface mixin, and if <span><code>Window</code>-reflecting body element event handler


s/, and if/ and the/

(you could keep the additional comma I suppose, but I usually try to omit it if there's only two items)

annevk · 2018-07-26T09:10:11Z

source

+   <var>eventTarget</var>.</p></li>
+
+   <li><p>If <var>eventTarget</var>'s <span>node document</span> is not an <span>active
+   document</span>, then return null.</p></li>


It's a little surprising this step comes this late. This is only applicable to body and frameset elements?

Yes that's correct. The key here is that we want to make sure the event handler name check is before "active document" check.

This is tested in https://github.com/web-platform-tests/wpt/blob/7cdedf945221ce000c977008769a09b034e87716/html/webappapis/scripting/events/event-handler-attributes-windowless-body.html.

TimothyGu · 2018-07-29T04:55:19Z

What would help with that is a small write-up (that we can eventually use in the commit message) of what is being changed and why.

Of course.

Move the "When an event handler H of an EventTarget object T is first set to a non-null value" steps into a dedicated activate an event handler algorithm. Create a corresponding deactivate an event handler algorithm, called in the circumstances outlined in Event handlers-created event listener need to be removed sometimes #3836, thereby fixing that issue.
Define "When an event handler content attribute is set" and "When an event handler content attribute is removed" steps rigorously using the attribute change steps hook in DOM. Care is taken to clarify certain edge cases, such as calling removeAttribute() on a nonexistent event handler content attribute but with a corresponding event handler IDL attribute defined, and calling setAttribute() an attribute with the same value as it currently possesses but with the event handler deactivated (exhibit B in Event handlers-created event listener need to be removed sometimes #3836).
Formalize multiple definitions:
- event handler as a struct containing a value and an event listener
- location of event handlers and the concept of event handler target
- rigorous set of steps on how to obtain the target of an event handler through determine the target of an event handler algorithm. The latter algorithm supersedes the handwavy and inconsistently applied "If an event handler IDL attribute exposes an event handler of an object that doesn't exist" steps
- storage of event handlers in the event handler map
More descriptive names for H (eventHandler), T (eventTarget</var), and E (event) in various algorithms

domenic

Pushed some minor tweaks, but LGTM. Would love @annevk's review too.

I made a half-hearted attempt at sectioning out this mega-section into subsections, but it didn't work. I'll file a follow-up issue explaining and brainstorming for the future.

domenic · 2018-07-30T18:17:41Z

Oh, shoot, I forgot. I don't think this works after all:

If eventTarget's node document is not an active document, then return null.

In particular this will not bail out for const eventTarget = document.createElement("body") inside an active document.

I think this might be correct:

If eventTarget is not the body element of its node document.

TimothyGu · 2018-07-31T03:48:03Z

@domenic Unfortunately, what you described doesn't seem to be what browsers implement:

const b = document.createElement("body");
const f = () => {};
b.onblur = f;
console.assert(b.onblur === f);
console.assert(window.onblur === f);

On Chrome, Safari, and Firefox this code executes fine.

I guess I should add another test case then.

domenic · 2018-07-31T03:49:46Z

That seems quite broken, but if everyone agrees, then it's probably not worth fixing. Amazing...

Edit: we may even want to add a spec note/example about this unusual fact.

See: whatwg/html#3850

…a=testonly Automatic update from web-platform-testsHTML event handlers: More uniform naming convention -- HTML event handlers: Use .window.js instead of .html for some files -- HTML event handlers: Use IDL parser for list of event handlers -- HTML event handlers: Use synchronous test() where able -- HTML event handlers: Additionally test unshadowed event handlers -- HTML: tests for new event handler spec See: whatwg/html#3850 -- HTML event handlers: Test document.createElement("body") -- wpt-commits: db24506f8af9048c0a80d89c9981ef445d6c3af9, 3e273bc58ae2af40f4b00b514f2fd6d604ffc98d, 8bf45a39c3435737ebc862904af245d27b68efce, 776041862ca0cab7a8dd10e29361dbf3359a0b2f, fa58a12fc565fc152bff196e803fd9290dfa1bc7, fc2eba9d6f0a860c0db1457852124e320fd8d499, 8e668a6b63e5dbb68304c182fdc1f7b4e6edaf96 wpt-pr: 12201

This change aligns the event listener removal behavior with implementations, specifically in two aspects: - The event handler's value should be set to null (i.e., deactivated). - Event listeners and handlers should be removed from the Window object as well. See prior investigation around deactivation of event handlers in whatwg#3836 and whatwg#3850. See investigation around `document.open()`'s behavior in whatwg#3818. Tests: web-platform-tests/wpt#12122

This change aligns the event listener removal behavior with implementations, specifically in two aspects: - The event handler's value should be set to null (i.e., deactivated). - Event listeners and handlers should be removed from the Window object as well. See prior investigation around deactivation of event handlers in #3836 and #3850. See investigation around document.open()'s behavior in #3818. Tests: web-platform-tests/wpt#12122

This change aligns the event listener removal behavior with implementations, specifically in two aspects: - The event handler's value should be set to null (i.e., deactivated). - Event listeners and handlers should be removed from the Window object as well. See prior investigation around deactivation of event handlers in whatwg#3836 and whatwg#3850. See investigation around document.open()'s behavior in whatwg#3818. Tests: web-platform-tests/wpt#12122

…a=testonly Automatic update from web-platform-testsHTML event handlers: More uniform naming convention -- HTML event handlers: Use .window.js instead of .html for some files -- HTML event handlers: Use IDL parser for list of event handlers -- HTML event handlers: Use synchronous test() where able -- HTML event handlers: Additionally test unshadowed event handlers -- HTML: tests for new event handler spec See: whatwg/html#3850 -- HTML event handlers: Test document.createElement("body") -- wpt-commits: db24506f8af9048c0a80d89c9981ef445d6c3af9, 3e273bc58ae2af40f4b00b514f2fd6d604ffc98d, 8bf45a39c3435737ebc862904af245d27b68efce, 776041862ca0cab7a8dd10e29361dbf3359a0b2f, fa58a12fc565fc152bff196e803fd9290dfa1bc7, fc2eba9d6f0a860c0db1457852124e320fd8d499, 8e668a6b63e5dbb68304c182fdc1f7b4e6edaf96 wpt-pr: 12201 UltraBlame original commit: 240261e9447c0e8ec0e0b25ec09ca905c70c317d

TimothyGu added 2 commits July 24, 2018 14:49

wip

aff2794

specify vs expose

65a805f

TimothyGu force-pushed the event-handler branch 2 times, most recently from 06c31f6 to 65a805f Compare July 24, 2018 21:58

TimothyGu added 3 commits July 24, 2018 15:10

s/three/two/

842cc49

Fix name definition

5fab899

Finished (?)

88fe641

TimothyGu force-pushed the event-handler branch from f12a5cd to 88fe641 Compare July 25, 2018 23:56

TimothyGu changed the title ~~[WIP] Rewrite event handlers~~ Rewrite event handlers Jul 26, 2018

annevk added needs tests Moving the issue forward requires someone to write tests topic: events labels Jul 26, 2018

annevk reviewed Jul 26, 2018

View reviewed changes

TimothyGu mentioned this pull request Jul 26, 2018

HTML: tests for new event handler spec web-platform-tests/wpt#12201

Merged

TimothyGu removed the needs tests Moving the issue forward requires someone to write tests label Jul 26, 2018

TimothyGu added 2 commits July 27, 2018 14:05

Review comments

44a61cc

Remove confusing specify vs. expose dichotomy

16a46db

TimothyGu and others added 2 commits July 28, 2018 22:01

Fix some variables

48ba981

Minor tweaks

3191647

domenic approved these changes Jul 30, 2018

View reviewed changes

domenic mentioned this pull request Jul 30, 2018

Split out subsections of "Event handlers" #3861

Open

TimothyGu added a commit to web-platform-tests/wpt that referenced this pull request Jul 31, 2018

HTML: tests for new event handler spec

74810f6

See: whatwg/html#3850

TimothyGu added a commit to web-platform-tests/wpt that referenced this pull request Jul 31, 2018

HTML: tests for new event handler spec

fc2eba9

See: whatwg/html#3850

Add note about document.createElement("body")

541e5db

domenic assigned annevk Aug 2, 2018

annevk approved these changes Aug 7, 2018

View reviewed changes

domenic merged commit 753e9fe into whatwg:master Aug 7, 2018

jdm mentioned this pull request Aug 7, 2018

Ensure that event handlers match updated web standard servo/servo#21354

Open

TimothyGu deleted the event-handler branch August 7, 2018 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rewrite event handlers #3850

Rewrite event handlers #3850

Uh oh!

TimothyGu commented Jul 24, 2018 •

edited by pr-preview bot

Loading

Uh oh!

TimothyGu commented Jul 26, 2018

Uh oh!

annevk left a comment

Uh oh!

annevk Jul 26, 2018

Uh oh!

TimothyGu Jul 26, 2018

Uh oh!

annevk Jul 26, 2018

Uh oh!

annevk Jul 26, 2018

Uh oh!

TimothyGu Jul 26, 2018 •

edited

Loading

Uh oh!

TimothyGu commented Jul 29, 2018

Uh oh!

domenic left a comment

Uh oh!

domenic commented Jul 30, 2018

Uh oh!

TimothyGu commented Jul 31, 2018

Uh oh!

domenic commented Jul 31, 2018 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Rewrite event handlers #3850

Rewrite event handlers #3850

Uh oh!

Conversation

TimothyGu commented Jul 24, 2018 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TimothyGu commented Jul 26, 2018

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

annevk Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

annevk Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

annevk Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

TimothyGu Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Jul 29, 2018

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

domenic commented Jul 30, 2018

Uh oh!

TimothyGu commented Jul 31, 2018

Uh oh!

domenic commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

TimothyGu commented Jul 24, 2018 •

edited by pr-preview bot

Loading

TimothyGu Jul 26, 2018 •

edited

Loading

domenic commented Jul 31, 2018 •

edited

Loading