add signal to addEventListener #919

benjamingr · 2020-11-09T17:49:31Z

An attempt to try and fix #911 and get more familiar with the spec in the process.

I am happy to contribute WPT for this if that would help.

At least two implementers are interested (and none opposed):
- Chrome Support AbortSignals in addEventListeners options to unsubscribe from events? #911 (comment)
- Firefox add signal to addEventListener #919 (comment)
Tests are written and can be reviewed and commented upon at:
- Test addEventListener abort signal web-platform-tests/wpt#26472
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1146467
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1679204
- Safari: https://bugs.webkit.org/show_bug.cgi?id=218753

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

dom.bs

benjamingr · 2020-11-10T08:17:07Z

Thanks for the feedback @domenic - please take a look :]

benjamingr · 2020-11-10T08:19:22Z

I am also not sure what "Participation" is and I believe I contributed to whatwg small fixes to repos (like whatwg/fetch) in the past - but just in case I signed https://participate.whatwg.org/submit-agreement since that's what the contribution guide pointed to

annevk · 2020-11-10T09:59:07Z

(You contributed to Fetch before WHATWG had an IPR Policy in place.)

benjamingr · 2020-11-10T15:42:17Z

@annevk thanks, I've signed the agreement.

I'll move this PR from draft - though I am not sure what the next steps to contribute are.

A browser (chrome) has expressed interest in this and opened a crbug and work item for this (yay!).

Do I:

Prototype this in Node.js using our own EventTarget? (probably won't count/help right?)
Write down WPTs for the feature?
Open bugs for other browsers and ask them for feedback?

Sorry if it's a stupid question - feel free to point me towards reading material :]

annevk · 2020-11-10T15:53:43Z

All of these seem like good steps to take (and the latter two are required, see https://whatwg.org/working-mode#changes). Node.js wouldn't count on its own indeed, but it might help with persuading implementers and verifying the approach. I'll ping some people internally too.

benjamingr · 2020-11-10T16:21:03Z

Great, thanks (appreciate it!).

I've opened https://bugs.webkit.org/show_bug.cgi?id=218753 in WebKit - would it be better if I opened the bug for Firefox or if you did it?

I'll try to follow up with WPTs next week or asap.

annevk · 2020-11-10T16:37:25Z

FWIW, I checked and I'm pretty confident that Mozilla is supportive of adding this as well. Maybe wait with filing a bug until the tests and such are there.

dom.bs

domenic · 2020-11-10T16:59:08Z

@benjamingr to confirm on the agreement front, you signed up saying that you do not work in the field of web technologies. Is that correct?

benjamingr · 2020-11-10T17:02:33Z

@domenic

you signed up saying that you do not work in the field of web technologies. Is that correct?

Correct, my role at Testim.io does not involve working on "creating and influencing web standards and web technologies that could be adopted by the web community" - all of that is my free time though I hope this changes in the future.

I have not (ever) been paid by an employer company or business entity for any of the open source work I do.

domenic · 2020-11-10T17:03:42Z

Awesome, thanks for confirming; I'll verify you then.

benjamingr · 2020-11-10T17:26:42Z

Initial work on WPTs: web-platform-tests/wpt#26472

annevk

Thanks for working on this, found a final set of nits, but it seems this is ready to land soon.

dom.bs

Adds support for AbortSignals in EventTarget's addEventListener options

benjamingr · 2020-11-30T17:08:54Z

Hey, thanks. I've addressed the comments above, rebased and squashed to it's one clear commit. PTAL :]

annevk

Let me know if you want help with the final nits. Since I've done a couple rounds I would appreciate if @domenic could do a final spot check as well and then we can merge.

dom.bs

benjamingr · 2020-12-01T17:00:08Z

I am in no rush to merge this and am happy to improve things. My motivation for working on this was to learn how to contribute better so I am happy to iterate on this more if you have more suggestions. I do want to eventually merge this :)

benjamingr · 2020-12-02T15:14:19Z

I just landed an implementation of this spec in Node.js as well (with the relevant WPTs) :]

annevk · 2020-12-03T15:38:39Z

@benjamingr want to put your name in the Acknowledgments section? I will go ahead and merge the tests as that is the only question blocking this as far as I'm concerned.

Tests for whatwg/dom#919.

benjamingr · 2020-12-03T15:56:35Z

Sure

annevk · 2020-12-03T16:06:06Z

Congrats @benjamingr on getting this landed! Great work and really cool to see it might be adopted quickly by both browsers and Node.js.

…stonly Automatic update from web-platform-tests DOM: signal support for addEventListener() Tests for whatwg/dom#919. -- wpt-commits: 625e1310ce19e9dde25b01f9eda0452c6ec274da wpt-pr: 26472

…stonly Automatic update from web-platform-tests DOM: signal support for addEventListener() Tests for whatwg/dom#919. -- wpt-commits: 625e1310ce19e9dde25b01f9eda0452c6ec274da wpt-pr: 26472 UltraBlame original commit: 56e1a8d30892478277b812e71b2af7467dae595f

…stonly Automatic update from web-platform-tests DOM: signal support for addEventListener() Tests for whatwg/dom#919. -- wpt-commits: 625e1310ce19e9dde25b01f9eda0452c6ec274da wpt-pr: 26472 UltraBlame original commit: fa142f3429f24749a5c3d236cf535973c19be0b1

