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.
<rdar://problem/22802031>
I would hold off on this (until|if) it lands in the DOM spec. https://github.com/whatwg/dom/pull/82
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).
Any progress on this? It has been in the DOM spec for a few months now. FireFox has landed an implementation earlier this month.
(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.
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).
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.
(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.
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
Created attachment 280515 [details] WIP patch
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 on attachment 280515 [details] WIP patch This is cool.
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.
(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.
Created attachment 280570 [details] WIP patch
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.
Created attachment 280571 [details] WIP patch
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
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 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
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 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
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 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
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
Chris Dumez did some of this already.
(In reply to comment #26) > Chris Dumez did some of this already. Sorry, I was thinking about scrolling options.
Created attachment 280604 [details] WIP Patch
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.
Created attachment 280624 [details] Patch
Created attachment 280626 [details] Patch
Created attachment 280631 [details] Patch
Comment on attachment 280631 [details] Patch Clearing flags on attachment: 280631 Committed r201730: <http://trac.webkit.org/changeset/201730>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 158453
*** Bug 158451 has been marked as a duplicate of this bug. ***
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.
Created attachment 280662 [details] Patch
Comment on attachment 280662 [details] Patch Clearing flags on attachment: 280662 Committed r201735: <http://trac.webkit.org/changeset/201735>
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
Re-opened since this is blocked by bug 158465
(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.
Created attachment 280711 [details] Patch
Created attachment 280716 [details] Patch
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 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 on attachment 280716 [details] Patch Clearing flags on attachment: 280716 Committed r201757: <http://trac.webkit.org/changeset/201757>
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.
(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.