fix: tighten up event attributes and hoisting logic (#9433)

- add event delegation to spread_attributes
- add event attributes to spread
- don't delegate when bindings/actions on the same element in order to preserve backwards compatibility of ordering
- don't hoist identifiers when one of them is used in an event that is not delegateable

---------

Co-authored-by: Simon Holthausen <[email protected]>

Author: trueadm

.changeset/strong-lemons-provide.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: handle event attribute spreading with event delegation

packages/svelte/src/compiler/phases/1-parse/state/element.js

@@ -127,7 +127,9 @@ export default function tag(parser) {
 					attributes: [],
 					fragment: create_fragment(true),
 					metadata: {
-						svg: false
+						svg: false,
+						has_spread: false,
+						can_delegate_events: null
 					},
 					parent: null
 			  }

packages/svelte/src/compiler/phases/2-analyze/index.js

@@ -11,7 +11,7 @@ import {
 	object
 } from '../../utils/ast.js';
 import * as b from '../../utils/builders.js';
-import { DelegatedEvents, ReservedKeywords, Runes, SVGElements } from '../constants.js';
+import { ReservedKeywords, Runes, SVGElements } from '../constants.js';
 import { Scope, ScopeRoot, create_scopes, get_rune, set_scope } from '../scope.js';
 import { merge } from '../visitors.js';
 import Stylesheet from './css/Stylesheet.js';
@@ -20,6 +20,7 @@ import { warn } from '../../warnings.js';
 import check_graph_for_cycles from './utils/check_graph_for_cycles.js';
 import { regex_starts_with_newline } from '../patterns.js';
 import { create_attribute, is_element_node } from '../nodes.js';
+import { DelegatedEvents } from '../../../constants.js';
 
 /**
  * @param {import('#compiler').Script | null} script
@@ -58,7 +59,7 @@ function get_component_name(filename) {
 }
 
 /**
- * @param {Pick<import('#compiler').OnDirective, 'expression'| 'name' | 'modifiers'>} node
+ * @param {Pick<import('#compiler').OnDirective, 'expression'| 'name' | 'modifiers'> & { type: string }} node
  * @param {import('./types').Context} context
  * @returns {null | import('#compiler').DelegatedEvent}
  */
@@ -70,16 +71,13 @@ function get_delegated_event(node, context) {
 	if (!handler || node.modifiers.includes('capture') || !DelegatedEvents.includes(event_name)) {
 		return null;
 	}
-	// If we are not working with a RegularElement/SlotElement, then bail-out.
+	// If we are not working with a RegularElement, then bail-out.
 	const element = context.path.at(-1);
-	if (element == null || (element.type !== 'RegularElement' && element.type !== 'SlotElement')) {
+	if (element?.type !== 'RegularElement') {
 		return null;
 	}
-	// If we have multiple OnDirectives of the same type, bail-out.
-	if (
-		element.attributes.filter((attr) => attr.type === 'OnDirective' && attr.name === event_name)
-			.length > 1
-	) {
+	// If element says we can't delegate because we have multiple OnDirectives of the same type, bail-out.
+	if (!element.metadata.can_delegate_events) {
 		return null;
 	}
 
@@ -89,6 +87,11 @@ function get_delegated_event(node, context) {
 	let target_function = null;
 	let binding = null;
 
+	if (node.type === 'Attribute' && element.metadata.has_spread) {
+		// event attribute becomes part of the dynamic spread array
+		return non_hoistable;
+	}
+
 	if (handler.type === 'ArrowFunctionExpression' || handler.type === 'FunctionExpression') {
 		target_function = handler;
 	} else if (handler.type === 'Identifier') {
@@ -101,16 +104,29 @@ function get_delegated_event(node, context) {
 					return non_hoistable;
 				}
 
-				const element =
-					parent.type === 'OnDirective'
-						? path.at(-2)
-						: parent.type === 'ExpressionTag' &&
-						  is_event_attribute(/** @type {import('#compiler').Attribute} */ (path.at(-2)))
-						? path.at(-3)
-						: null;
+				/** @type {import('#compiler').RegularElement | null} */
+				let element = null;
+				/** @type {string | null} */
+				let event_name = null;
+				if (parent.type === 'OnDirective') {
+					element = /** @type {import('#compiler').RegularElement} */ (path.at(-2));
+					event_name = parent.name;
+				} else if (
+					parent.type === 'ExpressionTag' &&
+					is_event_attribute(/** @type {import('#compiler').Attribute} */ (path.at(-2)))
+				) {
+					element = /** @type {import('#compiler').RegularElement} */ (path.at(-3));
+					const attribute = /** @type {import('#compiler').Attribute} */ (path.at(-2));
+					event_name = get_attribute_event_name(attribute.name);
+				}
 
-				if (element) {
-					if (element.type !== 'RegularElement' && element.type !== 'SlotElement') {
+				if (element && event_name) {
+					if (
+						element.type !== 'RegularElement' ||
+						!determine_element_spread_and_delegatable(element).metadata.can_delegate_events ||
+						(element.metadata.has_spread && node.type === 'Attribute') ||
+						!DelegatedEvents.includes(event_name)
+					) {
 						return non_hoistable;
 					}
 				} else if (parent.type !== 'FunctionDeclaration' && parent.type !== 'VariableDeclarator') {
@@ -772,16 +788,15 @@ const common_visitors = {
 
 			let name = node.name.slice(2);
 
-			if (
-				name.endsWith('capture') &&
-				name !== 'ongotpointercapture' &&
-				name !== 'onlostpointercapture'
-			) {
+			if (is_capture_event(name)) {
 				name = name.slice(0, -7);
 				modifiers.push('capture');
 			}
 
-			const delegated_event = get_delegated_event({ name, expression, modifiers }, context);
+			const delegated_event = get_delegated_event(
+				{ type: node.type, name, expression, modifiers },
+				context
+			);
 
 			if (delegated_event !== null) {
 				if (delegated_event.type === 'hoistable') {
@@ -950,6 +965,8 @@ const common_visitors = {
 			node.metadata.svg = true;
 		}
 
+		determine_element_spread_and_delegatable(node);
+
 		// Special case: Move the children of <textarea> into a value attribute if they are dynamic
 		if (
 			context.state.options.namespace !== 'foreign' &&
@@ -1005,6 +1022,77 @@ const common_visitors = {
 	}
 };
 
+/**
+ * Check if events on this element can theoretically be delegated. They can if there's no
+ * possibility of an OnDirective and an event attribute on the same element, and if there's
+ * no OnDirectives of the same type (the latter is a bit too strict because `on:click on:click on:keyup`
+ * means that `on:keyup` can be delegated but we gloss over this edge case).
+ * @param {import('#compiler').RegularElement} node
+ */
+function determine_element_spread_and_delegatable(node) {
+	if (typeof node.metadata.can_delegate_events === 'boolean') {
+		return node; // did this already
+	}
+
+	let events = new Map();
+	let has_spread = false;
+	let has_on = false;
+	let has_action_or_bind = false;
+	for (const attribute of node.attributes) {
+		if (
+			attribute.type === 'OnDirective' ||
+			(attribute.type === 'Attribute' && is_event_attribute(attribute))
+		) {
+			let event_name = attribute.name;
+			if (attribute.type === 'Attribute') {
+				event_name = get_attribute_event_name(event_name);
+			}
+			events.set(event_name, (events.get(event_name) || 0) + 1);
+			if (!has_on && attribute.type === 'OnDirective') {
+				has_on = true;
+			}
+		} else if (!has_spread && attribute.type === 'SpreadAttribute') {
+			has_spread = true;
+		} else if (
+			!has_action_or_bind &&
+			(attribute.type === 'BindDirective' || attribute.type === 'UseDirective')
+		) {
+			has_action_or_bind = true;
+		}
+	}
+	node.metadata.can_delegate_events =
+		// Actions/bindings need the old on:-events to fire in order
+		!has_action_or_bind &&
+		// spreading events means we don't know if there's an event attribute with the same name as an on:-event
+		!(has_spread && has_on) &&
+		// multiple on:-events/event attributes with the same name
+		![...events.values()].some((count) => count > 1);
+	node.metadata.has_spread = has_spread;
+
+	return node;
+}
+
+/**
+ * @param {string} event_name
+ */
+function get_attribute_event_name(event_name) {
+	if (is_capture_event(event_name)) {
+		event_name = event_name.slice(0, -7);
+	}
+	event_name = event_name.slice(2);
+	return event_name;
+}
+
+/**
+ * @param {string} name
+ * @returns boolean
+ */
+function is_capture_event(name) {
+	return (
+		name.endsWith('capture') && name !== 'ongotpointercapture' && name !== 'onlostpointercapture'
+	);
+}
+
 /**
  * @param {Map<import('estree').LabeledStatement, import('../types.js').ReactiveStatement>} unsorted_reactive_declarations
  */

packages/svelte/src/compiler/phases/3-transform/client/types.d.ts

@@ -42,7 +42,7 @@ export interface ComponentClientTransformState extends ClientTransformState {
 		/** Used if condition for singular prop is false (see comment above) */
 		grouped: Statement;
 	}[];
-	/** Stuff that happens after the render effect (bindings, events, actions) */
+	/** Stuff that happens after the render effect (bindings, actions) */
 	readonly after_update: Statement[];
 	/** The HTML template string */
 	readonly template: string[];

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

@@ -1728,7 +1728,6 @@ export const template_visitors = {
 		const is_custom_element = is_custom_element_node(node);
 		let needs_input_reset = false;
 		let needs_content_reset = false;
-		let has_spread = false;
 
 		/** @type {import('#compiler').BindDirective | null} */
 		let value_binding = null;
@@ -1748,27 +1747,22 @@ export const template_visitors = {
 
 		for (const attribute of node.attributes) {
 			if (attribute.type === 'Attribute') {
-				if (is_event_attribute(attribute)) {
-					serialize_event_attribute(attribute, context);
-				} else {
-					attributes.push(attribute);
-					if (
-						(attribute.name === 'value' || attribute.name === 'checked') &&
-						!is_text_attribute(attribute)
-					) {
-						needs_input_reset = true;
-						needs_content_reset = true;
-					} else if (
-						attribute.name === 'contenteditable' &&
-						(attribute.value === true ||
-							(is_text_attribute(attribute) && attribute.value[0].data === 'true'))
-					) {
-						is_content_editable = true;
-					}
+				attributes.push(attribute);
+				if (
+					(attribute.name === 'value' || attribute.name === 'checked') &&
+					!is_text_attribute(attribute)
+				) {
+					needs_input_reset = true;
+					needs_content_reset = true;
+				} else if (
+					attribute.name === 'contenteditable' &&
+					(attribute.value === true ||
+						(is_text_attribute(attribute) && attribute.value[0].data === 'true'))
+				) {
+					is_content_editable = true;
 				}
 			} else if (attribute.type === 'SpreadAttribute') {
 				attributes.push(attribute);
-				has_spread = true;
 				needs_input_reset = true;
 				needs_content_reset = true;
 			} else if (attribute.type === 'ClassDirective') {
@@ -1829,14 +1823,19 @@ export const template_visitors = {
 
 		// Then do attributes
 		let is_attributes_reactive = false;
-		if (has_spread) {
+		if (node.metadata.has_spread) {
 			const spread_id = serialize_element_spread_attributes(attributes, context, node_id);
 			if (child_metadata.namespace !== 'foreign') {
 				add_select_to_spread_update(spread_id, node, context, node_id);
 			}
 			is_attributes_reactive = spread_id !== null;
 		} else {
 			for (const attribute of /** @type {import('#compiler').Attribute[]} */ (attributes)) {
+				if (is_event_attribute(attribute)) {
+					serialize_event_attribute(attribute, context);
+					continue;
+				}
+
 				if (needs_special_value_handling && attribute.name === 'value') {
 					serialize_element_special_value_attribute(node.name, node_id, attribute, context);
 					continue;

packages/svelte/src/compiler/phases/constants.js

@@ -66,32 +66,6 @@ export const VoidElements = [
 	'wbr'
 ];
 
-// list of Element events that will be delegated
-export const DelegatedEvents = [
-	'beforeinput',
-	'click',
-	'dblclick',
-	'contextmenu',
-	'focusin',
-	'focusout',
-	// 'input', This conflicts with bind:input
-	'keydown',
-	'keyup',
-	'mousedown',
-	'mousemove',
-	'mouseout',
-	'mouseover',
-	'mouseup',
-	'pointerdown',
-	'pointermove',
-	'pointerout',
-	'pointerover',
-	'pointerup',
-	'touchend',
-	'touchmove',
-	'touchstart'
-];
-
 export const PassiveEvents = ['wheel', 'touchstart', 'touchmove', 'touchend', 'touchcancel'];
 
 // TODO this is currently unused

packages/svelte/src/compiler/types/template.d.ts

@@ -283,6 +283,14 @@ export interface RegularElement extends BaseElement {
 	metadata: {
 		/** `true` if this is an svg element */
 		svg: boolean;
+		/** `true` if contains a SpreadAttribute */
+		has_spread: boolean;
+		/**
+		 * `true` if events on this element can theoretically be delegated. This doesn't necessarily mean that
+		 * a specific event will be delegated, as there are other factors which affect the final outcome.
+		 * `null` only until it was determined whether this element can be delegated or not.
+		 */
+		can_delegate_events: boolean | null;
 	};
 }
 

packages/svelte/src/constants.js

@@ -3,3 +3,32 @@ export const EACH_INDEX_REACTIVE = 1 << 1;
 export const EACH_KEYED = 1 << 2;
 export const EACH_IS_CONTROLLED = 1 << 3;
 export const EACH_IS_ANIMATED = 1 << 4;
+
+/** List of Element events that will be delegated */
+export const DelegatedEvents = [
+	'beforeinput',
+	'click',
+	'dblclick',
+	'contextmenu',
+	'focusin',
+	'focusout',
+	// 'input', This conflicts with bind:input
+	'keydown',
+	'keyup',
+	'mousedown',
+	'mousemove',
+	'mouseout',
+	'mouseover',
+	'mouseup',
+	'pointerdown',
+	'pointermove',
+	'pointerout',
+	'pointerover',
+	'pointerup',
+	'touchend',
+	'touchmove',
+	'touchstart'
+];
+
+/** List of Element events that will be delegated and are passive */
+export const PassiveDelegatedEvents = ['touchstart', 'touchmove', 'touchend'];