Revise event listener removal in document.open() #3893
Conversation
source
Outdated
| <li><p>If <var>node</var> has an associated <span>event handler map</span>, then for each | ||
| <var>name</var> → <var>eventHandler</var> of <var>node</var>'s associated <span>event handler | ||
| map</span>, <span>deactivate an event handler</span> given <var>node</var> and | ||
| <var>name</var>.</p></li> |
There was a problem hiding this comment.
So is this a change we want implementations to make? Because when I looked at implementations they were not doing this.
Should we always do this when we say "remove all event listeners" anywhere? Perhaps it needs to become part of the same primitive?
There was a problem hiding this comment.
From your test it does seem to match. I wonder if implementations are a little different from what we specified then. That if the corresponding event listener is removed it simply returns null?
There was a problem hiding this comment.
Because when I looked at implementations they were not doing this.
For Chrome and Safari at least, they are already doing this. Firefox is not removing event listeners to begin with.
Should we always do this when we say "remove all event listeners" anywhere? Perhaps it needs to become part of the same primitive?
I'd be +1 on that. Two things of note:
- we should probably need to move that concept from DOM to HTML if we were to do that, as the concept of event handlers is HTML-only
- right now "remove all event listeners" might only be used in
document.open()…
There was a problem hiding this comment.
I wonder if implementations are a little different from what we specified then. That if the corresponding event listener is removed it simply returns null?
What Blink (and probably WebKit) does is it embeds the event handler inside an event listener. When the event handler IDL attribute is gotten, Blink searches through the list of existing event listeners to find one backed by an event handler (maybe with some caching, maybe not). So yes, at least for Blink I believe what you described is true.
In the current version of the spec, I'm not doing this. Instead I have an event listener embedded inside an event handler. There are two considerations for this:
- The current approach in the spec avoids marking certain event listeners (a concept defined in DOM) as "special"
- The event-handler-backed event listener can never be removed by DOM APIs like
removeListener(), so it's unobservable if event listener is owned by the event handler or vice versa.
There was a problem hiding this comment.
I believe "remove all event listeners" was indeed only used for document.open(); we introduced it for that purpose in whatwg/dom@21e7ec3.
We could create a wrapper in the HTML standard, perhaps "remove all event listeners and handlers" in the Event handlers section of the spec.
|
I don't think the commit message adequately captures what is being changed here. It seems to me there's two important changes:
|
|
(To be clear, really appreciate that you are picking this up!) |
domenic
left a comment
domenic
left a comment
There was a problem hiding this comment.
Technical changes LGTM, modulo conversation about refactoring the event handler removal.
Agreed on expanding the commit message; we should also expand the reference to #3818 to contextualize this within the larger context of document.open() changes. (Especially if we're going to hold off on filing browser bugs until everything settles down, which may be a good idea.)
source
Outdated
| <li><p>If <var>document</var> is not an <span>active document</span>, then return | ||
| <var>document</var>.</p></li> | ||
|
|
||
| <li><p>Let <var>window</var> be <var>document</var>'s <var>relevant global object</var>.</p></li> |
There was a problem hiding this comment.
var -> span, probably needs a data-x too
source
Outdated
| <li><p>If <var>node</var> has an associated <span>event handler map</span>, then for each | ||
| <var>name</var> → <var>eventHandler</var> of <var>node</var>'s associated <span>event handler | ||
| map</span>, <span>deactivate an event handler</span> given <var>node</var> and | ||
| <var>name</var>.</p></li> |
There was a problem hiding this comment.
I believe "remove all event listeners" was indeed only used for document.open(); we introduced it for that purpose in whatwg/dom@21e7ec3.
We could create a wrapper in the HTML standard, perhaps "remove all event listeners and handlers" in the Event handlers section of the spec.
|
Contrary to what I said on IRC, I think having that refactoring would be good given that this algorithm itself has two callers already. (I might also slightly prefer the "special" event listener approach, but what we have now is good too.) |
TimothyGu
changed the title
source
Outdated
| </ol> | ||
|
|
||
| <p>To <dfn>erase all event listeners and handlers</dfn> given an <code>EventTarget</code> object | ||
| <var>eventTarget</var>, take the following steps:</p> |
source
Outdated
| handler map</span>, <span>deactivate an event handler</span> given <var>eventTarget</var> and | ||
| <var>name</var>.</p></li> | ||
|
|
||
| <li><p><span>Remove all event listeners</span> with <var>eventTarget</var>.</p></li> |
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
TimothyGu
force-pushed
the
document-open-1
branch
from
3b00228 to
7508403
Compare
3 participants
Tests: web-platform-tests/wpt#12122
See: #3818
See: #3836
/dynamic-markup-insertion.html ( diff )
/webappapis.html ( diff )