Test addEventListener abort signal #26472

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

benjamingr · 2020-11-14T08:30:22Z

I think the WPTs are ready for review :]

annevk

Thanks for writing these, looks like a good start!

If you want to test interaction with once/capture it seems you should also check the event listener is still correctly invoked for them.

We should also test passing the signal to multiple listeners, both with different event names and the same event name. And there also check what happens if you abort from one of the listeners (which listeners will still get invoked and which will not).

You might also want to consider https://web-platform-tests.org/writing-tests/testharness.html#tests-for-other-or-multiple-globals-any-js to reduce the boilerplate and make these tests even more reusable across JavaScript hosts.

annevk · 2020-11-16T09:29:33Z

dom/events/AddEventListenerOptions-signal.html

+  const et = new EventTarget();
+  const controller = new AbortController();
+  et.addEventListener('test', handler, { signal: controller.signal, once: true });
+  controller.abort();


Why would this not remove it already? I'm not sure how this test works.

This would remove it already, this test checks that even if I add an event listener with once, passing a signal and aborting the controller removes it :]

I see, that makes sense. We should also test the inverse, that once still removes it if you pass a signal in.

I'll add tests for that and cases you mentioned as well.

It seems there's still no test that tests once itself. I.e., dispatch an event twice and only bring count up to 1?

benjamingr · 2020-11-16T14:54:30Z

Done, thanks for the review please take a look :]

annevk · 2020-11-16T16:08:44Z

I think there should be more coverage for the multiple listener scenarios. E.g., multiple listeners for the same name and signal, but different handlers. And then one of the handlers aborts. The remaining handlers should not run (due to the removed field of their event listener being set).

benjamingr · 2020-11-16T16:24:21Z

@annevk

, multiple listeners for the same name and signal, but different handlers. And then one of the handlers aborts.

Is that not this test?

smaug---- · 2020-11-16T23:00:39Z

I'd like to see tests where one listener adds another listener with signal and then aborts it.
And similar but with nested event dispatch.

benjamingr · 2020-11-19T09:52:51Z

@smaug---- thanks! I've added a test for your first suggestion (and rebased) . I'm not sure what you mean by "similar but with nested event dispatch".

benjamingr · 2020-11-21T10:00:12Z

Just updating that I've added the "nested event dispatch" test after talking about it with @smaug---- and Anne on #whatwg

annevk · 2020-11-30T13:58:54Z

Should we remove the test added in #26462 as part of this or incorporate it? If we keep it as-is we should remove .tentative from the filename.

benjamingr · 2020-11-30T16:57:50Z

@annevk likely remove since it's tested in https://github.com/web-platform-tests/wpt/pull/26472/files#diff-96216b6e1d920cc33c8560820458aeb2f57e707edcc1c290bf642163398aabd4R15-R17 but there might be merit in a simpler tests that test a more basic use case?

annevk · 2020-12-01T10:27:00Z

Not that I know of; let's remove it as part of this PR.

cc @josepharhar

benjamingr · 2020-12-01T16:58:15Z

Done

Tests: web-platform-tests/wpt#26472. Fixes #911.

This passes the tests which are in web-platform-tests/wpt#26472 Because of complications in #include handling, AbortFollower needs to be in a different header file than AbortSignal, yet AbortSignalImpl needs to be available when AbortFollower is used. Another option would have been to make DOMEventTargetHelper.h a bit different and uninline some hot methods there or move them to another file, but that would have been equally bad and Abort* is used way less often. AbortFollower and AbortSignalImpl are thus just moved to a new header. Memory management is such that Listener in EventListenerManager owns the possible ListenerSignalFollower instance which follows the relevant signal. In order to be able remove event listener, ListenerSignalFollower has many similar fields as Listener. ListenerSignalFollower can't easily have just a pointer to Listener* since Listener isn't stored as a pointer in EventListenerManager. ListenerSignalFollower is cycle collectable so that Listener->ListenerSignalFollower can be traversed/unlinked and also strong pointers in ListenerSignalFollower itself can be traversed/unlinked. There is an XXX in the .webidl, since nullability of signal is unclear in the spec pr. Whether or not it ends up being nullable shouldn't change the actual C++ implementation. Differential Revision: https://phabricator.services.mozilla.com/D97938

This passes the tests which are in web-platform-tests/wpt#26472 Because of complications in #include handling, AbortFollower needs to be in a different header file than AbortSignal, yet AbortSignalImpl needs to be available when AbortFollower is used. Another option would have been to make DOMEventTargetHelper.h a bit different and uninline some hot methods there or move them to another file, but that would have been equally bad and Abort* is used way less often. AbortFollower and AbortSignalImpl are thus just moved to a new header. Memory management is such that Listener in EventListenerManager owns the possible ListenerSignalFollower instance which follows the relevant signal. In order to be able remove event listener, ListenerSignalFollower has many similar fields as Listener. ListenerSignalFollower can't easily have just a pointer to Listener* since Listener isn't stored as a pointer in EventListenerManager. ListenerSignalFollower is cycle collectable so that Listener->ListenerSignalFollower can be traversed/unlinked and also strong pointers in ListenerSignalFollower itself can be traversed/unlinked. There is an XXX in the .webidl, since nullability of signal is unclear in the spec pr. Whether or not it ends up being nullable shouldn't change the actual C++ implementation. Differential Revision: https://phabricator.services.mozilla.com/D97938 UltraBlame original commit: b3d0fba5fd8d624b5abf53df196aedae5e83a269

