Editorial: add "add an event listener" hook #596
Conversation
This hook ensures that any special casing in addEventListener() is shared with event handler attributes. Helps with w3c/ServiceWorker#1004.
See whatwg/dom#596 and whatwg/dom#365 and w3c/ServiceWorker#1004 for more context.
|
Since event handlers don't have options, maybe this should accept a few booleans instead of a dictionary-or-boolean (with good defaults that make sense for event handlers)? |
|
I'd rather not make this an ever expanding set of arguments. I'd be okay with making the last argument optional to make the HTML caller slightly less ugly, but I'm not sure it's worth it. |
|
The issue is that for anyone that calls it except for the addEventListener() definition, they have to manually assemble an AddEventListenerOptions dict, which feels like it should be specific to the addEventListener API and to JS consumers of it. |
|
I guess you're considering a third caller we haven't decided on adding thus far. I guess if that might not use AddEventListenerOptions we should make that argument optional for now and not pass it from HTML. Then whenever this third caller comes around we can restructure this without having to change HTML. |
|
Yeah I'm kind of thinking of https://domenic.github.io/async-local-storage/#add-a-simple-event-listener |
|
Hmm, I suppose we could use that signature, requiring the caller to create an event listener concept wrapper. |
|
I pushed a more elaborate change here that also integrates with Infra better and basically does the above comment. The only thing I wonder about is whether service workers throw before returning if callback is null, but that was already a problem of sorts. |
| @@ -328,7 +328,7 @@ method, passing the same arguments. | |||
| {{Event}} interface (or a derived interface). In the example above | |||
| <var ignore>ev</var> is the <a>event</a>. It is | |||
| passed as argument to | |||
| <a>event listener</a>'s <b>callback</b> | |||
| <a>event listener</a>'s <a for="event listener">callback</a> | |||
There was a problem hiding this comment.
Yesss I've been wanting this for a while.
dom.bs
Outdated
| The <var>options</var> argument sets listener-specific options. For compatibility this can be just | ||
| a boolean, in which case the method behaves exactly as if the value was specified as | ||
| <p>The <var>options</var> argument sets listener-specific options. For compatibility this can be a | ||
| boolean, in which case the method behaves exactly as if the value was specified as | ||
| <var>options</var>' <code>capture</code> member. |
There was a problem hiding this comment.
While here you could make all these <code>s link to the dictionary member definitions, as is done in the algorithms below.
dom.bs
Outdated
| <a>event</a>'s {{Event/eventPhase}} attribute value is {{Event/CAPTURING_PHASE}}. Either way, | ||
| <b>callback</b> will be invoked if <a>event</a>'s {{Event/eventPhase}} attribute value is | ||
| {{Event/AT_TARGET}}. | ||
| <p>When set to true, <var>options</var>' <code>capture</code> member prevents |
There was a problem hiding this comment.
While here you could consistify to use "options's" per https://wiki.whatwg.org/wiki/Style#Grammar
There was a problem hiding this comment.
I thought there was still an exception when the word is plural?
There was a problem hiding this comment.
We've had this discussion before :). options is not a class of values; it is a single JS value. So it's not really plural in the grammatical sense that would trigger leaving off the s, where capture belongs to a collection of options.
Stated another way, here "options" is short for "options variable", which is not plural.
A case where it would be plural is "the stock options' combined value was a billion dollars."
dom.bs
Outdated
| <p>The | ||
| <dfn method for=EventTarget><code>addEventListener(<var>type</var>, <var>callback</var>, <var>options</var>)</code></dfn> | ||
| method, when invoked, must run these steps: | ||
| <p>To <dfn export>add an event listener</dfn> given an <code>EventTarget</code> object |
There was a problem hiding this comment.
{{EventTarget}} instead of just code
dom.bs
Outdated
| </ol> | ||
|
|
||
| <p class=note>The <a>add an event listener</a> concept exists to ensure | ||
| <a>event handler attributes</a> use the same code path. [[HTML]] |
|
I think I addressed the feedback, but I ended up touching some unrelated sections since it seemed good to make those suggested editorial changes consistently throughout. |
This hook was provided whatwg/dom#596 and event handlers need to use it to fix whatwg/dom#365 and w3c/ServiceWorker#1004 properly. Tests will be added as part of resolving those issues.
This hook was provided whatwg/dom#596 and event handlers need to use it to fix whatwg/dom#365 and w3c/ServiceWorker#1004 properly. Tests will be added as part of resolving those issues.
This hook ensures that any special casing in addEventListener() is shared with event handler attributes.
Helps with w3c/ServiceWorker#1004.
Preview | Diff