Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support AbortSignals in addEventListeners options to unsubscribe from events? #911

Closed
benjamingr opened this issue Oct 29, 2020 · 23 comments · Fixed by #919
Closed

Support AbortSignals in addEventListeners options to unsubscribe from events? #911

benjamingr opened this issue Oct 29, 2020 · 23 comments · Fixed by #919
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs implementer interest Moving the issue forward requires implementers to express interest topic: aborting AbortController and AbortSignal topic: events

Comments

@benjamingr
Copy link
Member

Since Node added (experimental) support for AbortController, we have also added support for out event utility functions (that already work with EventTargets) for cancellation:

import { once, EventEmitter } from 'events';
import { setTimeout } from 'timers/promises;

const et = new EventTarget();
const ee = new EventEmitter();
const ac = new AbortController();

setTimeout(100).then(() => ac.abort());

let { signal } = ac;
// run with experimental tla flag or wrap in an iife
await once(ee, 'foo', { signal }); // cancel waiting for the event, removes the listener
await once(et, 'foo', { signal }); // same thing, with event target, removes the listener
for await(const item of on(ee, 'foo' , { signal } )) { // same thing, but with async iterator variant
}

It would be useful to "upstream" this behaviour to the DOM specification as it would be useful to have in browsers:

// Does not yet work, suggested:
const ac = new AbortController();
let { signal } = ac;
const et = new EventTarget();
et.addEventListener('foo', (e) => {
  
}, { signal } );
ac.abort(); // removes the listener from the event target, same as et.removeEventListener('foo', thatFunctionReference)

It's useful to use the web's cancellation platform (AbortController) to cancel listening to an event. This is also ergonomic for frameworks/libraries that set up a lot of event listeners and could unsubscribe from all of them in one go (through the controller).

Was this suggested at some point in the past? (I couldn't find it) Is this a good idea? A bad idea?

cc @domenic @annevk

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: events labels Oct 29, 2020
@annevk
Copy link
Member

annevk commented Oct 29, 2020

I see, so this would remove event listeners upon the signal that is associated with them being aborted? I haven't seen suggestions for this.

@benjamingr
Copy link
Member Author

I see, so this would remove event listeners upon the signal that is associated with them being aborted? I haven't seen suggestions for this.

Yes, basically it would be a way to make working with EventTargets more consistent with other DOM APIs.

For example: the person who asked me for this said "I need to make a few requests and subscribe to a few events when the user navigates to a certain page in my SPA. I am already using an AbortController to abort all the pending requests if the user navigates away - it would be very useful to also unsubscribe from all events the same way essentially having one way to 'cleanup' "

@domenic
Copy link
Member

domenic commented Oct 29, 2020

This was suggested in the context of .on() and observables in #544, but extending addEventListener seems like a nice incremental approach.

I'm quite supportive of this change. @mfreed7 do you think this would be something the Chromium DOM team could plan to implement? I know it's always a bit hard to make the case for spending time on minor ergonomic improvements, but see above for some web developer testimony on why this would be appreciated.

@mfreed7
Copy link
Contributor

mfreed7 commented Nov 6, 2020

I'm quite supportive of this change. @mfreed7 do you think this would be something the Chromium DOM team could plan to implement? I know it's always a bit hard to make the case for spending time on minor ergonomic improvements, but see above for some web developer testimony on why this would be appreciated.

This definitely seems like a nice addition to the platform - I can see the use case for cleaning everything up in one way. I can't guarantee timing, but we could probably take a look at prototyping this, if someone else can handle the spec work?

@mfreed7
Copy link
Contributor

mfreed7 commented Nov 10, 2020

Chromium bug filed: https://bugs.chromium.org/p/chromium/issues/detail?id=1146467

@annevk annevk added the topic: aborting AbortController and AbortSignal label Nov 10, 2020
@josepharhar
Copy link
Contributor

@benjamingr
Copy link
Member Author

benjamingr commented Nov 10, 2020

That's awesome @josepharhar thanks!

I've started working on WPTs here: web-platform-tests/wpt#26472 I haven't run them or tested them fully yet but feel free to use the cases.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 11, 2020
I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 11, 2020
I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 12, 2020
I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 12, 2020
I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527343
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826659}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 12, 2020
I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527343
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826659}
pull bot pushed a commit to Mu-L/chromium that referenced this issue Nov 12, 2020
I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527343
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826659}
@josepharhar
Copy link
Contributor

