Bug 149466 - Implement EventListenerOptions argument to addEventListener
Summary: Implement EventListenerOptions argument to addEventListener
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar, WebExposed
: 158451 (view as bug list)
Depends on: 157882 158451 158453 158465
Blocks: 158601
  Show dependency treegraph
 
Reported: 2015-09-22 09:48 PDT by Simon Fraser (smfr)
Modified: 2016-06-09 20:27 PDT (History)
14 users (show)

See Also:


Attachments
WIP patch (32.00 KB, patch)
2016-06-04 10:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (38.92 KB, patch)
2016-06-05 19:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (38.91 KB, patch)
2016-06-05 19:44 PDT, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (872.72 KB, application/zip)
2016-06-05 20:45 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.01 MB, application/zip)
2016-06-05 20:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (743.71 KB, application/zip)
2016-06-05 20:53 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.49 MB, application/zip)
2016-06-05 20:54 PDT, Build Bot
no flags Details
WIP Patch (38.98 KB, patch)
2016-06-06 10:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (43.22 KB, patch)
2016-06-06 12:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (61.16 KB, patch)
2016-06-06 13:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (61.13 KB, patch)
2016-06-06 13:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.58 KB, patch)
2016-06-06 14:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.49 KB, patch)
2016-06-06 19:32 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.19 KB, patch)
2016-06-07 09:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.26 KB, patch)
2016-06-07 10:02 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2015-09-22 09:48:54 PDT
This is a proposal to allow authors to specify that they'll never preventDefault on events:
http://rbyers.github.io/EventListenerOptions/EventListenerOptions.html

allowing the browser to do various optimizations.
Comment 1 Radar WebKit Bug Importer 2015-09-22 09:49:39 PDT
<rdar://problem/22802031>
Comment 2 Theresa O'Connor 2015-09-22 10:20:33 PDT
I would hold off on this (until|if) it lands in the DOM spec.

https://github.com/whatwg/dom/pull/82
Comment 3 Rick Byers 2016-01-06 08:31:57 PST
FYI, this has now landed in the spec: https://github.com/whatwg/dom/commit/253a21b8e78e37447c47983916a7cf39c4f6a3c5

Explainer here: https://github.com/RByers/EventListenerOptions/blob/gh-pages/explainer.md

