Repository navigation
Conversation
ef33cdd to
5bfb5a7
Compare
|
Just as context, here is a minimal reproduction of the HighChart bug that lead me to this: https://jsfiddle.net/5ewv9gL7/. Change to From there I looked at the HighChart logic https://github.com/highcharts/highcharts/blob/69edf8952a04b9872b6510fbdacd7dff8b03970b/ts/Core/Globals.ts#L201 that works in The logic is taken from here it seems https://github.com/WICG/EventListenerOptions/blob/gh-pages/explainer.md#feature-detection |
…ions
`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 angular#54142; that patch generalises the workaround to every recognised
member.
The copy itself was the correct fix for angular#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 angular#70431
Co-authored-by: Matthieu Riegler <[email protected]>
318d25c to
53722e3
Compare
JeanMeche
added
action: merge
|
This pull request has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
PR Type
What is the current behavior?
Issue Number: #70431
Since zone.js 0.14.7 (June 2024, #55796), the patched
addEventListenersilently drops event listener options that are exposed as accessors or as non-enumerable properties.copyEventListenerOptionscopies the caller's options with{...options}before forwarding to native. Object spread copies only own enumerable data properties. The native call, by contrast, reads each dictionary member via WebIDL — a plain[[Get]], which invokes accessors and ignores enumerability. The copy is therefore lossy in a way the native call is not.Observed in Chromium, zone.js 0.15.1:
capture: trueonce: truepassivegetter (feature-detection)Nothing throws in any case.
signalwas already special-cased forAbortController.prototype.signalafter #54142; the other three members were not.The
passiverow is the one with ecosystem-wide fallout: it defeats the MDN feature test for passive-listener support, where the getter being read is the signal. That snippet is used verbatim by Highcharts (See: https://github.com/highcharts/highcharts/blob/69edf8952a04b9872b6510fbdacd7dff8b03970b/ts/Core/Globals.ts#L201). Making every listener they register non-passive. Including ones they explicitly intended to be passive.What is the new behavior?
The copy is still made — that fix from #55796 is preserved (frozen/readonly options still work). Each recognised member (
capture,once,passive,signal) is additionally read from the source object via[[Get]]when the spread didn't already produce it, which recovers non-enumerable and prototype accessors. ThehasOwnPropertygate on the loop ensures no getter is invoked twice.The call site is reordered to
buildEventListenerOptions(copyEventListenerOptions(...))so the passive-events code path ({...options, passive: true}inbuildEventListenerOptions) also spreads a normalised data object rather than the caller's raw input.Tests added in
packages/zone.js/test/browser/browser.spec.tscover:passivegetter invoked exactly once (MDN feature-detect pattern);capture: trueaccessor honoured for both add and remove;once: trueaccessor honoured across two dispatches;AbortControllerpassed directly as options (regression guard for AbortController error with walletconnect lib after zone.js 0.14.3 update #54142);Alternatives considered and rejected:
Object.defineProperties({}, Object.getOwnPropertyDescriptors(options))). Losessignal— it lives onAbortController.prototype, not as an own property.Object.create(Object.getPrototypeOf(options), Object.getOwnPropertyDescriptors(options))). Throws on built-ins:TypeError: Cannot read private member #signal from an object whose class did not declare it. This is the hazard the pre-existing code comment already warned about.Does this PR introduce a breaking change?
Other information
Provenance: introduced by #55796 /
85c1719. Bisected: 0.13.3, 0.14.0, 0.14.4, 0.14.6 all behave correctly; 0.14.7 → 0.16.2 do not.Fixes #70431.