I merged the implementation into chromium.
To try it out, install chrome canary and enable the "Experimental Web Platform Features" flag here: chrome://flags/#enable-experimental-web-platform-features

It isn't actually in canary yet at the time of writing this comment, but it should be in the next day or two. If you want to check, see if this page has a release version, and if it does, see if the your version in canary in chrome://version is newer than or equal to the version on that webpage.

I filed a tag review here to get feedback for the feature: w3ctag/design-reviews#569

I've started working on WPTs here: web-platform-tests/wpt#26472 I haven't run them or tested them fully yet but feel free to use the cases.

Thanks! I didn't catch this and I also added a WPT, but the more test coverage the better! I trust you much more than myself to capture all the use cases. If there are any edge cases you have in mind or possible differences between implementations, please test them.

@benjamingr
Copy link
Member Author

benjamingr commented Nov 12, 2020

@josepharhar that's great (and very quick!) thanks a bunch. I will play with it and I will also PR this feature into Node.js's emitter. I expect to make progress over the weekend (I don't actually work on this as part of my job, just a hobby :]).

It isn't actually in canary yet at the time of writing this comment, but it should be in the next day or two. If you want to check, see if this page has a release version, and if it does, see if the your version in canary in chrome://version is newer than or equal to the version on that webpage.

I'll just fetch (sync) and build (I have chromium from-source and depot-tools and all that stuff locally, it just builds slowly :])

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 13, 2020
…ddEventListener, a=testonly

Automatic update from web-platform-tests
Prototype of adding AbortController to addEventListener

I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527343
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826659}

--

wpt-commits: 612b2ed03c8e59bcdbde7dea44bc467ca0623b44
wpt-pr: 26462
@benjamingr
Copy link
Member Author

Hey @josepharhar generally the implementation looks fine and works well. I've tested a few different cases (calling .abort during emit between listeners, once, passive, bubbling etc). I'll try testing it in a big code base to see if I run into more issue :]

The only issue I noticed is that passing a signal doesn't seem to work when passing capture:

{
  const et = new EventTarget();
  const ac = new AbortController();
  et.addEventListener('foo', () => console.log('1'), { signal: ac.signal, once: true });
  et.addEventListener('foo', () => console.log('2'), { signal: ac.signal });
  et.addEventListener('foo', () => console.log('3'), { signal: ac.signal, capture: true });
  ac.abort();
  et.dispatchEvent(new Event('foo')); // 1 and 2 are not logged, 3 is 
}

Screen Shot 2020-11-14 at 0 13 01

@josepharhar
Copy link
Contributor

Thanks for finding that capture bug! Fix (with a WPT) is on the way: https://chromium-review.googlesource.com/c/chromium/src/+/2538368

@benjamingr
Copy link
Member Author

benjamingr commented Nov 14, 2020

Thanks, I now ran the actual WPTs I wrote - there is one more failure here - the following test (add a listener with an already aborted controller where the same listener was already added) seems to fail (and the "capture" one but that's already handled :])

./wpt run --channel dev --binary ../chromium/src/out/Default/Chromium.app/Contents/MacOS/Chromium chrome dom/events/AddEventListenerOptions-signal.html
test(function() {
  let count = 0;
  function handler() {
    count++;
  }
  const et = new EventTarget();
  const controller = new AbortController();
  et.addEventListener('test', handler, { signal: controller.signal });
  et.dispatchEvent(new Event('test'));
  assert_equals(count, 1, "Adding a signal still adds a listener");
  et.dispatchEvent(new Event('test'));
  assert_equals(count, 2, "The listener was not added with the once flag");
  controller.abort();
  et.dispatchEvent(new Event('test'));
  assert_equals(count, 2, "Aborting on the controller removes the listener");
  et.addEventListener('test', handler, { signal: controller.signal });
  et.dispatchEvent(new Event('test'));
  assert_equals(count, 2, "Passing an aborted signal never adds the handler");
}, "Passing an AbortSignal to addEventListener options should allow removing a listener");

aosmond pushed a commit to aosmond/gecko that referenced this issue Nov 15, 2020
…ddEventListener, a=testonly

Automatic update from web-platform-tests
Prototype of adding AbortController to addEventListener

I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527343
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826659}

--

wpt-commits: 612b2ed03c8e59bcdbde7dea44bc467ca0623b44
wpt-pr: 26462
@josepharhar
Copy link
Contributor

