Skip to content
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

Merged
merged 9 commits into from
Aug 18, 2017
7 changes: 3 additions & 4 deletions src/3p-frame-messaging.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,12 @@ export const MessageType = {
* @param {!EventTarget} element
* @param {string} eventType
* @param {function(!Event)} listener
* @param {boolean=} opt_capture
* @param {Object=} opt_evtListenerOpts
* @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);
}


Expand Down Expand Up @@ -133,4 +133,3 @@ export let IframeTransportEvent;
// { transportId: "2", message: "hello" }, // Another
// { transportId: "3", message: "goodbye" } // And another
// ]

109 changes: 82 additions & 27 deletions src/event-helper-listen.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -24,32 +31,80 @@
* @param {!EventTarget} element
* @param {string} eventType
* @param {function(!Event)} listener
* @param {boolean=} opt_capture
* @param {Object=} opt_evtListenerOpts
Copy link
Contributor

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?

Copy link
Contributor Author

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 native addEventListener instead (please confirm). So if we're starting this, maybe we should reinforce using options.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor

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.

* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
}
19 changes: 10 additions & 9 deletions src/event-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @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);
}

/**
Expand All @@ -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 {
Expand All @@ -85,7 +85,7 @@ export function listenOnce(element, eventType, listener, opt_capture) {
localListener = null;
unlisten();
}
}, opt_capture);
}, opt_evtListenerOpts);
return unlisten;
}

Expand All @@ -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) {
Expand Down
57 changes: 57 additions & 0 deletions test/functional/test-event-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ import {
listenOncePromise,
loadPromise,
} from '../../src/event-helper';
import {
detectEvtListenerOptsSupport,
resetEvtListenerOptsSupportForTesting,
} from '../../src/event-helper-listen';
import {Observable} from '../../src/observable';
import * as sinon from 'sinon';

Expand All @@ -38,6 +42,8 @@ describe('EventHelper', () => {
let element;
let loadObservable;
let errorObservable;
let addEventListenerStub;
let removeEventListenerStub;

beforeEach(() => {
sandbox = sinon.sandbox.create();
Expand Down Expand Up @@ -245,4 +251,55 @@ describe('EventHelper', () => {
expect(initCustomEventSpy).to.be.calledOnce;
});

it('should detect when addEventListener options are supported', () => {
const eventListenerStubAcceptOpts = (type, listener, options) => {
const getCapture = options.capture;
if (getCapture) {
// Added to bypass linter (never used warning)
}
};
// Simulate an addEventListener that accepts options
addEventListenerStub =
sandbox.stub(self, 'addEventListener', eventListenerStubAcceptOpts);
// Simulate a removeEventListener that accepts options
removeEventListenerStub =
sandbox.stub(self, 'removeEventListener', eventListenerStubAcceptOpts);
resetEvtListenerOptsSupportForTesting();
expect(detectEvtListenerOptsSupport()).to.be.true;
expect(addEventListenerStub.called).to.be.true;
expect(removeEventListenerStub.called).to.be.true;
resetEvtListenerOptsSupportForTesting();
});

it('should cache the result of the test and only do it once', () => {
resetEvtListenerOptsSupportForTesting();
expect(detectEvtListenerOptsSupport()).to.be.true;
expect(addEventListenerStub.called).to.be.true;
expect(removeEventListenerStub.called).to.be.true;
expect(detectEvtListenerOptsSupport()).to.be.true;
expect(addEventListenerStub.calledOnce).to.be.true;
expect(removeEventListenerStub.calledOnce).to.be.true;
});

it('should detect when addEventListener options are not supported', () => {
const eventListenerStubRejectOpts = (type, listener, capture) => {
const getCapture = capture;
if (getCapture) {
// Added to bypass linter (never used warning)
}
};
// Simulate an addEventListener that does not accept options
addEventListenerStub =
sandbox.stub(self, 'addEventListener', eventListenerStubRejectOpts);
// Simulate a removeEventListener that does not accept options
removeEventListenerStub =
sandbox.stub(self, 'removeEventListener', eventListenerStubRejectOpts);
resetEvtListenerOptsSupportForTesting();
expect(detectEvtListenerOptsSupport()).to.be.false;
expect(addEventListenerStub.called).to.be.true;
expect(removeEventListenerStub.called).to.be.true;
expect(detectEvtListenerOptsSupport()).to.be.false;
expect(removeEventListenerStub.calledOnce).to.be.true;
});

});