-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Passive event listener wrapper #10721
Changes from all commits
8887ffd
0b1abb6
2128257
43cb170
851bcd0
72c9882
c577e05
c3f5ee3
c7dee6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,13 @@ | |
* limitations under the License. | ||
*/ | ||
|
||
/** | ||
* Whether addEventListener supports options or only takes capture as a boolean | ||
* @type {boolean|undefined} | ||
* @visibleForTesting | ||
*/ | ||
let optsSupported; | ||
|
||
/** | ||
* Listens for the specified event on the element. | ||
* | ||
|
@@ -24,32 +31,80 @@ | |
* @param {!EventTarget} element | ||
* @param {string} eventType | ||
* @param {function(!Event)} listener | ||
* @param {boolean=} opt_capture | ||
* @param {Object=} opt_evtListenerOpts | ||
* @return {!UnlistenDef} | ||
*/ | ||
export function internalListenImplementation(element, eventType, listener, | ||
opt_capture) { | ||
let localElement = element; | ||
let localListener = listener; | ||
/** @type {?Function} */ | ||
let wrapped = event => { | ||
try { | ||
return localListener(event); | ||
} catch (e) { | ||
// reportError is installed globally per window in the entry point. | ||
self.reportError(e); | ||
throw e; | ||
} | ||
}; | ||
const capture = opt_capture || false; | ||
localElement.addEventListener(eventType, wrapped, capture); | ||
return () => { | ||
if (localElement) { | ||
localElement.removeEventListener(eventType, wrapped, capture); | ||
} | ||
// Ensure these are GC'd | ||
localListener = null; | ||
localElement = null; | ||
wrapped = null; | ||
}; | ||
} | ||
export function internalListenImplementation(element, eventType, listener, | ||
opt_evtListenerOpts) { | ||
let localElement = element; | ||
let localListener = listener; | ||
/** @type {?Function} */ | ||
let wrapped = event => { | ||
try { | ||
return localListener(event); | ||
} catch (e) { | ||
// reportError is installed globally per window in the entry point. | ||
self.reportError(e); | ||
throw e; | ||
} | ||
}; | ||
const optsSupported = detectEvtListenerOptsSupport(); | ||
let capture = false; | ||
if (opt_evtListenerOpts) { | ||
capture = opt_evtListenerOpts.capture; | ||
} | ||
localElement.addEventListener( | ||
eventType, | ||
wrapped, | ||
optsSupported ? opt_evtListenerOpts : capture | ||
); | ||
return () => { | ||
if (localElement) { | ||
localElement.removeEventListener( | ||
eventType, | ||
wrapped, | ||
optsSupported ? opt_evtListenerOpts : capture | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did it based on
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
); | ||
} | ||
// Ensure these are GC'd | ||
localListener = null; | ||
localElement = null; | ||
wrapped = null; | ||
}; | ||
} | ||
|
||
/** | ||
* Tests whether the browser supports options as an argument of addEventListener | ||
* or not. | ||
* | ||
* @return {boolean} | ||
* @suppress {checkTypes} | ||
*/ | ||
export function detectEvtListenerOptsSupport() { | ||
// Only run the test once | ||
if (optsSupported !== undefined) { | ||
return optsSupported; | ||
} | ||
|
||
optsSupported = false; | ||
try { | ||
// Test whether browser supports EventListenerOptions or not | ||
const options = { | ||
get capture() { | ||
optsSupported = true; | ||
}, | ||
}; | ||
self.addEventListener('test-options', null, options); | ||
self.removeEventListener('test-options', null, options); | ||
} catch (err) { | ||
// EventListenerOptions are not supported | ||
} | ||
return optsSupported; | ||
} | ||
|
||
/** | ||
* Resets the test for whether addEventListener supports options or not. | ||
*/ | ||
export function resetEvtListenerOptsSupportForTesting() { | ||
optsSupported = undefined; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be |
||
* @return {!UnlistenDef} | ||
*/ | ||
export function listen(element, eventType, listener, opt_capture) { | ||
export function listen(element, eventType, listener, opt_evtListenerOpts) { | ||
return internalListenImplementation( | ||
element, eventType, listener, opt_capture); | ||
element, eventType, listener, opt_evtListenerOpts); | ||
} | ||
|
||
/** | ||
|
@@ -72,10 +72,10 @@ export function getData(event) { | |
* @param {!EventTarget} element | ||
* @param {string} eventType | ||
* @param {function(!Event)} listener | ||
* @param {boolean=} opt_capture | ||
* @param {Object=} opt_evtListenerOpts | ||
* @return {!UnlistenDef} | ||
*/ | ||
export function listenOnce(element, eventType, listener, opt_capture) { | ||
export function listenOnce(element, eventType, listener, opt_evtListenerOpts) { | ||
let localListener = listener; | ||
const unlisten = internalListenImplementation(element, eventType, event => { | ||
try { | ||
|
@@ -85,7 +85,7 @@ export function listenOnce(element, eventType, listener, opt_capture) { | |
localListener = null; | ||
unlisten(); | ||
} | ||
}, opt_capture); | ||
}, opt_evtListenerOpts); | ||
return unlisten; | ||
} | ||
|
||
|
@@ -95,16 +95,17 @@ export function listenOnce(element, eventType, listener, opt_capture) { | |
* fired on the element. | ||
* @param {!EventTarget} element | ||
* @param {string} eventType | ||
* @param {boolean=} opt_capture | ||
* @param {Object=} opt_evtListenerOpts | ||
* @param {function(!UnlistenDef)=} opt_cancel An optional function that, when | ||
* provided, will be called with the unlistener. This gives the caller | ||
* access to the unlistener, so it may be called manually when necessary. | ||
* @return {!Promise<!Event>} | ||
*/ | ||
export function listenOncePromise(element, eventType, opt_capture, opt_cancel) { | ||
export function listenOncePromise(element, eventType, opt_evtListenerOpts, | ||
opt_cancel) { | ||
let unlisten; | ||
const eventPromise = new Promise(resolve => { | ||
unlisten = listenOnce(element, eventType, resolve, opt_capture); | ||
unlisten = listenOnce(element, eventType, resolve, opt_evtListenerOpts); | ||
}); | ||
eventPromise.then(unlisten, unlisten); | ||
if (opt_cancel) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm: do you want to update all callers to only use options struct? Or will you continue to allow
capture
as boolean?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find any callers that use
capture
actually, most use the nativeaddEventListener
instead (please confirm). So if we're starting this, maybe we should reinforce using options.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. If the case, let's definitely go with options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aghassemi could you please confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it compiles it is is cool. That is what they type checker is for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find any usages either and yeah type check should catch them anyway.