Thanks, I now ran the actual WPTs I wrote - there is one more failure here - the following test (add a listener with an already aborted controller where the same listener was already added) seems to fail (and the "capture" one but that's already handled :])

Thanks! Another fix on the way: https://chromium-review.googlesource.com/c/chromium/src/+/2541503
I'm copying the WPT you shared into dom/abort/addEventListenerAbortController.tentative.html, but feel free to move it out of there back into the new file in your WPT PR or whatever you'd like.

@benjamingr
Copy link
Member Author

@josephhahar thanks! I've added a few more WPTs and changed the format to also work in workers in web-platform-tests/wpt#26472 given Anne's suggestions :]

@smaug----
Copy link
Collaborator

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1679204

@benjamingr
Copy link
Member Author

benjamingr commented Nov 25, 2020

@annevk can you please explain what the next steps in the process are?

We have:

To be extra clear - I am in no rush, thankful that I get to work on this and am doing this for fun and to improve the web platform. I just want to understand what the next steps are.

Edit: I've also gone ahead and made a PR to Node.js to add this at https://github.com/nodejs/node/pull/36258/files

@annevk
Copy link
Member

annevk commented Nov 30, 2020

Hey @benjamingr, everything looks pretty splendid! I left some comments on the PR and test PR, but nothing major. I think you covered all the bullet points from https://whatwg.org/working-mode#changes.

annevk pushed a commit that referenced this issue Dec 3, 2020
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Jan 25, 2021
…ents

https://bugs.webkit.org/show_bug.cgi?id=218753
<rdar://problem/71258012>

Reviewed by Darin Adler.

LayoutTests/imported/w3c:
Import test coverage from WPT.

* web-platform-tests/dom/events/AddEventListenerOptions-signal.any-expected.txt: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.html: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.js: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.worker-expected.txt: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.worker.html: Added.
* web-platform-tests/dom/events/w3c-import.log:

Source/WebCore:
Support AbortSignal in addEventListenerOptions to unsubscribe from events:
- whatwg/dom#911
- whatwg/dom#919

Blink already added support for this.

Tests: imported/w3c/web-platform-tests/dom/events/AddEventListenerOptions-signal.any.html
       imported/w3c/web-platform-tests/dom/events/AddEventListenerOptions-signal.any.worker.html

* CMakeLists.txt:
* DerivedSources-input.xcfilelist:
* DerivedSources-output.xcfilelist:
* DerivedSources.make:
* Headers.cmake:
* Modules/async-clipboard/Clipboard.h:
* Modules/encryptedmedia/MediaKeySession.h:
* Modules/indexeddb/IDBRequest.h:
* Modules/mediastream/MediaDevices.h:
* Modules/mediastream/RTCPeerConnection.h:
* Modules/paymentrequest/PaymentRequest.h:
* Modules/speech/SpeechRecognition.h:
* Modules/webaudio/BaseAudioContext.h:
* Modules/webgpu/WebGPUDevice.h:
* Modules/webxr/WebXRSystem.h:
* Sources.txt:
* WebCore.xcodeproj/project.pbxproj:
* animation/WebAnimation.h:
* css/MediaQueryList.cpp:
(WebCore::MediaQueryList::addListener):
(WebCore::MediaQueryList::removeListener):
* css/MediaQueryList.h:
* dom/AbortSignal.h:
* dom/AddEventListenerOptions.h: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
(WebCore::AddEventListenerOptions::AddEventListenerOptions):
* dom/AddEventListenerOptions.idl: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
* dom/EventListener.h:
* dom/EventListenerMap.cpp:
* dom/EventListenerOptions.h: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
(WebCore::EventListenerOptions::EventListenerOptions):
* dom/EventListenerOptions.idl: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
* dom/EventTarget.cpp:
(WebCore::EventTarget::addEventListener):
(WebCore::EventTarget::removeEventListenerForBindings):
(WebCore::EventTarget::removeEventListener):
(WebCore::EventTarget::setAttributeEventListener):
(WebCore::EventTarget::innerInvokeEventListeners):
* dom/EventTarget.h:
(WebCore::EventTarget::removeEventListener):
* dom/EventTarget.idl:
* dom/MessagePort.cpp:
(WebCore::MessagePort::MessagePort):
(WebCore::MessagePort::removeEventListener):
* dom/MessagePort.h:
* dom/Node.cpp:
(WebCore::tryAddEventListener):
(WebCore::tryRemoveEventListener):
(WebCore::Node::removeEventListener):
* dom/Node.h:
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::removeEventListener):
* html/HTMLMediaElement.h:
* html/ImageDocument.cpp:
* html/track/TextTrackCue.h:
* inspector/agents/InspectorDOMAgent.cpp:
* loader/appcache/DOMApplicationCache.h:
* page/DOMWindow.cpp:
(WebCore::DOMWindow::removeEventListener):
* page/DOMWindow.h:
* platform/cocoa/PlaybackSessionModelMediaElement.mm:
* platform/cocoa/VideoFullscreenModelVideoElement.mm:
* svg/SVGElement.cpp:
(WebCore::SVGElement::removeEventListener):
* svg/SVGElement.h:
* svg/SVGTRefElement.cpp:
* svg/animation/SVGSMILElement.cpp:
* testing/Internals.cpp:
* workers/service/ServiceWorkerContainer.h:

