event_context (#13737)

Author: trueadm

.changeset/giant-plants-shout.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: ensure event context is reset before invoking callback

packages/svelte/src/internal/client/dom/elements/events.js

@@ -5,6 +5,12 @@ import { hydrating } from '../hydration.js';
 import { queue_micro_task } from '../task.js';
 import { FILENAME } from '../../../../constants.js';
 import * as w from '../../warnings.js';
+import {
+	active_effect,
+	active_reaction,
+	set_active_effect,
+	set_active_reaction
+} from '../../runtime.js';
 
 /** @type {Set<string>} */
 export const all_registered_events = new Set();
@@ -55,7 +61,17 @@ export function create_event(event_name, dom, handler, options) {
 			handle_event_propagation.call(dom, event);
 		}
 		if (!event.cancelBubble) {
-			return handler.call(this, event);
+			var previous_reaction = active_reaction;
+			var previous_effect = active_effect;
+
+			set_active_reaction(null);
+			set_active_effect(null);
+			try {
+				return handler.call(this, event);
+			} finally {
+				set_active_reaction(previous_reaction);
+				set_active_effect(previous_effect);
+			}
 		}
 	}
 
@@ -196,6 +212,16 @@ export function handle_event_propagation(event) {
 		}
 	});
 
+	// This started because of Chromium issue https://chromestatus.com/feature/5128696823545856,
+	// where removal or moving of of the DOM can cause sync `blur` events to fire, which can cause logic
+	// to run inside the current `active_reaction`, which isn't what we want at all. However, on reflection,
+	// it's probably best that all event handled by Svelte have this behaviour, as we don't really want
+	// an event handler to run in the context of another reaction or effect.
+	var previous_reaction = active_reaction;
+	var previous_effect = active_effect;
+	set_active_reaction(null);
+	set_active_effect(null);
+
 	try {
 		/**
 		 * @type {unknown}
@@ -253,6 +279,8 @@ export function handle_event_propagation(event) {
 		event.__root = handler_element;
 		// @ts-ignore remove proxy on currentTarget
 		delete event.currentTarget;
+		set_active_reaction(previous_reaction);
+		set_active_effect(previous_effect);
 	}
 }
 

packages/svelte/src/internal/client/dom/elements/transitions.js

@@ -1,13 +1,7 @@
 /** @import { AnimateFn, Animation, AnimationConfig, EachItem, Effect, TransitionFn, TransitionManager } from '#client' */
 import { noop, is_function } from '../../../shared/utils.js';
 import { effect } from '../../reactivity/effects.js';
-import {
-	active_effect,
-	active_reaction,
-	set_active_effect,
-	set_active_reaction,
-	untrack
-} from '../../runtime.js';
+import { active_effect, untrack } from '../../runtime.js';
 import { loop } from '../../loop.js';
 import { should_intro } from '../../render.js';
 import { current_each_item } from '../blocks/each.js';
@@ -21,16 +15,7 @@ import { queue_micro_task } from '../task.js';
  * @returns {void}
  */
 function dispatch_event(element, type) {
-	var previous_reaction = active_reaction;
-	var previous_effect = active_effect;
-	set_active_reaction(null);
-	set_active_effect(null);
-	try {
-		element.dispatchEvent(new CustomEvent(type));
-	} finally {
-		set_active_reaction(previous_reaction);
-		set_active_effect(previous_effect);
-	}
+	element.dispatchEvent(new CustomEvent(type));
 }
 
 /**

packages/svelte/src/internal/client/reactivity/effects.js

@@ -15,8 +15,7 @@ import {
 	set_is_destroying_effect,
 	set_is_flushing_effect,
 	set_signal_status,
-	untrack,
-	set_active_effect
+	untrack
 } from '../runtime.js';
 import {
 	DIRTY,
@@ -423,26 +422,13 @@ export function destroy_effect(effect, remove_dom = true) {
 		/** @type {TemplateNode | null} */
 		var node = effect.nodes_start;
 		var end = effect.nodes_end;
-		var previous_reaction = active_reaction;
-		var previous_effect = active_effect;
 
-		// Really we only need to do this in Chromium because of https://chromestatus.com/feature/5128696823545856,
-		// as removal of the DOM can cause sync `blur` events to fire, which can cause logic to run inside
-		// the current `active_reaction`, which isn't what we want at all. Additionally, the blur event handler
-		// might create a derived or effect and they will be incorrectly attached to the wrong thing
-		set_active_reaction(null);
-		set_active_effect(null);
-		try {
-			while (node !== null) {
-				/** @type {TemplateNode | null} */
-				var next = node === end ? null : /** @type {TemplateNode} */ (get_next_sibling(node));
+		while (node !== null) {
+			/** @type {TemplateNode | null} */
+			var next = node === end ? null : /** @type {TemplateNode} */ (get_next_sibling(node));
 
-				node.remove();
-				node = next;
-			}
-		} finally {
-			set_active_reaction(previous_reaction);
-			set_active_effect(previous_effect);
+			node.remove();
+			node = next;
 		}
 
 		removed = true;

packages/svelte/tests/runtime-runes/samples/event-context/_config.js

@@ -0,0 +1,14 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	test({ assert, target, logs }) {
+		const [b1] = target.querySelectorAll('button');
+
+		b1?.click();
+		b1?.click();
+		b1?.click();
+		flushSync();
+		assert.htmlEqual(target.innerHTML, '<button>4</button>');
+	}
+});

packages/svelte/tests/runtime-runes/samples/event-context/main.svelte

@@ -0,0 +1,13 @@
+<script>
+	let count = $state(0);
+	let button = $state();
+
+	function do_thing() {
+		button?.click();
+		return false;
+	}
+</script>
+
+<button bind:this={button} onclick={() => count++ }>{count}</button>
+
+{#if do_thing()}{/if}