…stonly Automatic update from web-platform-tests DOM: signal support for addEventListener() Tests for whatwg/dom#919. -- wpt-commits: 625e1310ce19e9dde25b01f9eda0452c6ec274da wpt-pr: 26472 UltraBlame original commit: 56e1a8d30892478277b812e71b2af7467dae595f

…stonly Automatic update from web-platform-tests DOM: signal support for addEventListener() Tests for whatwg/dom#919. -- wpt-commits: 625e1310ce19e9dde25b01f9eda0452c6ec274da wpt-pr: 26472 UltraBlame original commit: fa142f3429f24749a5c3d236cf535973c19be0b1

…stonly Automatic update from web-platform-tests DOM: signal support for addEventListener() Tests for whatwg/dom#919. -- wpt-commits: 625e1310ce19e9dde25b01f9eda0452c6ec274da wpt-pr: 26472 UltraBlame original commit: 56e1a8d30892478277b812e71b2af7467dae595f

…stonly Automatic update from web-platform-tests DOM: signal support for addEventListener() Tests for whatwg/dom#919. -- wpt-commits: 625e1310ce19e9dde25b01f9eda0452c6ec274da wpt-pr: 26472 UltraBlame original commit: fa142f3429f24749a5c3d236cf535973c19be0b1

…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

…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

domenic reviewed Nov 9, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

tobie mentioned this pull request Nov 10, 2020

WHATWG PR snapshots are wrong speced/bikeshed#1818

Closed

benjamingr marked this pull request as ready for review November 10, 2020 16:01

annevk requested a review from domenic November 10, 2020 16:37

annevk added the topic: aborting AbortController and AbortSignal label Nov 10, 2020

domenic reviewed Nov 10, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

benjamingr mentioned this pull request Nov 10, 2020

Test addEventListener abort signal web-platform-tests/wpt#26472

Merged

benjamingr mentioned this pull request Nov 10, 2020

RFC - EventTarget API Enhancements nodejs/node#36073

Closed

This was referenced Nov 25, 2020

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

Closed

events: support signal in EventTarget nodejs/node#36258

Closed

annevk reviewed Nov 30, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Show resolved Hide resolved

add signal to addEventListener

d67e335

Adds support for AbortSignals in EventTarget's addEventListener options

benjamingr force-pushed the event-target-signal branch from a0a5dd9 to d67e335 Compare November 30, 2020 17:08

annevk reviewed Dec 1, 2020

View reviewed changes

dom.bs Outdated Show resolved Hide resolved

dom.bs Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

dom.bs Outdated Show resolved Hide resolved

more code review

ab05ff3

annevk added 2 commits December 2, 2020 10:33

nits

5266342

make the one other event listener dictionary thing consistent

b2df711

annevk requested a review from domenic December 2, 2020 09:37

domenic approved these changes Dec 2, 2020

View reviewed changes

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 3, 2020

DOM: signal support for addEventListener()

625e131

Tests for whatwg/dom#919.

add myself to acks

0699e74

annevk merged commit 83037a1 into whatwg:master Dec 3, 2020

benjamingr mentioned this pull request Dec 4, 2020

feat(op_crates/web) EventTarget signal support denoland/deno#8616

Merged

lacolaco mentioned this pull request Dec 23, 2020

Proposal: Component AbortSignal Support angular/angular#40247

Closed

mysticatea mentioned this pull request Jan 7, 2021

[DOM API] signal option on EventTarget.protoype.addEventListener Fyrd/caniuse#5732

Closed

josepharhar mentioned this pull request Jan 26, 2021

Adding AbortSignal option to addEventListener w3ctag/design-reviews#569

Closed

1 task

sideshowbarker added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Feb 28, 2021

ddbeck mentioned this pull request Mar 22, 2021

api.EventTarget.addEventListener.options - Missing feature for signal option mdn/browser-compat-data#9555

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add signal to addEventListener #919

add signal to addEventListener #919

benjamingr commented Nov 9, 2020 •

edited by pr-preview bot

Loading

benjamingr commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk commented Nov 10, 2020

domenic commented Nov 10, 2020

benjamingr commented Nov 10, 2020

domenic commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk left a comment

benjamingr commented Nov 30, 2020

annevk left a comment

benjamingr commented Dec 1, 2020

benjamingr commented Dec 2, 2020

annevk commented Dec 3, 2020

benjamingr commented Dec 3, 2020

annevk commented Dec 3, 2020

add signal to addEventListener #919

add signal to addEventListener #919

Conversation

benjamingr commented Nov 9, 2020 • edited by pr-preview bot Loading

benjamingr commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk commented Nov 10, 2020

domenic commented Nov 10, 2020

benjamingr commented Nov 10, 2020

domenic commented Nov 10, 2020

benjamingr commented Nov 10, 2020

annevk left a comment

Choose a reason for hiding this comment

benjamingr commented Nov 30, 2020

annevk left a comment

Choose a reason for hiding this comment

benjamingr commented Dec 1, 2020

benjamingr commented Dec 2, 2020

annevk commented Dec 3, 2020

benjamingr commented Dec 3, 2020

annevk commented Dec 3, 2020

benjamingr commented Nov 9, 2020 •

edited by pr-preview bot

Loading