Source/WebKit:
Minor build fixes.

* WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:

Source/WebKitLegacy/mac:
Minor build fixes.

* DOM/DOMNode.mm:

Source/WTF:
Add initializeWeakPtrFactory() protection function to CanMakeWeakPtr so that a subclass
can eagerly initialize the WeakPtrFactory even if it does not subclass
WeakPtrFactory<T, WeakPtrFactoryInitialization::Eager>. MessagePort used to subclass
WeakPtrFactory<T, WeakPtrFactoryInitialization::Eager> for thread-safety reason but it
now subclasses WeakPtrFactory<T, WeakPtrFactoryInitialization::Lazy> via EventTarget.

* wtf/WeakPtr.h:
(WTF::CanMakeWeakPtr::initializeWeakPtrFactory):


Canonical link: https://commits.webkit.org/233312@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271806 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@josepharhar
Copy link
Contributor

I just filed an intent to ship for chrome so I can enable this feature by default.
This should probably get documented on MDN, right?

@annevk annevk added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Jan 26, 2021
@annevk
Copy link
Member

annevk commented Jan 26, 2021

@whatwg/documentation it would be good to put this on MDN.

@josepharhar
Copy link
Contributor

This will be enabled by default in chrome 90

bertogg pushed a commit to Igalia/webkit that referenced this issue Feb 1, 2021
…ents

https://bugs.webkit.org/show_bug.cgi?id=218753
<rdar://problem/71258012>

Reviewed by Darin Adler.

LayoutTests/imported/w3c:
Import test coverage from WPT.

* web-platform-tests/dom/events/AddEventListenerOptions-signal.any-expected.txt: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.html: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.js: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.worker-expected.txt: Added.
* web-platform-tests/dom/events/AddEventListenerOptions-signal.any.worker.html: Added.
* web-platform-tests/dom/events/w3c-import.log:

Source/WebCore:
Support AbortSignal in addEventListenerOptions to unsubscribe from events:
- whatwg/dom#911
- whatwg/dom#919

Blink already added support for this.

Tests: imported/w3c/web-platform-tests/dom/events/AddEventListenerOptions-signal.any.html
       imported/w3c/web-platform-tests/dom/events/AddEventListenerOptions-signal.any.worker.html

