Re: [whatwg/dom] add signal to addEventListener (#919)

@domenic commented on this pull request.



> @@ -1113,6 +1121,11 @@ participate in a tree structure.</p>
  <a for="event listener">callback</a>, and <a for="event listener">capture</a> is
  <var>listener</var>'s <a for="event listener">capture</a>, then <a for=list>append</a>
  <var>listener</var> to <var>eventTarget</var>'s <a>event listener list</a>.
+
+  <li><p>If <var>signal</var> is not null then <a for=AbortSignal lt=add>add the following</a>

You need to look at _listener_'s signal; there is no _signal_ variable declared in this algorithm.

> @@ -1113,6 +1121,11 @@ participate in a tree structure.</p>
  <a for="event listener">callback</a>, and <a for="event listener">capture</a> is
  <var>listener</var>'s <a for="event listener">capture</a>, then <a for=list>append</a>
  <var>listener</var> to <var>eventTarget</var>'s <a>event listener list</a>.
+
+  <li><p>If <var>signal</var> is not null then <a for=AbortSignal lt=add>add the following</a>
+  abort steps to it:
+    <li><a for=list>remove</a> <var>listener</var> from <var>eventTarget</var>'s
+    <a for=EventTarget>event listener list</a>.

What if the signal is already aborted? Probably the whole algorithm should no-op?

> @@ -1113,6 +1121,11 @@ participate in a tree structure.</p>
  <a for="event listener">callback</a>, and <a for="event listener">capture</a> is
  <var>listener</var>'s <a for="event listener">capture</a>, then <a for=list>append</a>
  <var>listener</var> to <var>eventTarget</var>'s <a>event listener list</a>.
+
+  <li><p>If <var>signal</var> is not null then <a for=AbortSignal lt=add>add the following</a>
+  abort steps to it:
+    <li><a for=list>remove</a> <var>listener</var> from <var>eventTarget</var>'s

Missing `<ol>` wrapper

> @@ -1113,6 +1121,11 @@ participate in a tree structure.</p>
  <a for="event listener">callback</a>, and <a for="event listener">capture</a> is
  <var>listener</var>'s <a for="event listener">capture</a>, then <a for=list>append</a>
  <var>listener</var> to <var>eventTarget</var>'s <a>event listener list</a>.
+
+  <li><p>If <var>signal</var> is not null then <a for=AbortSignal lt=add>add the following</a>
+  abort steps to it:
+    <li><a for=list>remove</a> <var>listener</var> from <var>eventTarget</var>'s

This should use "remove an event listener", not list-remove, since that has the additional side effect of setting listener's removed to true (which I believe is important if you remove in the middle of event dispatch).

> @@ -1042,6 +1045,9 @@ are not to be used for anything else. [[!HTML]]
   <a for="event listener">callback</a> will only be invoked once after which the event listener will
   be removed.
 
+  <p>When passed a signal, <var>options</var>'s {{AddEventListenerOptions/signal}} when its
+  [=AbortSignal/aborted flag=] is set the <a for="event listener">callback</a> will be removed.

This phrasing is a bit awkward. In general this section is web-developer facing summary documentation, so it's OK to depart from the nearby sentences. For example, I might suggest something like

> If an `AbortSignal` is passed for _options_'s `signal`, then the event listener will be removed when signal is aborted.

> @@ -278,7 +278,8 @@ function imgFetched(ev) {
 <a>Event listeners</a> can be removed
 by utilizing the
 {{EventTarget/removeEventListener()}}
-method, passing the same arguments.
+method, passing the same arguments or by passing an {{AbortSignal}} to
+{{EventTarget/addEventListener()}}

Suggested phrasing: break this into two sentences, i.e. the existing sentence as-is, plus another stating the alternative way.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/dom/pull/919#pullrequestreview-526516635