From 402106330530965ff5a1a5f05fc6529403486328 Mon Sep 17 00:00:00 2001
From: Tan Li Hau <tanhauhau@users.noreply.github.com>
Date: Wed, 8 Jul 2020 01:50:35 +0800
Subject: [PATCH] spread condition for input element (#5004)

---
 CHANGELOG.md                                  |   1 +
 .../render_dom/wrappers/Element/Attribute.ts  | 177 +++++++++++-------
 .../wrappers/Element/SpreadAttribute.ts       |   3 +
 .../render_dom/wrappers/Element/index.ts      |  27 +--
 .../InputOne.svelte                           |  18 ++
 .../InputTwo.svelte                           |  19 ++
 .../spread-element-input-value/_config.js     |  62 ++++++
 .../spread-element-input-value/main.svelte    |   9 +
 .../spread-element-input-value/utils.js       |   6 +
 9 files changed, 240 insertions(+), 82 deletions(-)
 create mode 100644 src/compiler/compile/render_dom/wrappers/Element/SpreadAttribute.ts
 create mode 100644 test/runtime/samples/spread-element-input-value/InputOne.svelte
 create mode 100644 test/runtime/samples/spread-element-input-value/InputTwo.svelte
 create mode 100644 test/runtime/samples/spread-element-input-value/_config.js
 create mode 100644 test/runtime/samples/spread-element-input-value/main.svelte
 create mode 100644 test/runtime/samples/spread-element-input-value/utils.js

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0088e4b0b3..d6fcc626c8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 * Support nullish coalescing (`??`) and optional chaining (`?.`) operators ([#1972](https://github.com/sveltejs/svelte/issues/1972))
 * Support `import.meta` ([#4379](https://github.com/sveltejs/svelte/issues/4379))
+* Fix only setting `<input>` values when they're changed when there are spread attributes ([#4418](https://github.com/sveltejs/svelte/issues/4418))
 * Fix placement of `{@html}` when used at the root of a slot, at the root of a component, or in `<svelte:head>` ([#5012](https://github.com/sveltejs/svelte/issues/5012), [#5071](https://github.com/sveltejs/svelte/pull/5071))
 * Fix certain handling of two-way bound `contenteditable` elements ([#5018](https://github.com/sveltejs/svelte/issues/5018))
 * Fix handling of `import`ed value that is used as a store and is also mutated ([#5019](https://github.com/sveltejs/svelte/issues/5019))
diff --git a/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts b/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts
index 86b67ca47e..ec281648b8 100644
--- a/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts
+++ b/src/compiler/compile/render_dom/wrappers/Element/Attribute.ts
@@ -7,8 +7,9 @@ import { b, x } from 'code-red';
 import Expression from '../../../nodes/shared/Expression';
 import Text from '../../../nodes/Text';
 import handle_select_value_binding from './handle_select_value_binding';
+import { Identifier, Node } from 'estree';
 
-export default class AttributeWrapper {
+export class BaseAttributeWrapper {
 	node: Attribute;
 	parent: ElementWrapper;
 
@@ -21,7 +22,29 @@ export default class AttributeWrapper {
 			parent.not_static_content();
 
 			block.add_dependencies(node.dependencies);
+		}
+	}
+
+	render(_block: Block) {}
+}
 
+export default class AttributeWrapper extends BaseAttributeWrapper {
+	node: Attribute;
+	parent: ElementWrapper;
+	metadata: any;
+	name: string;
+	property_name: string;
+	is_indirectly_bound_value: boolean;
+	is_src: boolean;
+	is_select_value_attribute: boolean;
+	is_input_value: boolean;
+	should_cache: boolean;
+	last: Identifier;
+
+	constructor(parent: ElementWrapper, block: Block, node: Attribute) {
+		super(parent, block, node);
+
+		if (node.dependencies.size > 0) {
 			// special case — <option value={foo}> — see below
 			if (this.parent.node.name === 'option' && node.name === 'value') {
 				let select: ElementWrapper = this.parent;
@@ -42,31 +65,22 @@ export default class AttributeWrapper {
 				handle_select_value_binding(this, node.dependencies);
 			}
 		}
-	}
 
-	is_indirectly_bound_value() {
-		const element = this.parent;
-		const name = fix_attribute_casing(this.node.name);
-		return name === 'value' &&
-			(element.node.name === 'option' || // TODO check it's actually bound
-				(element.node.name === 'input' &&
-					element.node.bindings.some(
-						(binding) =>
-							/checked|group/.test(binding.name)
-					)));
+		this.name = fix_attribute_casing(this.node.name);
+		this.metadata = this.get_metadata();
+		this.is_indirectly_bound_value = is_indirectly_bound_value(this);
+		this.property_name = this.is_indirectly_bound_value
+			? '__value'
+			: this.metadata && this.metadata.property_name;
+		this.is_src = this.name === 'src'; // TODO retire this exception in favour of https://github.com/sveltejs/svelte/issues/3750
+		this.is_select_value_attribute = this.name === 'value' && this.parent.node.name === 'select';
+		this.is_input_value = this.name === 'value' && this.parent.node.name === 'input';
+		this.should_cache = should_cache(this);
 	}
 
 	render(block: Block) {
 		const element = this.parent;
-		const name = fix_attribute_casing(this.node.name);
-
-		const metadata = this.get_metadata();
-
-		const is_indirectly_bound_value = this.is_indirectly_bound_value();
-
-		const property_name = is_indirectly_bound_value
-			? '__value'
-			: metadata && metadata.property_name;
+		const { name, property_name, should_cache, is_indirectly_bound_value } = this;
 
 		// xlink is a special case... we could maybe extend this to generic
 		// namespaced attributes but I'm not sure that's applicable in
@@ -82,29 +96,15 @@ export default class AttributeWrapper {
 		const dependencies = this.get_dependencies();
 		const value = this.get_value(block);
 
-		const is_src = this.node.name === 'src'; // TODO retire this exception in favour of https://github.com/sveltejs/svelte/issues/3750
-		const is_select_value_attribute =
-			name === 'value' && element.node.name === 'select';
-
-		const is_input_value = name === 'value' && element.node.name === 'input';
-
-		const should_cache = is_src || this.node.should_cache();
-
-		const last = should_cache && block.get_unique_name(
-			`${element.var.name}_${name.replace(/[^a-zA-Z_$]/g, '_')}_value`
-		);
-
-		if (should_cache) block.add_variable(last);
-
 		let updater;
-		const init = should_cache ? x`${last} = ${value}` : value;
+		const init = this.get_init(block, value);
 
 		if (is_legacy_input_type) {
 			block.chunks.hydrate.push(
 				b`@set_input_type(${element.var}, ${init});`
 			);
-			updater = b`@set_input_type(${element.var}, ${should_cache ? last : value});`;
-		} else if (is_select_value_attribute) {
+			updater = b`@set_input_type(${element.var}, ${should_cache ? this.last : value});`;
+		} else if (this.is_select_value_attribute) {
 			// annoying special case
 			const is_multiple_select = element.node.get_static_attribute_value('multiple');
 
@@ -117,45 +117,37 @@ export default class AttributeWrapper {
 			block.chunks.mount.push(b`
 				${updater}
 			`);
-		} else if (is_src) {
+		} else if (this.is_src) {
 			block.chunks.hydrate.push(
-				b`if (${element.var}.src !== ${init}) ${method}(${element.var}, "${name}", ${last});`
+				b`if (${element.var}.src !== ${init}) ${method}(${element.var}, "${name}", ${this.last});`
 			);
-			updater = b`${method}(${element.var}, "${name}", ${should_cache ? last : value});`;
+			updater = b`${method}(${element.var}, "${name}", ${should_cache ? this.last : value});`;
 		} else if (property_name) {
 			block.chunks.hydrate.push(
 				b`${element.var}.${property_name} = ${init};`
 			);
 			updater = block.renderer.options.dev
-				? b`@prop_dev(${element.var}, "${property_name}", ${should_cache ? last : value});`
-				: b`${element.var}.${property_name} = ${should_cache ? last : value};`;
+				? b`@prop_dev(${element.var}, "${property_name}", ${should_cache ? this.last : value});`
+				: b`${element.var}.${property_name} = ${should_cache ? this.last : value};`;
 		} else {
 			block.chunks.hydrate.push(
 				b`${method}(${element.var}, "${name}", ${init});`
 			);
-			updater = b`${method}(${element.var}, "${name}", ${should_cache ? last : value});`;
+			updater = b`${method}(${element.var}, "${name}", ${should_cache ? this.last : value});`;
 		}
 
-		if (dependencies.length > 0) {
-			let condition = block.renderer.dirty(dependencies);
-
-			if (should_cache) {
-				condition = is_src
-					? x`${condition} && (${element.var}.src !== (${last} = ${value}))`
-					: x`${condition} && (${last} !== (${last} = ${value}))`;
-			}
-
-			if (is_input_value) {
-				const type = element.node.get_static_attribute_value('type');
+		if (is_indirectly_bound_value) {
+			const update_value = b`${element.var}.value = ${element.var}.__value;`;
+			block.chunks.hydrate.push(update_value);
 
-				if (type === null || type === "" || type === "text" || type === "email" || type === "password") {
-					condition = x`${condition} && ${element.var}.${property_name} !== ${should_cache ? last : value}`;
-				}
-			}
+			updater = b`
+				${updater}
+				${update_value};
+			`;
+		}
 
-			if (block.has_outros) {
-				condition = x`!#current || ${condition}`;
-			}
+		if (dependencies.length > 0) {
+			const condition = this.get_dom_update_conditions(block, block.renderer.dirty(dependencies));
 
 			block.chunks.update.push(b`
 				if (${condition}) {
@@ -167,13 +159,44 @@ export default class AttributeWrapper {
 		if (this.node.is_true && name === 'autofocus') {
 			block.autofocus = element.var;
 		}
+	}
 
-		if (is_indirectly_bound_value) {
-			const update_value = b`${element.var}.value = ${element.var}.__value;`;
+	get_init(block: Block, value) {
+		this.last = this.should_cache && block.get_unique_name(
+			`${this.parent.var.name}_${this.name.replace(/[^a-zA-Z_$]/g, '_')}_value`
+		);
 
-			block.chunks.hydrate.push(update_value);
-			if (dependencies.length > 0) block.chunks.update.push(update_value);
+		if (this.should_cache) block.add_variable(this.last);
+
+		return this.should_cache ? x`${this.last} = ${value}` : value;
+	}
+
+	get_dom_update_conditions(block: Block, dependency_condition: Node) {
+		const { property_name, should_cache, last } = this;
+		const element = this.parent;
+		const value = this.get_value(block);
+
+		let condition = dependency_condition;
+
+		if (should_cache) {
+			condition = this.is_src
+				? x`${condition} && (${element.var}.src !== (${last} = ${value}))`
+				: x`${condition} && (${last} !== (${last} = ${value}))`;
+		}
+
+		if (this.is_input_value) {
+			const type = element.node.get_static_attribute_value('type');
+
+			if (type === null || type === "" || type === "text" || type === "email" || type === "password") {
+				condition = x`${condition} && ${element.var}.${property_name} !== ${should_cache ? last : value}`;
+			}
+		}
+
+		if (block.has_outros) {
+			condition = x`!#current || ${condition}`;
 		}
+
+		return condition;
 	}
 
 	get_dependencies() {
@@ -194,15 +217,14 @@ export default class AttributeWrapper {
 
 	get_metadata() {
 		if (this.parent.node.namespace) return null;
-		const metadata = attribute_lookup[fix_attribute_casing(this.node.name)];
+		const metadata = attribute_lookup[this.name];
 		if (metadata && metadata.applies_to && !metadata.applies_to.includes(this.parent.node.name)) return null;
 		return metadata;
 	}
 
 	get_value(block) {
 		if (this.node.is_true) {
-			const metadata = this.get_metadata();
-			if (metadata && boolean_attribute.has(metadata.property_name.toLowerCase())) {
+			if (this.metadata && boolean_attribute.has(this.metadata.property_name.toLowerCase())) {
 				return x`true`;
 			}
 			return x`""`;
@@ -350,4 +372,19 @@ const boolean_attribute = new Set([
 	'required',
 	'reversed',
 	'selected'
-]);
\ No newline at end of file
+]);
+
+function should_cache(attribute: AttributeWrapper) {
+	return attribute.is_src || attribute.node.should_cache();
+}
+
+function is_indirectly_bound_value(attribute: AttributeWrapper) {
+	const element = attribute.parent;
+	return attribute.name === 'value' &&
+		(element.node.name === 'option' || // TODO check it's actually bound
+			(element.node.name === 'input' &&
+				element.node.bindings.some(
+					(binding) =>
+						/checked|group/.test(binding.name)
+				)));
+}
\ No newline at end of file
diff --git a/src/compiler/compile/render_dom/wrappers/Element/SpreadAttribute.ts b/src/compiler/compile/render_dom/wrappers/Element/SpreadAttribute.ts
new file mode 100644
index 0000000000..a47ddc892b
--- /dev/null
+++ b/src/compiler/compile/render_dom/wrappers/Element/SpreadAttribute.ts
@@ -0,0 +1,3 @@
+import { BaseAttributeWrapper } from "./Attribute";
+
+export default class SpreadAttributeWrapper extends BaseAttributeWrapper {}
diff --git a/src/compiler/compile/render_dom/wrappers/Element/index.ts b/src/compiler/compile/render_dom/wrappers/Element/index.ts
index 148f837ee6..f11ce63023 100644
--- a/src/compiler/compile/render_dom/wrappers/Element/index.ts
+++ b/src/compiler/compile/render_dom/wrappers/Element/index.ts
@@ -11,6 +11,7 @@ import { b, x, p } from 'code-red';
 import { namespaces } from '../../../../utils/namespaces';
 import AttributeWrapper from './Attribute';
 import StyleAttributeWrapper from './StyleAttribute';
+import SpreadAttributeWrapper from './SpreadAttribute';
 import { dimensions } from '../../../../utils/patterns';
 import Binding from './Binding';
 import InlineComponentWrapper from '../InlineComponent';
@@ -137,7 +138,7 @@ const events = [
 export default class ElementWrapper extends Wrapper {
 	node: Element;
 	fragment: FragmentWrapper;
-	attributes: AttributeWrapper[];
+	attributes: Array<AttributeWrapper | StyleAttributeWrapper | SpreadAttributeWrapper>;
 	bindings: Binding[];
 	event_handlers: EventHandler[];
 	class_dependencies: string[];
@@ -221,6 +222,9 @@ export default class ElementWrapper extends Wrapper {
 			if (attribute.name === 'style') {
 				return new StyleAttributeWrapper(this, block, attribute);
 			}
+			if (attribute.type === 'Spread') {
+				return new SpreadAttributeWrapper(this, block, attribute);
+			}
 			return new AttributeWrapper(this, block, attribute);
 		});
 
@@ -418,7 +422,7 @@ export default class ElementWrapper extends Wrapper {
 			return x`@_document.createElementNS("${namespace}", "${name}")`;
 		}
 
-		const is = this.attributes.find(attr => attr.node.name === 'is');
+		const is: AttributeWrapper = this.attributes.find(attr => attr.node.name === 'is') as any;
 		if (is) {
 			return x`@element_is("${name}", ${is.render_chunks(block).reduce((lhs, rhs) => x`${lhs} + ${rhs}`)})`;
 		}
@@ -664,25 +668,24 @@ export default class ElementWrapper extends Wrapper {
 
 		this.attributes
 			.forEach(attr => {
-				const condition = attr.node.dependencies.size > 0
-					? block.renderer.dirty(Array.from(attr.node.dependencies))
+				const dependencies = attr.node.get_dependencies();
+
+				const condition = dependencies.length > 0
+					? block.renderer.dirty(dependencies)
 					: null;
 
-				if (attr.node.is_spread) {
+				if (attr instanceof SpreadAttributeWrapper) {
 					const snippet = attr.node.expression.manipulate(block);
 
 					initial_props.push(snippet);
 
 					updates.push(condition ? x`${condition} && ${snippet}` : snippet);
 				} else {
-					const metadata = attr.get_metadata();
-					const name = attr.is_indirectly_bound_value()
-						? '__value'
-						: (metadata && metadata.property_name) || fix_attribute_casing(attr.node.name);
-					const snippet = x`{ ${name}: ${attr.get_value(block)} }`;
-					initial_props.push(snippet);
+					const name = attr.property_name || attr.name;
+					initial_props.push(x`{ ${name}: ${attr.get_init(block, attr.get_value(block))} }`);
+					const snippet = x`{ ${name}: ${attr.should_cache ? attr.last : attr.get_value(block)} }`;
 
-					updates.push(condition ? x`${condition} && ${snippet}` : snippet);
+					updates.push(condition ? x`${attr.get_dom_update_conditions(block, condition)} && ${snippet}` : snippet);
 				}
 			});
 
diff --git a/test/runtime/samples/spread-element-input-value/InputOne.svelte b/test/runtime/samples/spread-element-input-value/InputOne.svelte
new file mode 100644
index 0000000000..92ecb7b9a9
--- /dev/null
+++ b/test/runtime/samples/spread-element-input-value/InputOne.svelte
@@ -0,0 +1,18 @@
+<script>
+  import { omit } from './utils.js';
+
+  export let value;
+	
+	function onInput(e) {  
+    value = e.target.value;
+  }
+
+  $: props = omit($$props, 'value');
+</script>
+
+<input
+	type="text"
+	{...props}
+	on:input={onInput}
+	{value}
+/>
diff --git a/test/runtime/samples/spread-element-input-value/InputTwo.svelte b/test/runtime/samples/spread-element-input-value/InputTwo.svelte
new file mode 100644
index 0000000000..33ec0622a4
--- /dev/null
+++ b/test/runtime/samples/spread-element-input-value/InputTwo.svelte
@@ -0,0 +1,19 @@
+<script>
+  import { omit } from './utils.js';
+
+  export let value;
+	
+	function onInput(e) {  
+    value = e.target.value;
+  }
+
+  $: props = omit($$props, 'value', 'minlength');
+</script>
+	
+<input
+	type="text"
+	minlength="10"
+	value={value}
+  {...props}
+	on:input={onInput}
+/>
diff --git a/test/runtime/samples/spread-element-input-value/_config.js b/test/runtime/samples/spread-element-input-value/_config.js
new file mode 100644
index 0000000000..a07de82419
--- /dev/null
+++ b/test/runtime/samples/spread-element-input-value/_config.js
@@ -0,0 +1,62 @@
+export default {
+	async test({ assert, component, target, window }) {
+		const [input1, input2] = target.querySelectorAll("input");
+
+		// we are not able emulate user interaction in jsdom,
+		// therefore, jsdom could not validate minlength / maxlength
+
+		// we simulate user input with
+		// setting input.value + dispathEvent
+
+		// and we determine if svelte does not set the `input.value` again by
+		// spying on the setter of `input.value`
+
+		const spy1 = spyOnValueSetter(input1, input1.value);
+		const spy2 = spyOnValueSetter(input2, input2.value);
+		
+		const event = new window.Event("input");
+		
+		input1.value = '12345';
+		spy1.reset();
+		await input1.dispatchEvent(event);
+
+		assert.ok(!spy1.isSetCalled());
+		
+		input2.value = '12345';
+		spy2.reset();
+		await input2.dispatchEvent(event);
+
+		assert.ok(!spy2.isSetCalled());
+
+		spy1.reset();
+		component.val1 = '56789';
+		assert.ok(spy1.isSetCalled());
+
+		spy2.reset();
+		component.val2 = '56789';
+		assert.ok(spy2.isSetCalled());
+	},
+};
+
+function spyOnValueSetter(input, initialValue) {
+	let value = initialValue;
+	let isSet = false;
+	Object.defineProperty(input, "value", {
+		get() {
+			return value;
+		},
+		set(_value) {
+			value = _value;
+			isSet = true;
+		},
+	});
+
+	return {
+		isSetCalled() {
+			return isSet;
+		},
+		reset() {
+			isSet = false;
+		},
+	};
+}
diff --git a/test/runtime/samples/spread-element-input-value/main.svelte b/test/runtime/samples/spread-element-input-value/main.svelte
new file mode 100644
index 0000000000..bb5f0e00bf
--- /dev/null
+++ b/test/runtime/samples/spread-element-input-value/main.svelte
@@ -0,0 +1,9 @@
+<script>
+	import InputOne from './InputOne.svelte';
+	import InputTwo from './InputTwo.svelte';
+	export let val1 = '';
+	export let val2 = '';
+</script>
+
+<InputOne bind:value={val1} required minlength="10" />
+<InputTwo bind:value={val2} required minlength="10" />
\ No newline at end of file
diff --git a/test/runtime/samples/spread-element-input-value/utils.js b/test/runtime/samples/spread-element-input-value/utils.js
new file mode 100644
index 0000000000..ee941bda55
--- /dev/null
+++ b/test/runtime/samples/spread-element-input-value/utils.js
@@ -0,0 +1,6 @@
+export function omit(obj, ...keysToOmit) {
+	return Object.keys(obj).reduce((acc, key) => {
+		if (keysToOmit.indexOf(key) === -1) acc[key] = obj[key];
+		return acc;
+	}, {});
+}