Passive event listener wrapper #10721

wassgha · 2017-07-31T08:08:06Z

Changes

Added passive option to the event listener to make use of the new addEventListener interface.
Possible use case includes preventing scrolling when dragging

Re-opened #5105 to remove @suppress typechecks, and change Object to AddEventListenerOptions when the Closure Compiler extern is updated

Closes #4819 , Relates to #5109

wassgha · 2017-07-31T19:21:40Z

@erwinmombay is updating the type-checker to support passive events for this to pass

erwinmombay · 2017-08-03T22:58:35Z

i am close to updating. theres a dozen of broken types now though that i need to fix

aghassemi · 2017-08-10T16:25:59Z

src/event-helper.js

 * @return {!UnlistenDef}
 */
-export function listenOnce(element, eventType, listener, opt_capture) {
+export function listenOnce(element, eventType, listener,


could you add to listenPromise* as well, for completeness

aghassemi · 2017-08-10T16:26:32Z

src/event-helper-listen.js

+
+  // Test whether browser supports the passive option or not
+  let passiveSupported = false;
+  try {


couple of unit tests to make sure detection is works as browser's implementation may change. Ideally tests call the top-level exported listen, mock the addEventListener for test-passive to either support or not support and then spy on addEventListener and removeEventListener to make sure the third arg is correct

cramforce · 2017-08-11T00:32:26Z

src/event-helper.js

@@ -50,11 +50,12 @@ export function createCustomEvent(win, type, detail, opt_eventInit) {
 * @param {string} eventType
 * @param {function(!Event)} listener
 * @param {boolean=} opt_capture


Any reason not to make this
@param {boolean|EventListenerOptions} opt_catureOrOptions

Since it's our implementation and we already implement once through listenOncePromise, I thought there's really no point from passing an object when the only two options are capture and passive, but if you think it should be that way then sure!

I don't want to add more arguments when more options are added.

cramforce · 2017-08-11T00:33:40Z

src/event-helper-listen.js

@@ -41,11 +43,34 @@ export function internalListenImplementation(element, eventType, listener,
      throw e;
    }
  };
+
+  // Test whether browser supports the passive option or not
+  let passiveSupported = false;


Cache this value in a global and only run the feature detection code once per window.

cramforce · 2017-08-11T00:34:53Z

src/event-helper-listen.js

+  // Test whether browser supports the passive option or not
+  let passiveSupported = false;
+  try {
+    const options = Object.defineProperty({}, 'passive', {


The polyfill is more general if you ask for capture instead. That way this code doesn't have to change for further options. If people really need this test, they can export a function for feature detection.

…n possible. Needs ampproject#10721 to land first.

cramforce · 2017-08-13T21:57:57Z

Usage in #10901

wassgha · 2017-08-14T18:03:28Z

Ready for final review, will need to revert back to using AddEventListenerOptions instead of Object when it is recognized as a valid type.
I have not found any instances where we currently use the capture parameter, please double check for side effects.

cramforce · 2017-08-14T18:22:46Z

src/event-helper.js

@@ -49,12 +49,12 @@ export function createCustomEvent(win, type, detail, opt_eventInit) {
 * @param {!EventTarget} element
 * @param {string} eventType
 * @param {function(!Event)} listener
- * @param {boolean=} opt_capture
+ * @param {Object=} opt_evtListenerOpts


Should this be boolean|Object=? If not, you need to change all callsites.

jridgewell · 2017-08-14T19:06:46Z

src/event-helper-listen.js

+ * addEventListener or not
+ * @enum {string}
+ */
+ const optTest = {


We've used undefined, true, and false elsewhere in the project.

jridgewell · 2017-08-14T19:08:17Z

src/event-helper-listen.js

+   };
+   const optsSupported = detectEvtListenerOptsSupport();
+   let capture = false;
+   if (opt_evtListenerOpts && opt_evtListenerOpts.capture) {


It's unnecessary to check for opt_evtListenerOpts.capture

jridgewell · 2017-08-14T19:11:11Z

src/event-helper-listen.js

+       localElement.removeEventListener(
+           eventType,
+           wrapped,
+           optsSupported ? opt_evtListenerOpts : capture


Only capture matters for #removeEventListener per https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener#Matching_event_listeners_for_removal.

Did it based on

It's worth noting that some browser releases have been inconsistent on this, and unless you have specific reasons otherwise, it's probably wise to use the same values used for the call to addEventListener() when calling removeEventListener().

jridgewell · 2017-08-14T19:12:35Z

src/event-helper-listen.js

+   try {
+     let optsSupported = self.optsSupported;
+     const options = Object.defineProperty({}, 'capture', {
+       get: function() {


Just use a object with getter:

const options = { get capture() { optsSupported = true; }, }

jridgewell · 2017-08-14T19:13:04Z

src/event-helper-listen.js

+         optsSupported = optTest.SUPPORTED;
+       },
+     });
+     self.addEventListener('test-opts', null, options);


This needs to be cleaned up.

jridgewell · 2017-08-14T19:14:19Z

src/event-helper-listen.js

+ * @suppress {checkTypes}
+ */
+ export function detectEvtListenerOptsSupport() {
+   if (!self.optsSupported) {


Setting this on self is unnecessary, just use a module level variable. See https://github.com/ampproject/amphtml/blob/master/src/dom.js#L375-L400.

dvoytenko · 2017-08-14T22:58:49Z

src/event-helper-listen.js

@@ -24,32 +31,79 @@
 * @param {!EventTarget} element
 * @param {string} eventType
 * @param {function(!Event)} listener
- * @param {boolean=} opt_capture
+ * @param {Object=} opt_evtListenerOpts


Just to confirm: do you want to update all callers to only use options struct? Or will you continue to allow capture as boolean?

I could not find any callers that use capture actually, most use the native addEventListener instead (please confirm). So if we're starting this, maybe we should reinforce using options.

Cool. If the case, let's definitely go with options.

@aghassemi could you please confirm?

If it compiles it is is cool. That is what they type checker is for.

I can't find any usages either and yeah type check should catch them anyway.

aghassemi

one request

aghassemi · 2017-08-16T16:06:17Z

src/event-helper-listen.js

+ /**
+  * Resets the test for whether addEventListener supports options or not.
+  */
+ export function resetEvtListenerOptsSupport() {


add ForTesting suffix to the name

… checks

jridgewell

Nits.

jridgewell · 2017-08-14T22:12:00Z

src/event-helper-listen.js

   // Only run the test once
-   if (self.optsSupported != optTest.NOT_RUN) {
-     return self.optsSupported == optTest.SUPPORTED;
+   if (typeof optsSupported != 'undefined') {


You can just check for === undefined.

jridgewell · 2017-08-16T16:51:04Z

src/event-helper-listen.js

+ */
+ export function detectEvtListenerOptsSupport() {
+   // Only run the test once
+   if (typeof optsSupported != 'undefined') {


optsSupported !== undefined

jridgewell · 2017-08-16T16:51:20Z

src/event-helper-listen.js

+         optsSupported = true;
+       },
+     };
+     self.addEventListener('test-opts', null, options);


This needs to be cleaned up.

Cleaned up as in?

#removeEventListener

aghassemi · 2017-08-17T17:12:19Z

test failure and we can merge after

wassgha · 2017-08-17T21:48:49Z

For future reference, the test was failing because removeEventListener on the test was calling the getter of capture in options which was setting optsSupported to true even though addEventListener was stubbed to not accept options. Had to stub removeEventListener too.

aghassemi · 2017-08-17T23:41:55Z

@cramforce Blocked on your review, PTAL

…n possible. Needs #10721 to land first.

…n possible. Needs ampproject#10721 to land first.

googlebot added the cla: yes label Jul 31, 2017

wassgha requested a review from aghassemi July 31, 2017 08:08

wassgha force-pushed the video-dock-ui branch 6 times, most recently from 65843c4 to 0d3bb38 Compare July 31, 2017 18:20

wassgha force-pushed the video-dock-ui branch 3 times, most recently from b90a2d6 to 1440784 Compare August 1, 2017 00:48

wassgha force-pushed the video-dock-ui branch from 1440784 to 150939d Compare August 9, 2017 23:51

wassgha changed the title ~~Video Docking Minimized Controls~~ Passive event listener wrapper Aug 9, 2017

wassgha requested a review from dvoytenko August 10, 2017 00:58

wassgha mentioned this pull request Aug 10, 2017

Custom video controls (phase 1), minimized video controls and video docking bug fixes #10865

Closed

aghassemi suggested changes Aug 10, 2017

View reviewed changes

cramforce requested changes Aug 11, 2017

View reviewed changes

wassgha force-pushed the video-dock-ui branch from 2877299 to cb1b7b4 Compare August 13, 2017 06:15

cramforce added a commit to cramforce/amphtml that referenced this pull request Aug 13, 2017

Use a passive event handlers for touch events in viewer messaging whe…

61d4278

…n possible. Needs ampproject#10721 to land first.

cramforce mentioned this pull request Aug 13, 2017

Use a passive event handlers for touch events in viewer messaging when possible. #10901

Merged

cramforce reviewed Aug 14, 2017

View reviewed changes

jridgewell requested changes Aug 14, 2017

View reviewed changes

dvoytenko reviewed Aug 14, 2017

View reviewed changes

aghassemi approved these changes Aug 16, 2017

View reviewed changes

wassgha added 2 commits August 16, 2017 09:20

Implemented minimized controls

8887ffd

Implemented minified controls

0b1abb6

wassgha added 6 commits August 16, 2017 09:20

Removed minimized controls part and only left passive event listener

2128257

Ignore type checking until closure compiler is updated

43cb170

Switched to options object instead of boolean args

851bcd0

Changed instances of AddEventListenerOptions to Object to bypass type…

72c9882

… checks

Caching per window, Added tests

c577e05

Requested changes

c3f5ee3

wassgha force-pushed the video-dock-ui branch from b7b1402 to 2202a70 Compare August 16, 2017 16:20

jridgewell approved these changes Aug 16, 2017

View reviewed changes

wassgha force-pushed the video-dock-ui branch 2 times, most recently from a84a879 to a280211 Compare August 17, 2017 04:24

wassgha added this to the Sprint H2 August milestone Aug 17, 2017

wassgha force-pushed the video-dock-ui branch from a280211 to 71aacff Compare August 17, 2017 21:46

Requested change

c7dee6c

wassgha force-pushed the video-dock-ui branch from 71aacff to c7dee6c Compare August 17, 2017 22:03

cramforce approved these changes Aug 17, 2017

View reviewed changes

aghassemi merged commit 1ef6ae4 into ampproject:master Aug 18, 2017

cramforce added a commit that referenced this pull request Aug 31, 2017

Use a passive event handlers for touch events in viewer messaging whe…

542c444

…n possible. Needs #10721 to land first.

cramforce added a commit to cramforce/amphtml that referenced this pull request Sep 1, 2017

Use a passive event handlers for touch events in viewer messaging whe…

3948586

…n possible. Needs ampproject#10721 to land first.

connorjclark mentioned this pull request Oct 30, 2019

AMP and Best Practices GoogleChrome/lighthouse#2819

Closed

Passive event listener wrapper #10721

Passive event listener wrapper #10721

Conversation

wassgha commented Jul 31, 2017 • edited Loading

Changes

wassgha commented Jul 31, 2017

erwinmombay commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramforce commented Aug 13, 2017

wassgha commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Aug 17, 2017

wassgha commented Aug 17, 2017

aghassemi commented Aug 17, 2017

wassgha commented Jul 31, 2017 •

edited

Loading