mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
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>
(cherry picked from commit 4c4a705ce3)
This commit is contained in:
@@ -92,6 +92,11 @@ export const globalSources: any = {};
|
||||
const EVENT_NAME_SYMBOL_REGX = new RegExp('^' + ZONE_SYMBOL_PREFIX + '(\\w+)(true|false)$');
|
||||
const IMMEDIATE_PROPAGATION_SYMBOL = zoneSymbol('propagationStopped');
|
||||
|
||||
// Recognised members of the `AddEventListenerOptions` WebIDL dictionary, used
|
||||
// by `copyEventListenerOptions` to recover accessors and non-enumerable
|
||||
// properties that a caller-supplied options object may expose.
|
||||
const KNOWN_EVENT_LISTENER_OPTIONS = ['capture', 'once', 'passive', 'signal'];
|
||||
|
||||
function prepareEventNames(eventName: string, eventNameToString?: (eventName: string) => string) {
|
||||
const falseEventName = (eventNameToString ? eventNameToString(eventName) : eventName) + FALSE_STR;
|
||||
const trueEventName = (eventNameToString ? eventNameToString(eventName) : eventName) + TRUE_STR;
|
||||
@@ -381,7 +386,8 @@ export function patchEventTarget(
|
||||
return {passive: true};
|
||||
}
|
||||
if (typeof options === 'object' && options.passive !== false) {
|
||||
return {...options, passive: true};
|
||||
options.passive = true;
|
||||
return options;
|
||||
}
|
||||
return options;
|
||||
}
|
||||
@@ -492,27 +498,22 @@ export function patchEventTarget(
|
||||
const passiveEvents: string[] = _global[zoneSymbol('PASSIVE_EVENTS')];
|
||||
|
||||
function copyEventListenerOptions(options: any) {
|
||||
if (typeof options === 'object' && options !== null) {
|
||||
// We need to destructure the target `options` object since it may
|
||||
// be frozen or sealed (possibly provided implicitly by a third-party
|
||||
// library), or its properties may be readonly.
|
||||
const newOptions: any = {...options};
|
||||
// The `signal` option was recently introduced, which caused regressions in
|
||||
// third-party scenarios where `AbortController` was directly provided to
|
||||
// `addEventListener` as options. For instance, in cases like
|
||||
// `document.addEventListener('keydown', callback, abortControllerInstance)`,
|
||||
// which is valid because `AbortController` includes a `signal` getter, spreading
|
||||
// `{...options}` wouldn't copy the `signal`. Additionally, using `Object.create`
|
||||
// isn't feasible since `AbortController` is a built-in object type, and attempting
|
||||
// to create a new object directly with it as the prototype might result in
|
||||
// unexpected behavior.
|
||||
if (options.signal) {
|
||||
newOptions.signal = options.signal;
|
||||
}
|
||||
return newOptions;
|
||||
if (typeof options !== 'object' || options === null) {
|
||||
return options;
|
||||
}
|
||||
|
||||
return options;
|
||||
// Spread copies own enumerable properties, invoking any getters exactly once.
|
||||
const newOptions: any = {...options};
|
||||
// Anything the spread could not see (inherited accessors such as
|
||||
// `AbortController.prototype.signal`, or non-enumerable properties defined
|
||||
// via `Object.defineProperty`) is read directly from the source, exactly once. Reading
|
||||
// from `options` rather than `newOptions` also gives prototype getters the
|
||||
// correct receiver.
|
||||
for (const key of KNOWN_EVENT_LISTENER_OPTIONS) {
|
||||
if (!Object.hasOwn(newOptions, key) && key in options) {
|
||||
newOptions[key] = options[key];
|
||||
}
|
||||
}
|
||||
return newOptions;
|
||||
}
|
||||
|
||||
const makeAddListener = function (
|
||||
@@ -556,7 +557,7 @@ export function patchEventTarget(
|
||||
}
|
||||
|
||||
const passive = !!passiveEvents && passiveEvents.indexOf(eventName) !== -1;
|
||||
const options = copyEventListenerOptions(buildEventListenerOptions(arguments[2], passive));
|
||||
const options = buildEventListenerOptions(copyEventListenerOptions(arguments[2]), passive);
|
||||
const signal: AbortSignal | undefined = options?.signal;
|
||||
if (signal?.aborted) {
|
||||
// the signal is an aborted one, just return without attaching the event listener.
|
||||
|
||||
@@ -2184,6 +2184,73 @@ describe('Zone Browser', function () {
|
||||
expect(logs).toEqual(['click2']);
|
||||
});
|
||||
|
||||
// Options exposed as accessors or non-enumerable properties must still
|
||||
// reach the native call. https://github.com/angular/angular/issues/54142
|
||||
describe('event listener options built with accessors', function () {
|
||||
it('should invoke a non-enumerable `passive` getter (MDN feature-detect pattern)', function () {
|
||||
let getCount = 0;
|
||||
const opts = Object.defineProperty({}, 'passive', {
|
||||
get: () => {
|
||||
getCount++;
|
||||
return false;
|
||||
},
|
||||
});
|
||||
const listener = () => {};
|
||||
button.addEventListener('click', listener, opts as any);
|
||||
expect(getCount).toBe(1);
|
||||
button.removeEventListener('click', listener, opts as any);
|
||||
});
|
||||
|
||||
it('should honour `capture: true` supplied as an accessor', function () {
|
||||
const opts = Object.defineProperty({}, 'capture', {get: () => true});
|
||||
const inner = document.createElement('span');
|
||||
button.appendChild(inner);
|
||||
let phase = -1;
|
||||
const listener = (e: Event) => {
|
||||
phase = e.eventPhase;
|
||||
};
|
||||
button.addEventListener('click', listener, opts as any);
|
||||
inner.dispatchEvent(clickEvent);
|
||||
expect(phase).toBe(Event.CAPTURING_PHASE);
|
||||
|
||||
button.removeEventListener('click', listener, opts as any);
|
||||
phase = -1;
|
||||
inner.dispatchEvent(clickEvent);
|
||||
expect(phase).toBe(-1);
|
||||
button.removeChild(inner);
|
||||
});
|
||||
|
||||
it('should honour `once: true` supplied as an accessor', function () {
|
||||
const opts = Object.defineProperty({}, 'once', {get: () => true});
|
||||
let callCount = 0;
|
||||
button.addEventListener('click', () => callCount++, opts as any);
|
||||
button.dispatchEvent(clickEvent);
|
||||
button.dispatchEvent(clickEvent);
|
||||
expect(callCount).toBe(1);
|
||||
});
|
||||
|
||||
// `AbortController.prototype.signal` is a prototype accessor, so an
|
||||
// own-properties-only copy would drop it.
|
||||
it('should honour `signal` on an AbortController passed as options', function () {
|
||||
const ac = new AbortController();
|
||||
const logs: string[] = [];
|
||||
button.addEventListener('click', () => logs.push('click'), ac);
|
||||
button.dispatchEvent(clickEvent);
|
||||
ac.abort();
|
||||
button.dispatchEvent(clickEvent);
|
||||
expect(logs).toEqual(['click']);
|
||||
expect(button.eventListeners!('click').length).toBe(0);
|
||||
});
|
||||
|
||||
// https://github.com/angular/angular/pull/55796
|
||||
it('should accept a frozen options object', function () {
|
||||
const opts = Object.freeze({capture: true, once: true});
|
||||
const listener = () => {};
|
||||
expect(() => button.addEventListener('click', listener, opts as any)).not.toThrow();
|
||||
button.removeEventListener('click', listener, opts as any);
|
||||
});
|
||||
});
|
||||
|
||||
// https://github.com/angular/angular/issues/56148
|
||||
it('should store the remove abort listener on the task itself and not the task data', function () {
|
||||
const logs: string[] = [];
|
||||
|
||||
Reference in New Issue
Block a user