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 <simon.holthausen@vercel.com>
baseballyama-docs/string-event
Dominic Gannaway 1 year ago committed by GitHub
parent 3f56baf760
commit 73ae5ef5bc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

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

@ -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
}

@ -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
*/

@ -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[];

@ -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,7 +1823,7 @@ 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);
@ -1837,6 +1831,11 @@ export const template_visitors = {
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;

@ -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

@ -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;
};
}

@ -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'];

@ -25,7 +25,9 @@ import {
EACH_IS_CONTROLLED,
EACH_INDEX_REACTIVE,
EACH_ITEM_REACTIVE,
EACH_IS_ANIMATED
EACH_IS_ANIMATED,
PassiveDelegatedEvents,
DelegatedEvents
} from '../../constants.js';
import {
create_fragment_from_html,
@ -77,7 +79,6 @@ const all_registerd_events = new Set();
/** @type {Set<(events: Array<string>) => void>} */
const root_event_handles = new Set();
const passive_delegated_events = new Set(['touchstart', 'touchmove', 'touchend']);
/** @returns {Text} */
function empty() {
@ -2864,10 +2865,10 @@ export function spread_attributes(dom, prev, attrs, css_hash) {
if (prefix === '$$') continue;
if (prefix === 'on') {
// TODO delegate?
/** @type {{ capture?: true }} */
const opts = {};
let event_name = key.slice(2).toLowerCase();
const delegated = DelegatedEvents.includes(event_name);
if (
event_name.endsWith('capture') &&
@ -2877,11 +2878,17 @@ export function spread_attributes(dom, prev, attrs, css_hash) {
event_name = event_name.slice(0, -7);
opts.capture = true;
}
if (prev?.[key]) {
if (!delegated && prev?.[key]) {
dom.removeEventListener(event_name, /** @type {any} */ (prev[key]), opts);
}
if (value != null) {
dom.addEventListener(event_name, value, opts);
if (!delegated) {
dom.addEventListener(event_name, value, opts);
} else {
// @ts-ignore
dom[`__${event_name}`] = value;
delegate([event_name]);
}
}
} else if (value == null) {
dom.removeAttribute(key);
@ -3231,7 +3238,7 @@ export function mount(component, options) {
container.addEventListener(
event_name,
bound_event_listener,
passive_delegated_events.has(event_name)
PassiveDelegatedEvents.includes(event_name)
? {
passive: true
}

@ -0,0 +1,18 @@
import { test } from '../../test';
// Checks that event handlers are not hoisted when one of them is not delegateable
export default test({
html: `<button>0</button>`,
async test({ assert, target }) {
const [button] = target.querySelectorAll('button');
button.click();
await Promise.resolve();
assert.htmlEqual(target.innerHTML, '<button>1</button>');
button.dispatchEvent(new MouseEvent('mouseenter'));
await Promise.resolve();
assert.htmlEqual(target.innerHTML, '<button>2</button>');
}
});

@ -0,0 +1,11 @@
<script>
let count = $state(0)
function increment() {
count += 1
}
</script>
<button onclick={increment} onmouseenter={increment}>
{count}
</button>

@ -0,0 +1,67 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
export default test({
html: `
<button>click me</button>
<button>click me</button>
<button>click me</button>
<button>click me</button>
`,
async test({ assert, target }) {
const [b1, b2, b3, b4] = target.querySelectorAll('button');
flushSync(() => {
b1?.click();
});
assert.htmlEqual(
target.innerHTML,
`
<button>click spread</button>
<button>click spread</button>
<button>click spread</button>
<button>click spread</button>
`
);
flushSync(() => {
b2?.click();
});
assert.htmlEqual(
target.innerHTML,
`
<button>click onclick</button>
<button>click onclick</button>
<button>click onclick</button>
<button>click onclick</button>
`
);
flushSync(() => {
b3?.click();
});
assert.htmlEqual(
target.innerHTML,
`
<button>click spread</button>
<button>click spread</button>
<button>click spread!</button>
<button>click spread!</button>
`
);
flushSync(() => {
b4?.click();
});
assert.htmlEqual(
target.innerHTML,
`
<button>click onclick</button>
<button>click onclick</button>
<button>click onclick?</button>
<button>click onclick?</button>
`
);
}
});

@ -0,0 +1,21 @@
<script>
let text = $state('click me');
let text2 = $state('');
let spread = { onclick: () => text = 'click spread' };
</script>
<button onclick={() => text = 'click onclick'} {...spread}>
{text}
</button>
<button {...spread} onclick={() => text = 'click onclick'}>
{text}
</button>
<button onclick={() => text = 'click onclick'} {...spread} on:click={() => text2 = '!'}>
{text}{text2}
</button>
<button on:click={() => text2 = '?'} {...spread} onclick={() => text = 'click onclick'}>
{text}{text2}
</button>
Loading…
Cancel
Save