We'll probably ship the basic EventListenerOptions API in blink soon, but we've got a lot of work to do to fully implement the performance benefits of the 'passive' option (it's high priority for us though).
Comment 4 David Tapuska 2016-05-18 06:00:59 PDT
Any progress on this? It has been in the DOM spec for a few months now.

FireFox has landed an implementation earlier this month.
Comment 5 Dean Jackson 2016-05-18 17:15:52 PDT
(In reply to comment #4)
> Any progress on this? It has been in the DOM spec for a few months now.
> 
> FireFox has landed an implementation earlier this month.

No progress to report. We'll get to it. Keep bugging us.
Comment 6 Chris Dumez 2016-06-03 19:24:08 PDT
I am working on initial support:
- Bindings support and passing all the options to native code
- Support for the 'once' / 'capture' options

The 'passive' flag will be there on our EventListeners but I am not planning to leverage it in my initial patch. However, Benjamin said he may be interested in updating our touch event handling code to leverage it (for performance).
Comment 7 David Tapuska 2016-06-03 19:29:38 PDT
The main thing that needs to be done for compatibility is to pass the dictionary in as the 3rd arg. I don't know if it makes sense to add the passive field unless you actually support it.  Chromium shipped this way for a few releases. We exposed the dictionary as 3rd argument in 48 I think and then enabled support for the passive field in 51.
Comment 8 Chris Dumez 2016-06-03 20:49:06 PDT
(In reply to comment #7)
> The main thing that needs to be done for compatibility is to pass the
> dictionary in as the 3rd arg. I don't know if it makes sense to add the
> passive field unless you actually support it.  Chromium shipped this way for
> a few releases. We exposed the dictionary as 3rd argument in 48 I think and
> then enabled support for the passive field in 51.

As far as I know, wether the passive field is in the IDL or not is not observable from JS. So I am planning to have the dictionary match the specification. All 3 flags will be passed to our implementation and stored. We anyway will hopefully find good uses for the passive flag in the fairly short term.
Comment 9 David Tapuska 2016-06-03 21:02:39 PDT
Fields are observable due to the resolution of dictionaries to objects. 

See https://github.com/WICG/EventListenerOptions/blob/gh-pages/EventListenerOptions.polyfill.js for how to detect passive is supported
Comment 10 Chris Dumez 2016-06-04 10:06:15 PDT
Created attachment 280515 [details]
WIP patch
Comment 11 WebKit Commit Bot 2016-06-04 10:08:19 PDT
Attachment 280515 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGElement.h:137:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/dom/EventListenerMap.h:58:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Dean Jackson 2016-06-05 13:58:54 PDT
Comment on attachment 280515 [details]
WIP patch

This is cool.
Comment 13 Rick Byers 2016-06-05 17:41:01 PDT
Thanks for working on this feature!

We're actively encouraging feature detection of the passive option (eg. Via Modernizer.passiveeventlisteners), so sites will go down a different code path depending on whether the dictionary member is present.  It shouldn't often matter, but it's possible preventDefault succeeding on passive listeners could cause compat issues.  So I suggest that if you're going to have the passive member you also should implement it's observable behavior (ignoring calls to preventDefault).  That should be trivial, and the perf benefit can always come later.
Comment 14 Chris Dumez 2016-06-05 18:46:10 PDT
(In reply to comment #13)
> Thanks for working on this feature!
> 
> We're actively encouraging feature detection of the passive option (eg. Via
> Modernizer.passiveeventlisteners), so sites will go down a different code
> path depending on whether the dictionary member is present.  It shouldn't
> often matter, but it's possible preventDefault succeeding on passive
> listeners could cause compat issues.  So I suggest that if you're going to
> have the passive member you also should implement it's observable behavior
> (ignoring calls to preventDefault).  That should be trivial, and the perf
> benefit can always come later.

Sure, ignoring calls to preventDefault() if passive is set should be trivial Island can be done from the start.
Comment 15 Chris Dumez 2016-06-05 19:36:04 PDT
Created attachment 280570 [details]
WIP patch
Comment 16 WebKit Commit Bot 2016-06-05 19:38:46 PDT
Attachment 280570 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/EventListenerMap.h:58:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/svg/SVGElement.h:137:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Chris Dumez 2016-06-05 19:44:14 PDT
Created attachment 280571 [details]
WIP patch
Comment 18 Build Bot 2016-06-05 20:45:40 PDT
Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1450022

New failing tests:
fast/workers/dedicated-worker-lifecycle.html
Comment 19 Build Bot 2016-06-05 20:45:44 PDT
Created attachment 280579 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 20 Build Bot 2016-06-05 20:48:03 PDT
Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1450024

New failing tests:
fast/workers/dedicated-worker-lifecycle.html
Comment 21 Build Bot 2016-06-05 20:48:07 PDT
Created attachment 280580 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 22 Build Bot 2016-06-05 20:53:04 PDT
Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1450028

New failing tests:
fast/workers/dedicated-worker-lifecycle.html
Comment 23 Build Bot 2016-06-05 20:53:09 PDT
Created attachment 280581 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 24 Build Bot 2016-06-05 20:54:28 PDT
Comment on attachment 280571 [details]
WIP patch

Attachment 280571 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1450029

New failing tests:
fast/workers/dedicated-worker-lifecycle.html
Comment 25 Build Bot 2016-06-05 20:54:33 PDT
Created attachment 280582 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 26 Simon Fraser (smfr) 2016-06-05 21:16:17 PDT
Chris Dumez did some of this already.
Comment 27 Simon Fraser (smfr) 2016-06-05 21:16:35 PDT
(In reply to comment #26)
> Chris Dumez did some of this already.

Sorry, I was thinking about scrolling options.
Comment 28 Chris Dumez 2016-06-06 10:06:04 PDT
Created attachment 280604 [details]
WIP Patch
Comment 29 Chris Dumez 2016-06-06 12:09:09 PDT
Created attachment 280615 [details]
WIP Patch

Filed https://github.com/whatwg/dom/issues/265 to ask for clarification about a corner case that came up while writing test cases.
Comment 30 Chris Dumez 2016-06-06 13:36:37 PDT
Created attachment 280624 [details]
Patch
Comment 31 Chris Dumez 2016-06-06 13:42:44 PDT
Created attachment 280626 [details]
Patch
Comment 32 Chris Dumez 2016-06-06 14:08:15 PDT
Created attachment 280631 [details]
Patch
Comment 33 WebKit Commit Bot 2016-06-06 15:27:00 PDT
Comment on attachment 280631 [details]
Patch

Clearing flags on attachment: 280631

Committed r201730: <http://trac.webkit.org/changeset/201730>
Comment 34 WebKit Commit Bot 2016-06-06 15:27:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 WebKit Commit Bot 2016-06-06 17:32:29 PDT
Re-opened since this is blocked by bug 158453
Comment 36 Ryan Haddad 2016-06-06 17:37:19 PDT
*** Bug 158451 has been marked as a duplicate of this bug. ***
Comment 37 Chris Dumez 2016-06-06 18:56:34 PDT
Comment on attachment 280631 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280631&action=review

> Source/WebCore/dom/EventTarget.cpp:260
>          RegisteredEventListener& registeredListener = entry[i];

The crashes happened because registeredListener is a reference to a Vector item.
However, when handleEvent() is called below, the JS may unregister event listeners, in which case the vector may get modified and the registeredListener reference gets invalidated.
This used to be fine because registeredListener was not used after calling handleEvent()...

> Source/WebCore/dom/EventTarget.cpp:289
> +        if (registeredListener.isPassive)

... However, I started using registeredListener after calling handleEvent() here...

> Source/WebCore/dom/EventTarget.cpp:292
> +        if (registeredListener.isOnce)

... and here.

This can be fixed by copying the registeredListener instead of using a reference as it is cheap to copy. I'll re-run all the tests in ASAN before re-landing to make sure this was the only issue.
Comment 38 Chris Dumez 2016-06-06 19:32:44 PDT
Created attachment 280662 [details]
Patch
Comment 39 Chris Dumez 2016-06-06 19:34:15 PDT
Comment on attachment 280662 [details]
Patch

Clearing flags on attachment: 280662

Committed r201735: <http://trac.webkit.org/changeset/201735>
Comment 40 Chris Dumez 2016-06-06 19:34:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Alexey Proskuryakov 2016-06-06 22:53:18 PDT
The new test fails on all bots:

@@ -6,9 +6,9 @@
 document.body.addEventListener('test', listenerFunction, { 'once': true })
 PASS listenerCallCount is 0
 document.body.dispatchEvent(new Event('test'))
-PASS listenerCallCount is 1
+FAIL listenerCallCount should be 1. Was 2.
 document.body.dispatchEvent(new Event('test'))
-PASS listenerCallCount is 1
+FAIL listenerCallCount should be 1. Was 2.
 PASS successfullyParsed is true
Comment 42 WebKit Commit Bot 2016-06-06 22:58:51 PDT
Re-opened since this is blocked by bug 158465
Comment 43 Chris Dumez 2016-06-07 07:34:35 PDT
(In reply to comment #41)
> The new test fails on all bots:
> 
> @@ -6,9 +6,9 @@
>  document.body.addEventListener('test', listenerFunction, { 'once': true })
>  PASS listenerCallCount is 0
>  document.body.dispatchEvent(new Event('test'))
> -PASS listenerCallCount is 1
> +FAIL listenerCallCount should be 1. Was 2.
>  document.body.dispatchEvent(new Event('test'))
> -PASS listenerCallCount is 1
> +FAIL listenerCallCount should be 1. Was 2.
>  PASS successfullyParsed is true

Ok, copying the listener is a bad idea as we need to update its ´removed' flag. I will reupload a fixed version this morning.
Comment 44 Chris Dumez 2016-06-07 09:28:26 PDT
Created attachment 280711 [details]
Patch
Comment 45 Chris Dumez 2016-06-07 10:02:54 PDT
Created attachment 280716 [details]
Patch
Comment 46 Chris Dumez 2016-06-07 10:07:17 PDT
Comment on attachment 280716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280716&action=review

Dean, can you please take another look. Here are the changes compared to my previous patch:

> Source/WebCore/dom/EventTarget.cpp:260
> +        RegisteredEventListener registeredListener = entry[i];

Copy the registeredListener instead of keeping a reference because this reference gets invalidated when:
1. we call removeEventListener() below if registeredListener.isOnce is true
2. potentially after calling handleEvent() as the JS can unregister events.

RegisteredEventListener is cheap to copy anyway.

> Source/WebCore/dom/EventTarget.cpp:273
> +        if (registeredListener.isOnce)

No more 'removed' flag on the registeredListener. Instead, we copy the registeredListener above and we remove it from the vector *before* calling handleEvent(). Note that the specification has been updated to do just that:
https://github.com/whatwg/dom/commit/5324200d70eb23276cb58b814416e19c436104f2
Comment 47 Dean Jackson 2016-06-07 10:27:56 PDT
Comment on attachment 280716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280716&action=review

>> Source/WebCore/dom/EventTarget.cpp:260
>> +        RegisteredEventListener registeredListener = entry[i];
> 
> Copy the registeredListener instead of keeping a reference because this reference gets invalidated when:
> 1. we call removeEventListener() below if registeredListener.isOnce is true
> 2. potentially after calling handleEvent() as the JS can unregister events.
> 
> RegisteredEventListener is cheap to copy anyway.

OK.

>> Source/WebCore/dom/EventTarget.cpp:273
>> +        if (registeredListener.isOnce)
> 
> No more 'removed' flag on the registeredListener. Instead, we copy the registeredListener above and we remove it from the vector *before* calling handleEvent(). Note that the specification has been updated to do just that:
> https://github.com/whatwg/dom/commit/5324200d70eb23276cb58b814416e19c436104f2

Nice.
Comment 48 WebKit Commit Bot 2016-06-07 10:51:49 PDT
Comment on attachment 280716 [details]
Patch

Clearing flags on attachment: 280716

Committed r201757: <http://trac.webkit.org/changeset/201757>
Comment 49 WebKit Commit Bot 2016-06-07 10:52:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 50 Rick Byers 2016-06-08 10:13:43 PDT
Thanks for importing the WPT and blink tests for this!
FYI I've (finally) landed my WPT test for passive: https://github.com/w3c/web-platform-tests/pull/3119 (sorry I didn't do that sooner), and I'll be deleting our blink LayoutTests tests for the API since they're now redundant with the WPT tests.

We haven't implemented 'once' yet, but I'm happy to review/merge any web-platform-test pull requests for it.
Comment 51 Chris Dumez 2016-06-08 10:15:21 PDT
(In reply to comment #50)
> Thanks for importing the WPT and blink tests for this!
> FYI I've (finally) landed my WPT test for passive:
> https://github.com/w3c/web-platform-tests/pull/3119 (sorry I didn't do that
> sooner), and I'll be deleting our blink LayoutTests tests for the API since
> they're now redundant with the WPT tests.
> 
> We haven't implemented 'once' yet, but I'm happy to review/merge any
> web-platform-test pull requests for it.

Thanks for letting us know. I'll import the new WPT test.