FIX #2446: apply bindings and event handlers in order

src/compile/render-dom/wrappers/Element/index.ts

@@ -21,6 +21,8 @@ import add_actions from '../shared/add_actions';
 import create_debugging_comment from '../shared/create_debugging_comment';
 import { get_context_merger } from '../shared/get_context_merger';
 import Slot from '../../../nodes/Slot';
+import EventHandler from "../../../nodes/EventHandler";
+import BindingWrapper from "./Binding";
 
 const events = [
 	{
@@ -308,8 +310,7 @@ export default class ElementWrapper extends Wrapper {
 			block.maintain_context = true;
 		}
 
-		this.add_bindings(block);
+		this.add_directives_in_order(block);
-		this.add_event_handlers(block);
 		this.add_attributes(block);
 		this.add_transitions(block);
 		this.add_animation(block);
@@ -389,29 +390,60 @@ export default class ElementWrapper extends Wrapper {
 			: `{}`}, ${this.node.namespace === namespaces.svg ? true : false})`;
 	}
 
-	add_bindings(block: Block) {
+	add_directives_in_order (block: Block) {
+		const bindingGroups = events
+			.map(event => ({
+				events: event.event_names,
+				bindings: this.bindings
+					.filter(binding => binding.node.name !== 'this')
+					.filter(binding => event.filter(this.node, binding.node.name))
+			}))
+			.filter(group => group.bindings.length);
+
+		const this_binding = this.bindings.find(b => b.node.name === 'this');
+
+		function getOrder (item) {
+			if (item instanceof EventHandler) {
+				return (item as EventHandler).start;
+			} else if (item instanceof BindingWrapper) {
+				return (item as BindingWrapper).node.start
+			} else {
+				return item.bindings[0].node.start
+			}
+		}
+
+		const ordered = [
+			...bindingGroups,
+			...this.node.handlers,
+			...(this_binding ? [this_binding] : [])
+		];
+		ordered.sort((a, b) => getOrder(a) - getOrder(b));
+
+		ordered.forEach(bindingGroupOrEventHandler => {
+			if (bindingGroupOrEventHandler instanceof EventHandler) {
+				add_event_handlers(block, this.var, [bindingGroupOrEventHandler as EventHandler])
+			} else if (bindingGroupOrEventHandler instanceof BindingWrapper) {
+				this.add_this_binding(block, bindingGroupOrEventHandler as BindingWrapper);
+			} else {
+				this.add_binding(block, bindingGroupOrEventHandler);
+			}
+		});
+	}
+
+	add_binding(block: Block, bindingGroup) {
 		const { renderer } = this;
 
-		if (this.bindings.length === 0) return;
+		if (bindingGroup.bindings.length === 0) return;
 
 		renderer.component.has_reactive_assignments = true;
 
-		const lock = this.bindings.some(binding => binding.needs_lock) ?
+		const lock = bindingGroup.bindings.some(binding => binding.needs_lock) ?
 			block.get_unique_name(`${this.var}_updating`) :
 			null;
 
 		if (lock) block.add_variable(lock, 'false');
 
-		const groups = events
+		[bindingGroup].forEach(group => {
-			.map(event => ({
-				events: event.event_names,
-				bindings: this.bindings
-					.filter(binding => binding.node.name !== 'this')
-					.filter(binding => event.filter(this.node, binding.node.name))
-			}))
-			.filter(group => group.bindings.length);
-
-		groups.forEach(group => {
 			const handler = renderer.component.get_unique_name(`${this.var}_${group.events.join('_')}_handler`);
 
 			renderer.component.add_var({
@@ -514,42 +546,55 @@ export default class ElementWrapper extends Wrapper {
 		if (lock) {
 			block.builders.update.add_line(`${lock} = false;`);
 		}
+	}
 
-		const this_binding = this.bindings.find(b => b.node.name === 'this');
+	add_this_binding(block: Block, this_binding: BindingWrapper) {
-		if (this_binding) {
+		const { renderer } = this;
-			const name = renderer.component.get_unique_name(`${this.var}_binding`);
 
-			renderer.component.add_var({
+		renderer.component.has_reactive_assignments = true;
-				name,
-				internal: true,
-				referenced: true
-			});
 
-			const { handler, object } = this_binding;
+		const lock = this.bindings.some(binding => binding.needs_lock) ?
+			block.get_unique_name(`${this.var}_updating`) :
+			null;
 
-			const args = [];
+		if (lock) block.add_variable(lock, 'false');
-			for (const arg of handler.contextual_dependencies) {
-				args.push(arg);
-				block.add_variable(arg, `ctx.${arg}`);
-			}
 
-			renderer.component.partly_hoisted.push(deindent`
+		if (lock) {
-				function ${name}(${['$$node', 'check'].concat(args).join(', ')}) {
+			block.builders.update.add_line(`${lock} = false;`);
-					${handler.snippet ? `if ($$node || (!$$node && ${handler.snippet} === check)) ` : ''}${handler.mutation}
+		}
-					${renderer.component.invalidate(object)};
-				}
-			`);
 
-			block.builders.mount.add_line(`@add_binding_callback(() => ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}));`);
+		const name = renderer.component.get_unique_name(`${this.var}_binding`);
-			block.builders.destroy.add_line(`ctx.${name}(${['null', this.var].concat(args).join(', ')});`);
+
-			block.builders.update.add_line(deindent`
+		renderer.component.add_var({
-				if (changed.items) {
+			name,
-					ctx.${name}(${['null', this.var].concat(args).join(', ')});
+			internal: true,
-					${args.map(a => `${a} = ctx.${a}`).join(', ')};
+			referenced: true
-					ctx.${name}(${[this.var, 'null'].concat(args).join(', ')});
+		});
-				}`
+
-			);
+		const { handler, object } = this_binding;
+
+		const args = [];
+		for (const arg of handler.contextual_dependencies) {
+			args.push(arg);
+			block.add_variable(arg, `ctx.${arg}`);
 		}
+
+		renderer.component.partly_hoisted.push(deindent`
+			function ${name}(${['$$node', 'check'].concat(args).join(', ')}) {
+				${handler.snippet ? `if ($$node || (!$$node && ${handler.snippet} === check)) ` : ''}${handler.mutation}
+				${renderer.component.invalidate(object)};
+			}
+		`);
+
+		block.builders.mount.add_line(`@add_binding_callback(() => ctx.${name}(${[this.var, 'null'].concat(args).join(', ')}));`);
+		block.builders.destroy.add_line(`ctx.${name}(${['null', this.var].concat(args).join(', ')});`);
+		block.builders.update.add_line(deindent`
+			if (changed.items) {
+				ctx.${name}(${['null', this.var].concat(args).join(', ')});
+				${args.map(a => `${a} = ctx.${a}`).join(', ')};
+				ctx.${name}(${[this.var, 'null'].concat(args).join(', ')});
+			}`
+		);
 	}
 
 	add_attributes(block: Block) {

test/js/samples/media-bindings/expected.js

@@ -26,18 +26,18 @@ function create_fragment(ctx) {
 	return {
 		c() {
 			audio = element("audio");
-			if (ctx.played === void 0 || ctx.currentTime === void 0) add_render_callback(audio_timeupdate_handler);
-			if (ctx.duration === void 0) add_render_callback(() => ctx.audio_durationchange_handler.call(audio));
 			if (ctx.buffered === void 0) add_render_callback(() => ctx.audio_progress_handler.call(audio));
 			if (ctx.buffered === void 0 || ctx.seekable === void 0) add_render_callback(() => ctx.audio_loadedmetadata_handler.call(audio));
+			if (ctx.played === void 0 || ctx.currentTime === void 0) add_render_callback(audio_timeupdate_handler);
+			if (ctx.duration === void 0) add_render_callback(() => ctx.audio_durationchange_handler.call(audio));
 
 			dispose = [
+				listen(audio, "progress", ctx.audio_progress_handler),
+				listen(audio, "loadedmetadata", ctx.audio_loadedmetadata_handler),
 				listen(audio, "timeupdate", audio_timeupdate_handler),
 				listen(audio, "durationchange", ctx.audio_durationchange_handler),
 				listen(audio, "play", ctx.audio_play_pause_handler),
 				listen(audio, "pause", ctx.audio_play_pause_handler),
-				listen(audio, "progress", ctx.audio_progress_handler),
-				listen(audio, "loadedmetadata", ctx.audio_loadedmetadata_handler),
 				listen(audio, "volumechange", ctx.audio_volumechange_handler),
 				listen(audio, "ratechange", ctx.audio_ratechange_handler)
 			];
@@ -53,10 +53,10 @@ function create_fragment(ctx) {
 
 		p(changed, ctx) {
 			if (!audio_updating && changed.currentTime && !isNaN(ctx.currentTime)) audio.currentTime = ctx.currentTime;
+			audio_updating = false;
 			if (changed.paused && audio_is_paused !== (audio_is_paused = ctx.paused)) audio[audio_is_paused ? "pause" : "play"]();
 			if (changed.volume && !isNaN(ctx.volume)) audio.volume = ctx.volume;
 			if (changed.playbackRate && !isNaN(ctx.playbackRate)) audio.playbackRate = ctx.playbackRate;
-			audio_updating = false;
 		},
 
 		i: noop,
@@ -75,6 +75,18 @@ function create_fragment(ctx) {
 function instance($$self, $$props, $$invalidate) {
 	let { buffered, seekable, played, currentTime, duration, paused, volume, playbackRate } = $$props;
 
+	function audio_progress_handler() {
+		buffered = time_ranges_to_array(this.buffered);
+		$$invalidate('buffered', buffered);
+	}
+
+	function audio_loadedmetadata_handler() {
+		buffered = time_ranges_to_array(this.buffered);
+		seekable = time_ranges_to_array(this.seekable);
+		$$invalidate('buffered', buffered);
+		$$invalidate('seekable', seekable);
+	}
+
 	function audio_timeupdate_handler() {
 		played = time_ranges_to_array(this.played);
 		currentTime = this.currentTime;
@@ -92,18 +104,6 @@ function instance($$self, $$props, $$invalidate) {
 		$$invalidate('paused', paused);
 	}
 
-	function audio_progress_handler() {
-		buffered = time_ranges_to_array(this.buffered);
-		$$invalidate('buffered', buffered);
-	}
-
-	function audio_loadedmetadata_handler() {
-		buffered = time_ranges_to_array(this.buffered);
-		seekable = time_ranges_to_array(this.seekable);
-		$$invalidate('buffered', buffered);
-		$$invalidate('seekable', seekable);
-	}
-
 	function audio_volumechange_handler() {
 		volume = this.volume;
 		$$invalidate('volume', volume);
@@ -134,11 +134,11 @@ function instance($$self, $$props, $$invalidate) {
 		paused,
 		volume,
 		playbackRate,
+		audio_progress_handler,
+		audio_loadedmetadata_handler,
 		audio_timeupdate_handler,
 		audio_durationchange_handler,
 		audio_play_pause_handler,
-		audio_progress_handler,
-		audio_loadedmetadata_handler,
 		audio_volumechange_handler,
 		audio_ratechange_handler
 	};
@@ -151,4 +151,4 @@ class Component extends SvelteComponent {
 	}
 }
 
-export default Component;
+export default Component;

test/runtime/samples/apply-directives-in-order/_config.js

@@ -0,0 +1,37 @@
+export default {
+	props: {
+		value: ''
+	},
+
+	html: `
+		<input>
+		<p></p>
+	`,
+
+	ssrHtml: `
+		<input>
+		<p></p>
+	`,
+
+	async test({ assert, component, target, window }) {
+		const input = target.querySelector('input');
+
+		const event = new window.Event('input');
+		input.value = 'h';
+		await input.dispatchEvent(event);
+
+		assert.equal(input.value, 'H');
+		assert.htmlEqual(target.innerHTML, `
+			<input>
+			<p>H</p>
+		`);
+
+		input.value = 'he';
+		await input.dispatchEvent(event);
+		assert.equal(input.value, 'HE');
+		assert.htmlEqual(target.innerHTML, `
+			<input>
+			<p>HE</p>
+		`);
+	},
+};

test/runtime/samples/apply-directives-in-order/main.svelte

@@ -0,0 +1,10 @@
+<script>
+	export let value;
+
+	function uppercase(event) {
+		event.target.value = event.target.value.toUpperCase()
+	}
+</script>
+
+<input on:input={uppercase} bind:value>
+<p>{value}</p>