Visitar URL original
fix(zone.js): preserve accessor and non-enumerable event listener opt… · angular/angular@4c4a705 · GitHub
Skip to content

Commit 4c4a705

Browse files
stefanwaldhauserJeanMeche
authored and
mattrbeck
committed
fix(zone.js): preserve accessor and non-enumerable event listener options
`copyEventListenerOptions` copied the caller's options with `{...options}` before forwarding to the native `addEventListener`. Object spread only copies own enumerable data properties, whereas the native call reads each dictionary member via WebIDL — a plain `[[Get]]` per member, which invokes accessors and ignores enumerability. The copy was therefore lossy in a way the native call is not: - `Object.defineProperty(opts, 'passive', { get })` (the shape used by MDN's passive-listener feature test) — the getter was never invoked, so libraries that use the feature test fall back to the legacy boolean and register every listener as non-passive. - `Object.defineProperty(opts, 'capture', { get: () => true })` — the listener was silently registered on the bubbling phase. - `Object.defineProperty(opts, 'once', { get: () => true })` — the listener fired on every dispatch. `signal` was already special-cased for `AbortController.prototype.signal` after #54142; that patch generalises the workaround to every recognised member. The copy itself was the correct fix for #54142 (frozen/readonly options) and is preserved. The fix reads each recognised member from the source via `[[Get]]` when the spread did not, which recovers accessors and non-enumerable properties without double-invoking any getter. The list of recognised members is hoisted to module scope so it isn't allocated on every `patchEventTarget` invocation. The call site is reordered to `buildEventListenerOptions( copyEventListenerOptions(...))` so the passive-events code path also spreads a normalised data object rather than the caller's raw input. Fixes #70431 Co-authored-by: Matthieu Riegler <kyro38@gmail.com>
1 parent 0904f90 commit 4c4a705

2 files changed

Lines changed: 89 additions & 21 deletions

File tree

‎packages/zone.js/lib/common/events.ts‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,11 @@ export const globalSources: any = {};
9292
const EVENT_NAME_SYMBOL_REGX = new RegExp('^' + ZONE_SYMBOL_PREFIX + '(\\w+)(true|false)$');
9393
const IMMEDIATE_PROPAGATION_SYMBOL = zoneSymbol('propagationStopped');
9494

