fix: prevent event delegation logic conflicting between svelte instances (#17728)

Fixes https://github.com/sveltejs/svelte.dev/issues/1793. There are
actually two fixes here, and either is sufficient to fix the playground,
but they are complementary. First, we only add the delegated event
handler _after_ the component has successfully mounted, otherwise it
will never get cleaned up if an error occurs during mount.

Second, instead of storing data on `event.__root` (which leaks between
instances), we reuse the existing `event_symbol` to provide the
necessary encapsulation. (I'll be honest I don't totally understand what
this property is for anyway and can't be bothered to figure it out right
now, but I'm sure it's important.)

No test because I'm not really sure how you _would_ test this; it
requires a fairly esoteric setup.

### Before submitting the PR, please make sure you do the following

- [x] It's really useful if your PR references an issue where it is
discussed ahead of time. In many cases, features are absent for a
reason. For large changes, please create an RFC:
https://github.com/sveltejs/rfcs
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [ ] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).

### Tests and linting

- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
pull/17725/head
Rich Harris 6 months ago committed by GitHub
parent 60a425193c
commit e47c747338
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: prevent event delegation logic conflicting between svelte instances

@ -11,8 +11,11 @@ import {
set_active_reaction
} from '../../runtime.js';
import { without_reactive_context } from './bindings/shared.js';
import { can_delegate_event } from '../../../../utils.js';
/**
* Used on elements, as a map of event type -> event handler,
* and on events themselves to track which element handled an event
*/
export const event_symbol = Symbol('events');
/** @type {Set<string>} */
@ -177,8 +180,8 @@ export function handle_event_propagation(event) {
last_propagated_event = event;
// composedPath contains list of nodes the event has propagated through.
// We check __root to skip all nodes below it in case this is a
// parent of the __root node, which indicates that there's nested
// We check `event_symbol` to skip all nodes below it in case this is a
// parent of the `event_symbol` node, which indicates that there's nested
// mounted apps. In this case we don't want to trigger events multiple times.
var path_idx = 0;
@ -186,7 +189,7 @@ export function handle_event_propagation(event) {
// without it the variable will be DCE'd and things will
// fail mysteriously in Firefox
// @ts-expect-error is added below
var handled_at = last_propagated_event === event && event.__root;
var handled_at = last_propagated_event === event && event[event_symbol];
if (handled_at) {
var at_idx = path.indexOf(handled_at);
@ -198,7 +201,7 @@ export function handle_event_propagation(event) {
// -> ignore, but set handle_at to document/window so that we're resetting the event
// chain in case someone manually dispatches the same event object again.
// @ts-expect-error
event.__root = handler_element;
event[event_symbol] = handler_element;
return;
}
@ -298,7 +301,7 @@ export function handle_event_propagation(event) {
}
} finally {
// @ts-expect-error is used above
event.__root = handler_element;
event[event_symbol] = handler_element;
// @ts-ignore remove proxy on currentTarget
delete event.currentTarget;
set_active_reaction(previous_reaction);

@ -161,48 +161,6 @@ const listeners = new Map();
function _mount(Component, { target, anchor, props = {}, events, context, intro = true }) {
init_operations();
/** @type {Set<string>} */
var registered_events = new Set();
/** @param {Array<string>} events */
var event_handle = (events) => {
for (var i = 0; i < events.length; i++) {
var event_name = events[i];
if (registered_events.has(event_name)) continue;
registered_events.add(event_name);
var passive = is_passive_event(event_name);
// Add the event listener to both the container and the document.
// The container listener ensures we catch events from within in case
// the outer content stops propagation of the event.
//
// The document listener ensures we catch events that originate from elements that were
// manually moved outside of the container (e.g. via manual portals).
for (const node of [target, document]) {
var counts = listeners.get(node);
if (counts === undefined) {
counts = new Map();
listeners.set(node, counts);
}
var count = counts.get(event_name);
if (count === undefined) {
node.addEventListener(event_name, handle_event_propagation, { passive });
counts.set(event_name, 1);
} else {
counts.set(event_name, count + 1);
}
}
}
};
event_handle(array_from(all_registered_events));
root_event_handles.add(event_handle);
/** @type {Exports} */
// @ts-expect-error will be defined because the render effect runs synchronously
var component = undefined;
@ -251,6 +209,49 @@ function _mount(Component, { target, anchor, props = {}, events, context, intro
}
);
// Setup event delegation _after_ component is mounted - if an error would happen during mount, it would otherwise not be cleaned up
/** @type {Set<string>} */
var registered_events = new Set();
/** @param {Array<string>} events */
var event_handle = (events) => {
for (var i = 0; i < events.length; i++) {
var event_name = events[i];
if (registered_events.has(event_name)) continue;
registered_events.add(event_name);
var passive = is_passive_event(event_name);
// Add the event listener to both the container and the document.
// The container listener ensures we catch events from within in case
// the outer content stops propagation of the event.
//
// The document listener ensures we catch events that originate from elements that were
// manually moved outside of the container (e.g. via manual portals).
for (const node of [target, document]) {
var counts = listeners.get(node);
if (counts === undefined) {
counts = new Map();
listeners.set(node, counts);
}
var count = counts.get(event_name);
if (count === undefined) {
node.addEventListener(event_name, handle_event_propagation, { passive });
counts.set(event_name, 1);
} else {
counts.set(event_name, count + 1);
}
}
}
};
event_handle(array_from(all_registered_events));
root_event_handles.add(event_handle);
return () => {
for (var event_name of registered_events) {
for (const node of [target, document]) {

Loading…
Cancel
Save