This passes the tests which are in web-platform-tests/wpt#26472 Because of complications in #include handling, AbortFollower needs to be in a different header file than AbortSignal, yet AbortSignalImpl needs to be available when AbortFollower is used. Another option would have been to make DOMEventTargetHelper.h a bit different and uninline some hot methods there or move them to another file, but that would have been equally bad and Abort* is used way less often. AbortFollower and AbortSignalImpl are thus just moved to a new header. Memory management is such that Listener in EventListenerManager owns the possible ListenerSignalFollower instance which follows the relevant signal. In order to be able remove event listener, ListenerSignalFollower has many similar fields as Listener. ListenerSignalFollower can't easily have just a pointer to Listener* since Listener isn't stored as a pointer in EventListenerManager. ListenerSignalFollower is cycle collectable so that Listener->ListenerSignalFollower can be traversed/unlinked and also strong pointers in ListenerSignalFollower itself can be traversed/unlinked. There is an XXX in the .webidl, since nullability of signal is unclear in the spec pr. Whether or not it ends up being nullable shouldn't change the actual C++ implementation. Differential Revision: https://phabricator.services.mozilla.com/D97938

This was referenced Nov 10, 2020

add signal to addEventListener whatwg/dom#919

Merged

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

Closed

benjamingr marked this pull request as ready for review November 14, 2020 08:30

wpt-pr-bot added the dom label Nov 14, 2020

wpt-pr-bot assigned zqzhang Nov 14, 2020

wpt-pr-bot requested review from annevk, jdm and zqzhang November 14, 2020 08:30

annevk reviewed Nov 16, 2020

View reviewed changes

benjamingr changed the title ~~initial addEventListener signal tests~~ Test addEventListener abort signal Nov 16, 2020

benjamingr force-pushed the test-eventtarget-signal branch from 2d0c0d3 to 0811102 Compare November 19, 2020 09:52

benjamingr mentioned this pull request Nov 25, 2020

events: support signal in EventTarget nodejs/node#36258

Closed

4 tasks

benjamingr added 2 commits December 1, 2020 18:56

initial addEventListener signal tests

f9b3e43

fixup! cr

13f930b

benjamingr force-pushed the test-eventtarget-signal branch from 279129b to 13f930b Compare December 1, 2020 16:57

annevk approved these changes Dec 2, 2020

View reviewed changes

annevk merged commit 625e131 into web-platform-tests:master Dec 3, 2020

annevk pushed a commit to whatwg/dom that referenced this pull request Dec 3, 2020

Add signal support to addEventListener()

83037a1

Tests: web-platform-tests/wpt#26472. Fixes #911.

benjamingr mentioned this pull request Dec 4, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test addEventListener abort signal #26472

Test addEventListener abort signal #26472

benjamingr commented Nov 10, 2020 •

edited

Loading

benjamingr commented Nov 14, 2020

annevk left a comment •

edited

Loading

annevk Nov 16, 2020

benjamingr Nov 16, 2020

annevk Nov 16, 2020

benjamingr Nov 16, 2020

annevk Nov 16, 2020

benjamingr commented Nov 16, 2020

annevk commented Nov 16, 2020

benjamingr commented Nov 16, 2020

smaug---- commented Nov 16, 2020

benjamingr commented Nov 19, 2020

benjamingr commented Nov 21, 2020

annevk commented Nov 30, 2020

benjamingr commented Nov 30, 2020

annevk commented Dec 1, 2020

benjamingr commented Dec 1, 2020

Test addEventListener abort signal #26472

Test addEventListener abort signal #26472

Conversation

benjamingr commented Nov 10, 2020 • edited Loading

benjamingr commented Nov 14, 2020

annevk left a comment • edited Loading

Choose a reason for hiding this comment

annevk Nov 16, 2020

Choose a reason for hiding this comment

benjamingr Nov 16, 2020

Choose a reason for hiding this comment

annevk Nov 16, 2020

Choose a reason for hiding this comment

benjamingr Nov 16, 2020

Choose a reason for hiding this comment

annevk Nov 16, 2020

Choose a reason for hiding this comment

benjamingr commented Nov 16, 2020

annevk commented Nov 16, 2020

benjamingr commented Nov 16, 2020

smaug---- commented Nov 16, 2020

benjamingr commented Nov 19, 2020

benjamingr commented Nov 21, 2020

annevk commented Nov 30, 2020

benjamingr commented Nov 30, 2020

annevk commented Dec 1, 2020

benjamingr commented Dec 1, 2020

benjamingr commented Nov 10, 2020 •

edited

Loading

annevk left a comment •

edited

Loading