95+
// Recognised members of the `AddEventListenerOptions` WebIDL dictionary, used
96+
// by `copyEventListenerOptions` to recover accessors and non-enumerable
97+
// properties that a caller-supplied options object may expose.
98+
const KNOWN_EVENT_LISTENER_OPTIONS = ['capture', 'once', 'passive', 'signal'];
99+
95100
function prepareEventNames(eventName: string, eventNameToString?: (eventName: string) => string) {
96101
const falseEventName = (eventNameToString ? eventNameToString(eventName) : eventName) + FALSE_STR;
97102
const trueEventName = (eventNameToString ? eventNameToString(eventName) : eventName) + TRUE_STR;
@@ -381,7 +386,8 @@ export function patchEventTarget(
381386
return {passive: true};
382387
}
383388
if (typeof options === 'object' && options.passive !== false) {
384-
return {...options, passive: true};
389+
options.passive = true;
390+
return options;
385391
}
386392
return options;
387393
}
@@ -492,27 +498,22 @@ export function patchEventTarget(
492498
const passiveEvents: string[] = _global[zoneSymbol('PASSIVE_EVENTS')];
493499

494500
function copyEventListenerOptions(options: any) {
495-
if (typeof options === 'object' && options !== null) {
496-
// We need to destructure the target `options` object since it may
497-
// be frozen or sealed (possibly provided implicitly by a third-party
498-
// library), or its properties may be readonly.
499-
const newOptions: any = {...options};
500-
// The `signal` option was recently introduced, which caused regressions in
501-
// third-party scenarios where `AbortController` was directly provided to
502-
// `addEventListener` as options. For instance, in cases like
503-
// `document.addEventListener('keydown', callback, abortControllerInstance)`,
504-
// which is valid because `AbortController` includes a `signal` getter, spreading
505-
// `{...options}` wouldn't copy the `signal`. Additionally, using `Object.create`
506-
// isn't feasible since `AbortController` is a built-in object type, and attempting
507-
// to create a new object directly with it as the prototype might result in
508-
// unexpected behavior.
509-
if (options.signal) {
510-
newOptions.signal = options.signal;
501+
if (typeof options !== 'object' || options === null) {
502+
return options;
503+
}
504+
// Spread copies own enumerable properties, invoking any getters exactly once.
505+
const newOptions: any = {...options};
506+
// Anything the spread could not see (inherited accessors such as
507+
// `AbortController.prototype.signal`, or non-enumerable properties defined
508+
// via `Object.defineProperty`) is read directly from the source, exactly once. Reading
509+
// from `options` rather than `newOptions` also gives prototype getters the
510+
// correct receiver.
511+
for (const key of KNOWN_EVENT_LISTENER_OPTIONS) {
512+
if (!Object.hasOwn(newOptions, key) && key in options) {
513+
newOptions[key] = options[key];
511514
}
512-
return newOptions;
513515
}
514-
515-
return options;
516+
return newOptions;
516517
}
517518

518519
const makeAddListener = function (
@@ -556,7 +557,7 @@ export function patchEventTarget(
556557
}
557558

558559
const passive = !!passiveEvents && passiveEvents.indexOf(eventName) !== -1;
559-
const options = copyEventListenerOptions(buildEventListenerOptions(arguments[2], passive));
560+
const options = buildEventListenerOptions(copyEventListenerOptions(arguments[2]), passive);
560561
const signal: AbortSignal | undefined = options?.signal;
561562
if (signal?.aborted) {
562563
// the signal is an aborted one, just return without attaching the event listener.

‎packages/zone.js/test/browser/browser.spec.ts‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2184,6 +2184,73 @@ describe('Zone Browser', function () {
21842184
expect(logs).toEqual(['click2']);
21852185
});
21862186

2187+
// Options exposed as accessors or non-enumerable properties must still
2188+
// reach the native call. https://github.com/angular/angular/issues/54142
2189+
describe('event listener options built with accessors', function () {
2190+
it('should invoke a non-enumerable `passive` getter (MDN feature-detect pattern)', function () {
2191+
let getCount = 0;
2192+
const opts = Object.defineProperty({}, 'passive', {
2193+
get: () => {
2194+
getCount++;
2195+
return false;
2196+
},
2197+
});
2198+
const listener = () => {};
2199+
button.addEventListener('click', listener, opts as any);
2200+
expect(getCount).toBe(1);
2201+
button.removeEventListener('click', listener, opts as any);
2202+
});
2203+
2204+
it('should honour `capture: true` supplied as an accessor', function () {
2205+
const opts = Object.defineProperty({}, 'capture', {get: () => true});
2206+
const inner = document.createElement('span');
2207+
button.appendChild(inner);
2208+
let phase = -1;
2209+
const listener = (e: Event) => {
2210+
phase = e.eventPhase;
2211+
};
2212+
button.addEventListener('click', listener, opts as any);
2213+
inner.dispatchEvent(clickEvent);
2214+
expect(phase).toBe(Event.CAPTURING_PHASE);
2215+
2216+
button.removeEventListener('click', listener, opts as any);
2217+
phase = -1;
2218+
inner.dispatchEvent(clickEvent);
2219+
expect(phase).toBe(-1);
2220+
button.removeChild(inner);
2221+
});
2222+
2223+
it('should honour `once: true` supplied as an accessor', function () {
2224+
const opts = Object.defineProperty({}, 'once', {get: () => true});
2225+
let callCount = 0;
2226+
button.addEventListener('click', () => callCount++, opts as any);
2227+
button.dispatchEvent(clickEvent);
2228+
button.dispatchEvent(clickEvent);
2229+
expect(callCount).toBe(1);
2230+
});
2231+
2232+
// `AbortController.prototype.signal` is a prototype accessor, so an
2233+
// own-properties-only copy would drop it.
2234+
it('should honour `signal` on an AbortController passed as options', function () {
2235+
const ac = new AbortController();
2236+
const logs: string[] = [];
2237+
button.addEventListener('click', () => logs.push('click'), ac);
2238+
button.dispatchEvent(clickEvent);
2239+
ac.abort();
2240+
button.dispatchEvent(clickEvent);
2241+
expect(logs).toEqual(['click']);
2242+
expect(button.eventListeners!('click').length).toBe(0);
2243+
});
2244+
2245+
// https://github.com/angular/angular/pull/55796
2246+
it('should accept a frozen options object', function () {
2247+
const opts = Object.freeze({capture: true, once: true});
2248+
const listener = () => {};
2249+
expect(() => button.addEventListener('click', listener, opts as any)).not.toThrow();
2250+
button.removeEventListener('click', listener, opts as any);
2251+
});
2252+
});
2253+
21872254
// https://github.com/angular/angular/issues/56148
21882255
it('should store the remove abort listener on the task itself and not the task data', function () {
21892256
const logs: string[] = [];

0 commit comments

Comments
 (0)