* CMakeLists.txt:
* DerivedSources-input.xcfilelist:
* DerivedSources-output.xcfilelist:
* DerivedSources.make:
* Headers.cmake:
* Modules/async-clipboard/Clipboard.h:
* Modules/encryptedmedia/MediaKeySession.h:
* Modules/indexeddb/IDBRequest.h:
* Modules/mediastream/MediaDevices.h:
* Modules/mediastream/RTCPeerConnection.h:
* Modules/paymentrequest/PaymentRequest.h:
* Modules/speech/SpeechRecognition.h:
* Modules/webaudio/BaseAudioContext.h:
* Modules/webgpu/WebGPUDevice.h:
* Modules/webxr/WebXRSystem.h:
* Sources.txt:
* WebCore.xcodeproj/project.pbxproj:
* animation/WebAnimation.h:
* css/MediaQueryList.cpp:
(WebCore::MediaQueryList::addListener):
(WebCore::MediaQueryList::removeListener):
* css/MediaQueryList.h:
* dom/AbortSignal.h:
* dom/AddEventListenerOptions.h: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
(WebCore::AddEventListenerOptions::AddEventListenerOptions):
* dom/AddEventListenerOptions.idl: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
* dom/EventListener.h:
* dom/EventListenerMap.cpp:
* dom/EventListenerOptions.h: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
(WebCore::EventListenerOptions::EventListenerOptions):
* dom/EventListenerOptions.idl: Copied from Source/WebCore/loader/appcache/DOMApplicationCache.h.
* dom/EventTarget.cpp:
(WebCore::EventTarget::addEventListener):
(WebCore::EventTarget::removeEventListenerForBindings):
(WebCore::EventTarget::removeEventListener):
(WebCore::EventTarget::setAttributeEventListener):
(WebCore::EventTarget::innerInvokeEventListeners):
* dom/EventTarget.h:
(WebCore::EventTarget::removeEventListener):
* dom/EventTarget.idl:
* dom/MessagePort.cpp:
(WebCore::MessagePort::MessagePort):
(WebCore::MessagePort::removeEventListener):
* dom/MessagePort.h:
* dom/Node.cpp:
(WebCore::tryAddEventListener):
(WebCore::tryRemoveEventListener):
(WebCore::Node::removeEventListener):
* dom/Node.h:
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::removeEventListener):
* html/HTMLMediaElement.h:
* html/ImageDocument.cpp:
* html/track/TextTrackCue.h:
* inspector/agents/InspectorDOMAgent.cpp:
* loader/appcache/DOMApplicationCache.h:
* page/DOMWindow.cpp:
(WebCore::DOMWindow::removeEventListener):
* page/DOMWindow.h:
* platform/cocoa/PlaybackSessionModelMediaElement.mm:
* platform/cocoa/VideoFullscreenModelVideoElement.mm:
* svg/SVGElement.cpp:
(WebCore::SVGElement::removeEventListener):
* svg/SVGElement.h:
* svg/SVGTRefElement.cpp:
* svg/animation/SVGSMILElement.cpp:
* testing/Internals.cpp:
* workers/service/ServiceWorkerContainer.h:

Source/WebKit:
Minor build fixes.

* WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:

Source/WebKitLegacy/mac:
Minor build fixes.

* DOM/DOMNode.mm:

Source/WTF:
Add initializeWeakPtrFactory() protection function to CanMakeWeakPtr so that a subclass
can eagerly initialize the WeakPtrFactory even if it does not subclass
WeakPtrFactory<T, WeakPtrFactoryInitialization::Eager>. MessagePort used to subclass
WeakPtrFactory<T, WeakPtrFactoryInitialization::Eager> for thread-safety reason but it
now subclasses WeakPtrFactory<T, WeakPtrFactoryInitialization::Lazy> via EventTarget.

* wtf/WeakPtr.h:
(WTF::CanMakeWeakPtr::initializeWeakPtrFactory):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@271806 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@jimmywarting

This comment has been minimized.

@sideshowbarker
Copy link
Contributor

FYI: I raised mdn/content#2759 with a patch that documents this feature in MDN. Review welcome (from anybody interested).

@caub
Copy link

caub commented Apr 29, 2021

Oh great feature, so something like that below can be rewritten with AbortController

button.addEventListener('click', () => {
  // ...

  function closeByClick() {
    // ... do stuff
    closeButton.removeEventListener('click', closeByClick);
    window.removeEventListener('keydown', closeByEscape);
  }
  function closeByEscape(e) {
    if (e.key !== 'Escape') return;
    // ... do stuff
    closeButton.removeEventListener('click', closeByClick);
    window.removeEventListener('keydown', closeByEscape);
  }

  closeButton.addEventListener('click', closeByClick);
  window.addEventListener('keydown', closeByEscape);
});

like this:

button.addEventListener('click', () => {
  // ...
  const controller = new AbortController();

  closeButton.addEventListener('click', () => {
    // ... do stuff
    controller.abort();
  }, { signal: controller.signal });

  window.addEventListener('keydown', e => {
    if (e.key !== 'Escape') return;
    // ... do stuff
    controller.abort();
  }, { signal: controller.signal });
});

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
I2P: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9396JedBBOM

This feature adds a new AbortSignal option, named "signal", to the
options parameter of addEventListener.
This new option can be used to remove the event listener with an
AbortController by calling abort().
More context: whatwg/dom#911

Bug: 1146467
Change-Id: I8ab1b93ec8be859c6876296fda4a9b808253c7d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527343
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826659}
GitOrigin-RevId: f0bbc3f3ab2bf91bea606fbc5fda751f5e60a0d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs implementer interest Moving the issue forward requires implementers to express interest topic: aborting AbortController and AbortSignal topic: events
Development

Successfully merging a pull request may close this issue.

9 participants