fix: run event attributes after binding event listeners (#11230)

* fix: run event attributes after binding event listeners

By running the event listener logic inside an effect on the first run we guarantee that they're attached after binding listeners. Fixes #11138.

* give arrow functions stable id, better code gen

* optimise normal function expressions too (rare but valid!)

---------

Co-authored-by: Rich Harris <[email protected]>

Author: dummdidumm

.changeset/seven-garlics-serve.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: make sure event attributes run after bindings

packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js

@@ -263,17 +263,29 @@ function serialize_element_spread_attributes(
 ) {
 	let needs_isolation = false;
 
-	/** @type {import('estree').Expression[]} */
+	/** @type {import('estree').ObjectExpression['properties']} */
 	const values = [];
 
 	for (const attribute of attributes) {
 		if (attribute.type === 'Attribute') {
 			const name = get_attribute_name(element, attribute, context);
 			// TODO: handle contains_call_expression
 			const [, value] = serialize_attribute_value(attribute.value, context);
-			values.push(b.object([b.init(name, value)]));
+
+			if (
+				is_event_attribute(attribute) &&
+				(attribute.value[0].expression.type === 'ArrowFunctionExpression' ||
+					attribute.value[0].expression.type === 'FunctionExpression')
+			) {
+				// Give the event handler a stable ID so it isn't removed and readded on every update
+				const id = context.state.scope.generate('event_handler');
+				context.state.init.push(b.var(id, value));
+				values.push(b.init(attribute.name, b.id(id)));
+			} else {
+				values.push(b.init(name, value));
+			}
 		} else {
-			values.push(/** @type {import('estree').Expression} */ (context.visit(attribute)));
+			values.push(b.spread(/** @type {import('estree').Expression} */ (context.visit(attribute))));
 		}
 
 		needs_isolation ||=
@@ -292,7 +304,7 @@ function serialize_element_spread_attributes(
 				'$.set_attributes',
 				element_id,
 				b.id(id),
-				b.array(values),
+				b.object(values),
 				lowercase_attributes,
 				b.literal(context.state.analysis.css.hash)
 			)
@@ -350,15 +362,27 @@ function serialize_dynamic_element_attributes(attributes, context, element_id) {
 	let needs_isolation = false;
 	let is_reactive = false;
 
-	/** @type {import('estree').Expression[]} */
+	/** @type {import('estree').ObjectExpression['properties']} */
 	const values = [];
 
 	for (const attribute of attributes) {
 		if (attribute.type === 'Attribute') {
 			const [, value] = serialize_attribute_value(attribute.value, context);
-			values.push(b.object([b.init(attribute.name, value)]));
+
+			if (
+				is_event_attribute(attribute) &&
+				(attribute.value[0].expression.type === 'ArrowFunctionExpression' ||
+					attribute.value[0].expression.type === 'FunctionExpression')
+			) {
+				// Give the event handler a stable ID so it isn't removed and readded on every update
+				const id = context.state.scope.generate('event_handler');
+				context.state.init.push(b.var(id, value));
+				values.push(b.init(attribute.name, b.id(id)));
+			} else {
+				values.push(b.init(attribute.name, value));
+			}
 		} else {
-			values.push(/** @type {import('estree').Expression} */ (context.visit(attribute)));
+			values.push(b.spread(/** @type {import('estree').Expression} */ (context.visit(attribute))));
 		}
 
 		is_reactive ||=
@@ -381,7 +405,7 @@ function serialize_dynamic_element_attributes(attributes, context, element_id) {
 					'$.set_dynamic_element_attributes',
 					element_id,
 					b.id(id),
-					b.array(values),
+					b.object(values),
 					b.literal(context.state.analysis.css.hash)
 				)
 			)
@@ -402,7 +426,7 @@ function serialize_dynamic_element_attributes(attributes, context, element_id) {
 				'$.set_dynamic_element_attributes',
 				element_id,
 				b.literal(null),
-				b.array(values),
+				b.object(values),
 				b.literal(context.state.analysis.css.hash)
 			)
 		)

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

@@ -4,6 +4,8 @@ import { get_descriptors, map_get, map_set, object_assign } from '../../utils.js
 import { AttributeAliases, DelegatedEvents, namespace_svg } from '../../../../constants.js';
 import { delegate } from './events.js';
 import { autofocus } from './misc.js';
+import { effect } from '../../reactivity/effects.js';
+import { run } from '../../../shared/utils.js';
 
 /**
  * The value/checked attribute in the template actually corresponds to the defaultValue property, so we need
@@ -81,14 +83,13 @@ export function set_custom_element_data(node, prop, value) {
 /**
  * Spreads attributes onto a DOM element, taking into account the currently set attributes
  * @param {Element & ElementCSSInlineStyle} element
- * @param {Record<string, unknown> | undefined} prev
- * @param {Record<string, unknown>[]} attrs
+ * @param {Record<string, any> | undefined} prev
+ * @param {Record<string, any>} next New attributes - this function mutates this object
  * @param {boolean} lowercase_attributes
  * @param {string} css_hash
- * @returns {Record<string, unknown>}
+ * @returns {Record<string, any>}
  */
-export function set_attributes(element, prev, attrs, lowercase_attributes, css_hash) {
-	var next = object_assign({}, ...attrs);
+export function set_attributes(element, prev, next, lowercase_attributes, css_hash) {
 	var has_hash = css_hash.length !== 0;
 
 	for (var key in prev) {
@@ -106,6 +107,8 @@ export function set_attributes(element, prev, attrs, lowercase_attributes, css_h
 
 	// @ts-expect-error
 	var attributes = /** @type {Record<string, unknown>} **/ (element.__attributes ??= {});
+	/** @type {Array<() => void>} */
+	var events = [];
 
 	for (key in next) {
 		var value = next[key];
@@ -135,7 +138,11 @@ export function set_attributes(element, prev, attrs, lowercase_attributes, css_h
 
 			if (value != null) {
 				if (!delegated) {
-					element.addEventListener(event_name, value, opts);
+					if (!prev) {
+						events.push(() => element.addEventListener(event_name, value, opts));
+					} else {
+						element.addEventListener(event_name, value, opts);
+					}
 				} else {
 					// @ts-ignore
 					element[`__${event_name}`] = value;
@@ -177,19 +184,23 @@ export function set_attributes(element, prev, attrs, lowercase_attributes, css_h
 		}
 	}
 
+	// On the first run, ensure that events are added after bindings so
+	// that their listeners fire after the binding listeners
+	if (!prev) {
+		effect(() => events.forEach(run));
+	}
+
 	return next;
 }
 
 /**
  * @param {Element} node
- * @param {Record<string, unknown> | undefined} prev
- * @param {Record<string, unknown>[]} attrs
+ * @param {Record<string, any> | undefined} prev
+ * @param {Record<string, any>} next The new attributes - this function mutates this object
  * @param {string} css_hash
  */
-export function set_dynamic_element_attributes(node, prev, attrs, css_hash) {
+export function set_dynamic_element_attributes(node, prev, next, css_hash) {
 	if (node.tagName.includes('-')) {
-		var next = object_assign({}, ...attrs);
-
 		for (var key in prev) {
 			if (!(key in next)) {
 				next[key] = null;
@@ -206,7 +217,7 @@ export function set_dynamic_element_attributes(node, prev, attrs, css_hash) {
 	return set_attributes(
 		/** @type {Element & ElementCSSInlineStyle} */ (node),
 		prev,
-		attrs,
+		next,
 		node.namespaceURI !== namespace_svg,
 		css_hash
 	);

packages/svelte/tests/runtime-runes/samples/event-attribute-after-binding/_config.js

@@ -0,0 +1,21 @@
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, target }) {
+		const [i1, i2] = target.querySelectorAll('input');
+
+		i1?.click();
+		await Promise.resolve();
+		assert.htmlEqual(
+			target.innerHTML,
+			'true true <input type="checkbox"> false false <input type="checkbox">'
+		);
+
+		i2?.click();
+		await Promise.resolve();
+		assert.htmlEqual(
+			target.innerHTML,
+			'true true <input type="checkbox"> true true <input type="checkbox">'
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/event-attribute-after-binding/main.svelte

@@ -0,0 +1,15 @@
+<script>
+	let checked_simple = $state(false);
+	let checked_simple_copy = $state(false);
+	
+	let checked_rest = $state(false);
+	let checked_rest_copy = $state(false);
+	let rest = $state(() => ({}));
+</script>
+
+{checked_simple} {checked_simple_copy}
+<input type="checkbox" onchange={() => {checked_simple_copy = checked_simple}} bind:checked={checked_simple} />
+
+{checked_rest} {checked_rest_copy}
+<!-- {...rest()} in order to force an isolated render effect -->
+<input type="checkbox" onchange={() => {checked_rest_copy = checked_rest}} {...rest()} bind:checked={checked_rest} />