From d38820f4ca20d8a9ae8e7ae76e7c0d9847cf9032 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Tue, 10 Dec 2024 00:17:46 +0100 Subject: [PATCH 1/3] fix: make `defaultValue` work with spread --- .changeset/silent-tips-cover.md | 5 + .../client/dom/elements/attributes.js | 14 +- .../form-default-value-spread/_config.js | 265 ++++++++++++++++++ .../form-default-value-spread/main.svelte | 199 +++++++++++++ 4 files changed, 482 insertions(+), 1 deletion(-) create mode 100644 .changeset/silent-tips-cover.md create mode 100644 packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/form-default-value-spread/main.svelte diff --git a/.changeset/silent-tips-cover.md b/.changeset/silent-tips-cover.md new file mode 100644 index 000000000000..1f51572cda24 --- /dev/null +++ b/.changeset/silent-tips-cover.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: make `defaultValue` work with spread diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index 0276069eee49..3eacf010673e 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -347,15 +347,27 @@ export function set_attributes( } else if (key === '__value' || (key === 'value' && value != null)) { // @ts-ignore element.value = element[key] = element.__value = value; + } else if (key === 'defaultValue') { + /** @type {HTMLInputElement} */ (element).defaultValue = value; + } else if (key === 'defaultChecked') { + /** @type {HTMLInputElement} */ (element).defaultChecked = value; + } else if (key === 'selected' && is_option_element) { + set_selected(/** @type {HTMLOptionElement} */ (element), value); } else { var name = key; if (!preserve_attribute_case) { name = normalize_attribute(name); } - if (value == null && !is_custom_element) { attributes[key] = null; + let default_value_reset = /**@type {HTMLInputElement}*/ (element).defaultValue; + let default_checked_reset = /**@type {HTMLInputElement}*/ (element).defaultChecked; element.removeAttribute(key); + if (key === 'value') { + /**@type {HTMLInputElement}*/ (element).defaultValue = default_value_reset; + } else if (key === 'checked') { + /**@type {HTMLInputElement}*/ (element).defaultChecked = default_checked_reset; + } } else if (setters.includes(name) && (is_custom_element || typeof value !== 'string')) { // @ts-ignore element[name] = value; diff --git a/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js b/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js new file mode 100644 index 000000000000..1ada598f443b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js @@ -0,0 +1,265 @@ +import { test } from '../../test'; +import { flushSync } from 'svelte'; + +export default test({ + async test({ assert, target }) { + /** + * @param {NodeListOf} inputs + * @param {string} field + * @param {any | any[]} value + */ + function check_inputs(inputs, field, value) { + for (let i = 0; i < inputs.length; i++) { + assert.equal(inputs[i][field], Array.isArray(value) ? value[i] : value, `field ${i}`); + } + } + + /** + * @param {any} input + * @param {string} field + * @param {any} value + */ + function set_input(input, field, value) { + input[field] = value; + input.dispatchEvent( + new Event(typeof value === 'boolean' ? 'change' : 'input', { bubbles: true }) + ); + } + + /** + * @param {HTMLOptionElement} option + */ + function select_option(option) { + option.selected = true; + option.dispatchEvent(new Event('change', { bubbles: true })); + } + + const after_reset = []; + + const reset = /** @type {HTMLInputElement} */ (target.querySelector('input[type=reset]')); + const [test1, test2, test3, test4, test5, test6, test7, test14] = + target.querySelectorAll('div'); + const [test8, test9, test10, test11] = target.querySelectorAll('select'); + const [ + test1_span, + test2_span, + test3_span, + test4_span, + test5_span, + test6_span, + test7_span, + test8_span, + test9_span, + test10_span, + test11_span + ] = target.querySelectorAll('span'); + + { + /** @type {NodeListOf} */ + const inputs = test1.querySelectorAll('input, textarea'); + check_inputs(inputs, 'value', 'x'); + assert.htmlEqual(test1_span.innerHTML, 'x x x x'); + + for (const input of inputs) { + set_input(input, 'value', 'foo'); + } + flushSync(); + check_inputs(inputs, 'value', 'foo'); + assert.htmlEqual(test1_span.innerHTML, 'foo foo foo foo'); + + after_reset.push(() => { + console.log('-------------'); + check_inputs(inputs, 'value', 'x'); + assert.htmlEqual(test1_span.innerHTML, 'x x x x'); + }); + } + + { + /** @type {NodeListOf} */ + const inputs = test2.querySelectorAll('input, textarea'); + check_inputs(inputs, 'value', 'x'); + assert.htmlEqual(test2_span.innerHTML, 'x x x x'); + + for (const input of inputs) { + set_input(input, 'value', 'foo'); + } + flushSync(); + check_inputs(inputs, 'value', 'foo'); + assert.htmlEqual(test2_span.innerHTML, 'foo foo foo foo'); + + after_reset.push(() => { + console.log('-------------'); + check_inputs(inputs, 'value', 'x'); + assert.htmlEqual(test2_span.innerHTML, 'x x x x'); + }); + } + + { + /** @type {NodeListOf} */ + const inputs = test3.querySelectorAll('input, textarea'); + check_inputs(inputs, 'value', 'y'); + assert.htmlEqual(test3_span.innerHTML, 'y y y y'); + + for (const input of inputs) { + set_input(input, 'value', 'foo'); + } + flushSync(); + check_inputs(inputs, 'value', 'foo'); + assert.htmlEqual(test3_span.innerHTML, 'foo foo foo foo'); + + after_reset.push(() => { + check_inputs(inputs, 'value', 'x'); + assert.htmlEqual(test3_span.innerHTML, 'x x x x'); + }); + } + + { + /** @type {NodeListOf} */ + const inputs = test4.querySelectorAll('input'); + check_inputs(inputs, 'checked', true); + assert.htmlEqual(test4_span.innerHTML, 'true true'); + + for (const input of inputs) { + set_input(input, 'checked', false); + } + flushSync(); + check_inputs(inputs, 'checked', false); + assert.htmlEqual(test4_span.innerHTML, 'false false'); + + after_reset.push(() => { + check_inputs(inputs, 'checked', true); + assert.htmlEqual(test4_span.innerHTML, 'true true'); + }); + } + + { + /** @type {NodeListOf} */ + const inputs = test5.querySelectorAll('input'); + check_inputs(inputs, 'checked', true); + assert.htmlEqual(test5_span.innerHTML, 'true true'); + + for (const input of inputs) { + set_input(input, 'checked', false); + } + flushSync(); + check_inputs(inputs, 'checked', false); + assert.htmlEqual(test5_span.innerHTML, 'false false'); + + after_reset.push(() => { + check_inputs(inputs, 'checked', true); + assert.htmlEqual(test5_span.innerHTML, 'true true'); + }); + } + + { + /** @type {NodeListOf} */ + const inputs = test6.querySelectorAll('input'); + check_inputs(inputs, 'checked', false); + assert.htmlEqual(test6_span.innerHTML, 'false false'); + + after_reset.push(() => { + check_inputs(inputs, 'checked', true); + assert.htmlEqual(test6_span.innerHTML, 'true true'); + }); + } + { + /** @type {NodeListOf} */ + const inputs = test7.querySelectorAll('input'); + check_inputs(inputs, 'checked', true); + assert.htmlEqual(test7_span.innerHTML, 'true'); + + after_reset.push(() => { + check_inputs(inputs, 'checked', false); + assert.htmlEqual(test7_span.innerHTML, 'false'); + }); + } + + { + /** @type {NodeListOf} */ + const options = test8.querySelectorAll('option'); + check_inputs(options, 'selected', [false, true, false]); + assert.htmlEqual(test8_span.innerHTML, 'b'); + + select_option(options[2]); + flushSync(); + check_inputs(options, 'selected', [false, false, true]); + assert.htmlEqual(test8_span.innerHTML, 'c'); + + after_reset.push(() => { + check_inputs(options, 'selected', [false, true, false]); + assert.htmlEqual(test8_span.innerHTML, 'b'); + }); + } + + { + /** @type {NodeListOf} */ + const options = test9.querySelectorAll('option'); + check_inputs(options, 'selected', [false, true, false]); + assert.htmlEqual(test9_span.innerHTML, 'b'); + + select_option(options[2]); + flushSync(); + check_inputs(options, 'selected', [false, false, true]); + assert.htmlEqual(test9_span.innerHTML, 'c'); + + after_reset.push(() => { + check_inputs(options, 'selected', [false, true, false]); + assert.htmlEqual(test9_span.innerHTML, 'b'); + }); + } + + { + /** @type {NodeListOf} */ + const options = test10.querySelectorAll('option'); + check_inputs(options, 'selected', [false, false, true]); + assert.htmlEqual(test10_span.innerHTML, 'c'); + + select_option(options[0]); + flushSync(); + check_inputs(options, 'selected', [true, false, false]); + assert.htmlEqual(test10_span.innerHTML, 'a'); + + after_reset.push(() => { + check_inputs(options, 'selected', [false, true, false]); + assert.htmlEqual(test10_span.innerHTML, 'b'); + }); + } + + { + /** @type {NodeListOf} */ + const options = test11.querySelectorAll('option'); + check_inputs(options, 'selected', [false, false, true]); + assert.htmlEqual(test11_span.innerHTML, 'c'); + + select_option(options[0]); + flushSync(); + check_inputs(options, 'selected', [true, false, false]); + assert.htmlEqual(test11_span.innerHTML, 'a'); + + after_reset.push(() => { + check_inputs(options, 'selected', [false, true, false]); + assert.htmlEqual(test11_span.innerHTML, 'b'); + }); + } + + { + /** @type {NodeListOf} */ + const inputs = test14.querySelectorAll('input, textarea'); + assert.equal(inputs[0].value, 'x'); + assert.equal(/** @type {HTMLInputElement} */ (inputs[1]).checked, true); + // this is still missing...i have no idea how to fix this lol + // assert.equal(inputs[2].value, 'x'); + + after_reset.push(() => { + assert.equal(inputs[0].value, 'y'); + assert.equal(/** @type {HTMLInputElement} */ (inputs[1]).checked, false); + assert.equal(inputs[2].value, 'y'); + }); + } + + reset.click(); + await Promise.resolve(); + flushSync(); + after_reset.forEach((fn) => fn()); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/main.svelte b/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/main.svelte new file mode 100644 index 000000000000..3fc7ef11a5e9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/main.svelte @@ -0,0 +1,199 @@ + + +
+

Input/Textarea value

+ +
+ + + + + + + + +
+ + +
+ + + + + + + + +
+ + +
+ + + + + + + + +
+ +

Input checked

+ +
+ + + + +
+ + +
+ + + + +
+ + +
+ + + + +
+ + +
+ + +
+ + + +

Select (single)

+ + + + + + + + + + + + +

Select (multiple)

+ + + + + + + + +

Static values

+
+ + + +
+ + +
+ +

+ Bound values: + {value1} {value3} {value6} {value8} + {value9} {value12} {value14} {value16} + {value17} {value20} {value22} {value24} + {checked2} {checked4} + {checked6} {checked8} + {checked10} {checked12} + {checked14} + {selected1} + {selected2} + {selected3} + {selected4} + {selected5} + {selected6} +

From ce373ff8fc31d7544aae8090960e5abd3ebb3acb Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Tue, 10 Dec 2024 10:50:56 +0100 Subject: [PATCH 2/3] chore: apply suggestions from review --- .../internal/client/dom/elements/attributes.js | 15 +++++++++------ .../samples/form-default-value-spread/_config.js | 2 -- .../samples/form-default-value/_config.js | 2 -- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index 3eacf010673e..ec57917e7696 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -347,10 +347,6 @@ export function set_attributes( } else if (key === '__value' || (key === 'value' && value != null)) { // @ts-ignore element.value = element[key] = element.__value = value; - } else if (key === 'defaultValue') { - /** @type {HTMLInputElement} */ (element).defaultValue = value; - } else if (key === 'defaultChecked') { - /** @type {HTMLInputElement} */ (element).defaultChecked = value; } else if (key === 'selected' && is_option_element) { set_selected(/** @type {HTMLOptionElement} */ (element), value); } else { @@ -358,8 +354,12 @@ export function set_attributes( if (!preserve_attribute_case) { name = normalize_attribute(name); } - if (value == null && !is_custom_element) { + let is_default_value_or_checked = name === 'defaultValue' || name === 'defaultChecked'; + + if (value == null && !is_custom_element && !is_default_value_or_checked) { attributes[key] = null; + // if we remove the value/checked attributes this also for some reasons reset + // the default value so we need to keep track of it and reassign it after the remove let default_value_reset = /**@type {HTMLInputElement}*/ (element).defaultValue; let default_checked_reset = /**@type {HTMLInputElement}*/ (element).defaultChecked; element.removeAttribute(key); @@ -368,7 +368,10 @@ export function set_attributes( } else if (key === 'checked') { /**@type {HTMLInputElement}*/ (element).defaultChecked = default_checked_reset; } - } else if (setters.includes(name) && (is_custom_element || typeof value !== 'string')) { + } else if ( + is_default_value_or_checked || + (setters.includes(name) && (is_custom_element || typeof value !== 'string')) + ) { // @ts-ignore element[name] = value; } else if (typeof value !== 'function') { diff --git a/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js b/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js index 1ada598f443b..26e90e431b54 100644 --- a/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/form-default-value-spread/_config.js @@ -68,7 +68,6 @@ export default test({ assert.htmlEqual(test1_span.innerHTML, 'foo foo foo foo'); after_reset.push(() => { - console.log('-------------'); check_inputs(inputs, 'value', 'x'); assert.htmlEqual(test1_span.innerHTML, 'x x x x'); }); @@ -88,7 +87,6 @@ export default test({ assert.htmlEqual(test2_span.innerHTML, 'foo foo foo foo'); after_reset.push(() => { - console.log('-------------'); check_inputs(inputs, 'value', 'x'); assert.htmlEqual(test2_span.innerHTML, 'x x x x'); }); diff --git a/packages/svelte/tests/runtime-runes/samples/form-default-value/_config.js b/packages/svelte/tests/runtime-runes/samples/form-default-value/_config.js index 5ef72aaa8ec2..35ab6e8ece44 100644 --- a/packages/svelte/tests/runtime-runes/samples/form-default-value/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/form-default-value/_config.js @@ -68,7 +68,6 @@ export default test({ assert.htmlEqual(test1_span.innerHTML, 'foo foo foo foo'); after_reset.push(() => { - console.log('-------------'); check_inputs(inputs, 'value', 'x'); assert.htmlEqual(test1_span.innerHTML, 'x x x x'); }); @@ -88,7 +87,6 @@ export default test({ assert.htmlEqual(test2_span.innerHTML, 'foo foo foo foo'); after_reset.push(() => { - console.log('-------------'); check_inputs(inputs, 'value', 'x'); assert.htmlEqual(test2_span.innerHTML, 'x x x x'); }); From 95b87ff59614562803f236adfb916a77a0b70070 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 10 Dec 2024 17:03:31 +0100 Subject: [PATCH 3/3] chore: more default value spread work This is a WIP, which I'm not sure if it goes anywhere. It started out to deeper understand the fix in #14640, and to fix some more theoretical loopholes, but this turns out to not fix the issue yet, and I'm not sure if it even will, so I'm punting on it for now but putting it up for others to see. --- .../server/visitors/shared/element.js | 34 ++++++++--- .../client/dom/elements/attributes.js | 56 +++++++++++++------ packages/svelte/src/internal/server/index.js | 16 ++++++ 3 files changed, 82 insertions(+), 24 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js index d626bb08db30..2838a30f6d6e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js @@ -46,7 +46,7 @@ export function build_element_attributes(node, context) { /** @type {Expression | null} */ let content = null; - let has_spread = false; + let needs_spread = false; // Use the index to keep the attributes order which is important for spreading let class_index = -1; let style_index = -1; @@ -82,8 +82,28 @@ export function build_element_attributes(node, context) { ) { events_to_capture.add(attribute.name); } - // the defaultValue/defaultChecked properties don't exist as attributes - } else if (attribute.name !== 'defaultValue' && attribute.name !== 'defaultChecked') { + } else if ( + (node.name === 'input' || node.name === 'textarea') && + (attribute.name === 'defaultValue' || attribute.name === 'defaultChecked') + ) { + // If there's a defaultValue but not value attribute, we turn it into a value attribute (same for checked). + // If we can't, then we need to use a spread in order to be able to detect at runtime whether or not + // the value/checked value is nullish, in which case defaultValue/defaultChecked should be used. + const replacement_name = attribute.name === 'defaultValue' ? 'value' : 'checked'; + if ( + !node.attributes.some( + (attr) => + attr.type === 'SpreadAttribute' || + ((attr.type === 'BindDirective' || attr.type === 'Attribute') && + attr.name === replacement_name) + ) + ) { + attributes.push({ ...attribute, name: replacement_name }); + } else { + needs_spread = true; + attributes.push(attribute); + } + } else { if (attribute.name === 'class') { class_index = attributes.length; } else if (attribute.name === 'style') { @@ -173,7 +193,7 @@ export function build_element_attributes(node, context) { } } else if (attribute.type === 'SpreadAttribute') { attributes.push(attribute); - has_spread = true; + needs_spread = true; if (is_load_error_element(node.name)) { events_to_capture.add('onload'); events_to_capture.add('onerror'); @@ -194,7 +214,7 @@ export function build_element_attributes(node, context) { } } - if (class_directives.length > 0 && !has_spread) { + if (class_directives.length > 0 && !needs_spread) { const class_attribute = build_class_directives( class_directives, /** @type {AST.Attribute | null} */ (attributes[class_index] ?? null) @@ -204,7 +224,7 @@ export function build_element_attributes(node, context) { } } - if (style_directives.length > 0 && !has_spread) { + if (style_directives.length > 0 && !needs_spread) { build_style_directives( style_directives, /** @type {AST.Attribute | null} */ (attributes[style_index] ?? null), @@ -215,7 +235,7 @@ export function build_element_attributes(node, context) { } } - if (has_spread) { + if (needs_spread) { build_element_spread_attributes(node, attributes, style_directives, class_directives, context); } else { for (const attribute of /** @type {AST.Attribute[]} */ (attributes)) { diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index ec57917e7696..9a219ae6d03c 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -344,36 +344,58 @@ export function set_attributes( element.style.cssText = value + ''; } else if (key === 'autofocus') { autofocus(/** @type {HTMLElement} */ (element), Boolean(value)); - } else if (key === '__value' || (key === 'value' && value != null)) { - // @ts-ignore - element.value = element[key] = element.__value = value; - } else if (key === 'selected' && is_option_element) { + } else if ( + key === '__value' || + (key === 'value' && + // For element we don't want to fall through to removeAttribute below, + // because that attribute corresponds to the defaultValue, not the value property + (value != null || !is_custom_element)) + ) { + set_value(element, value); + } else if (key === 'checked' && (value != null || !is_custom_element)) { + set_checked(element, value); + } else if (is_option_element && key === 'selected') { set_selected(/** @type {HTMLOptionElement} */ (element), value); } else { var name = key; if (!preserve_attribute_case) { name = normalize_attribute(name); } - let is_default_value_or_checked = name === 'defaultValue' || name === 'defaultChecked'; - if (value == null && !is_custom_element && !is_default_value_or_checked) { + if (value == null && !is_custom_element && key !== 'checked') { attributes[key] = null; - // if we remove the value/checked attributes this also for some reasons reset - // the default value so we need to keep track of it and reassign it after the remove - let default_value_reset = /**@type {HTMLInputElement}*/ (element).defaultValue; - let default_checked_reset = /**@type {HTMLInputElement}*/ (element).defaultChecked; element.removeAttribute(key); - if (key === 'value') { - /**@type {HTMLInputElement}*/ (element).defaultValue = default_value_reset; - } else if (key === 'checked') { - /**@type {HTMLInputElement}*/ (element).defaultChecked = default_checked_reset; - } } else if ( - is_default_value_or_checked || - (setters.includes(name) && (is_custom_element || typeof value !== 'string')) + // is_default_value_or_checked || + setters.includes(name) && + (is_custom_element || + typeof value !== 'string' || + name === 'defaultValue' || + name === 'defaultChecked') ) { + // if we adjust the value/checked attributes this also for some reasons reset + // the default value so we need to keep track of it and reassign it after the remove + let default_value_reset; + let default_checked_reset; + if (hydrating) { + default_value_reset = /**@type {HTMLInputElement}*/ (element).value; + default_checked_reset = /**@type {HTMLInputElement}*/ (element).checked; + } + // @ts-ignore element[name] = value; + + if (hydrating) { + if (key === 'defaultValue') { + /**@type {HTMLInputElement}*/ (element).value = /** @type {string} */ ( + default_value_reset + ); + } else if (key === 'defaultChecked') { + /**@type {HTMLInputElement}*/ (element).checked = /** @type {boolean} */ ( + default_checked_reset + ); + } + } } else if (typeof value !== 'function') { if (hydrating && (name === 'src' || name === 'href' || name === 'srcset')) { if (!skip_warning) check_src_in_dev_hydration(element, name, value ?? ''); diff --git a/packages/svelte/src/internal/server/index.js b/packages/svelte/src/internal/server/index.js index b944c602b884..08b8472bcd6b 100644 --- a/packages/svelte/src/internal/server/index.js +++ b/packages/svelte/src/internal/server/index.js @@ -224,6 +224,22 @@ export function spread_attributes(attrs, classes, styles, flags = 0) { var value = attrs[name]; + if (is_html) { + if (name === 'defaultvalue') { + if (attrs.value == null) { + name = 'value'; + } else { + continue; + } + } else if (name === 'defaultchecked') { + if (attrs.checked == null) { + name = 'checked'; + } else { + continue; + } + } + } + if (lowercase) { name = name.toLowerCase(); }