From 6575151e9a3366cb85029d85fb50281561d01013 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sat, 4 May 2024 18:26:54 +0200 Subject: [PATCH 1/7] fix: fine grained hydration attribute removal --- .changeset/loud-numbers-flow.md | 5 +++ .../3-transform/client/visitors/template.js | 31 ++++++++++++------- .../client/dom/elements/attributes.js | 7 +++-- .../_config.js | 12 +++++++ .../main.svelte | 5 +++ 5 files changed, 45 insertions(+), 15 deletions(-) create mode 100644 .changeset/loud-numbers-flow.md create mode 100644 packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js create mode 100644 packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/main.svelte diff --git a/.changeset/loud-numbers-flow.md b/.changeset/loud-numbers-flow.md new file mode 100644 index 000000000000..e04df7d6e503 --- /dev/null +++ b/.changeset/loud-numbers-flow.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: fine grained hydration attribute removal diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index b6fe9649eb3d..c6afeeab6108 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1838,7 +1838,10 @@ export const template_visitors = { const lets = []; const is_custom_element = is_custom_element_node(node); - let needs_input_reset = false; + /** + * @type {import('estree').Expression[]} + */ + let needs_input_reset = []; let needs_content_reset = false; /** @type {import('#compiler').BindDirective | null} */ @@ -1860,11 +1863,11 @@ export const template_visitors = { for (const attribute of node.attributes) { if (attribute.type === 'Attribute') { attributes.push(attribute); - if ( - (attribute.name === 'value' || attribute.name === 'checked') && - !is_text_attribute(attribute) - ) { - needs_input_reset = true; + if (attribute.name === 'value' && !is_text_attribute(attribute)) { + needs_input_reset.push(b.literal('value')); + needs_content_reset = true; + } else if (attribute.name === 'checked' && !is_text_attribute(attribute)) { + needs_input_reset.push(b.literal('checked')); needs_content_reset = true; } else if ( attribute.name === 'contenteditable' && @@ -1875,7 +1878,7 @@ export const template_visitors = { } } else if (attribute.type === 'SpreadAttribute') { attributes.push(attribute); - needs_input_reset = true; + needs_input_reset = [b.literal('value'), b.literal('checked')]; needs_content_reset = true; } else if (attribute.type === 'ClassDirective') { class_directives.push(attribute); @@ -1887,11 +1890,11 @@ export const template_visitors = { if (attribute.type === 'BindDirective') { if (attribute.name === 'group' || attribute.name === 'checked') { needs_special_value_handling = true; - needs_input_reset = true; + needs_input_reset.push(b.literal('checked')); } else if (attribute.name === 'value') { value_binding = attribute; needs_content_reset = true; - needs_input_reset = true; + needs_input_reset.push(b.literal('value')); } else if ( attribute.name === 'innerHTML' || attribute.name === 'innerText' || @@ -1907,7 +1910,7 @@ export const template_visitors = { if (child_metadata.namespace === 'foreign') { // input/select etc could mean something completely different in foreign namespace, so don't special-case them needs_content_reset = false; - needs_input_reset = false; + needs_input_reset = []; needs_special_value_handling = false; value_binding = null; } @@ -1916,8 +1919,12 @@ export const template_visitors = { child_metadata.bound_contenteditable = true; } - if (needs_input_reset && (node.name === 'input' || node.name === 'select')) { - context.state.init.push(b.stmt(b.call('$.remove_input_attr_defaults', context.state.node))); + if (needs_input_reset.length > 0 && (node.name === 'input' || node.name === 'select')) { + context.state.init.push( + b.stmt( + b.call('$.remove_input_attr_defaults', context.state.node, b.array(needs_input_reset)) + ) + ); } if (needs_content_reset && node.name === 'textarea') { diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index 5e39228f2b11..edd20df4cb53 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -14,10 +14,11 @@ import * as w from '../../warnings.js'; * @param {HTMLInputElement | HTMLSelectElement} dom * @returns {void} */ -export function remove_input_attr_defaults(dom) { +export function remove_input_attr_defaults(dom, to_remove = ['value', 'checked']) { if (hydrating) { - set_attribute(dom, 'value', null); - set_attribute(dom, 'checked', null); + for (const attr of to_remove) { + set_attribute(dom, attr, null); + } } } diff --git a/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js b/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js new file mode 100644 index 000000000000..db03102d8db8 --- /dev/null +++ b/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js @@ -0,0 +1,12 @@ +import { test } from '../../assert'; + +export default test({ + html: ``, + mode: ['server'], + test({ window, assert, mod }) { + assert.htmlEqual( + window.document.body.innerHTML, + `` + ); + } +}); diff --git a/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/main.svelte b/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/main.svelte new file mode 100644 index 000000000000..9dc489489b74 --- /dev/null +++ b/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/main.svelte @@ -0,0 +1,5 @@ + + + From a4acf7e71f73d56dd06b79d1fe9ee25b1cec19b6 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sat, 4 May 2024 19:54:14 +0200 Subject: [PATCH 2/7] fix: fix test for ssrHtml, html and runtime --- .../fine-grained-hydration-clean-attr/_config.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js b/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js index db03102d8db8..6602e5983459 100644 --- a/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js +++ b/packages/svelte/tests/runtime-browser/samples/fine-grained-hydration-clean-attr/_config.js @@ -1,12 +1,10 @@ import { test } from '../../assert'; export default test({ - html: ``, - mode: ['server'], - test({ window, assert, mod }) { - assert.htmlEqual( - window.document.body.innerHTML, - `` - ); + html: ``, + ssrHtml: ``, + test({ window, assert, target, mod }) { + const input = target.querySelector('input'); + assert.equal(input?.checked, true); } }); From e87bbe92626d2a2ef2df7363c8d232bbfbe8ca11 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sat, 4 May 2024 19:58:13 +0200 Subject: [PATCH 3/7] fix: snapshot tests update --- .../state-proxy-literal/_expected/client/index.svelte.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js index 455f227e09cf..fc05ca117c7c 100644 --- a/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js @@ -16,11 +16,11 @@ export default function State_proxy_literal($$anchor) { var fragment = root(); var input = $.first_child(fragment); - $.remove_input_attr_defaults(input); + $.remove_input_attr_defaults(input, ["value"]); var input_1 = $.sibling($.sibling(input, true)); - $.remove_input_attr_defaults(input_1); + $.remove_input_attr_defaults(input_1, ["value"]); var button = $.sibling($.sibling(input_1, true)); From a843d31b8fff670650d8bbe8d444197cf928e4c1 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sat, 4 May 2024 23:25:07 +0200 Subject: [PATCH 4/7] fix: revert changes --- .../3-transform/client/visitors/template.js | 31 +++++++------------ .../client/dom/elements/attributes.js | 7 ++--- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index c6afeeab6108..b6fe9649eb3d 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1838,10 +1838,7 @@ export const template_visitors = { const lets = []; const is_custom_element = is_custom_element_node(node); - /** - * @type {import('estree').Expression[]} - */ - let needs_input_reset = []; + let needs_input_reset = false; let needs_content_reset = false; /** @type {import('#compiler').BindDirective | null} */ @@ -1863,11 +1860,11 @@ export const template_visitors = { for (const attribute of node.attributes) { if (attribute.type === 'Attribute') { attributes.push(attribute); - if (attribute.name === 'value' && !is_text_attribute(attribute)) { - needs_input_reset.push(b.literal('value')); - needs_content_reset = true; - } else if (attribute.name === 'checked' && !is_text_attribute(attribute)) { - needs_input_reset.push(b.literal('checked')); + if ( + (attribute.name === 'value' || attribute.name === 'checked') && + !is_text_attribute(attribute) + ) { + needs_input_reset = true; needs_content_reset = true; } else if ( attribute.name === 'contenteditable' && @@ -1878,7 +1875,7 @@ export const template_visitors = { } } else if (attribute.type === 'SpreadAttribute') { attributes.push(attribute); - needs_input_reset = [b.literal('value'), b.literal('checked')]; + needs_input_reset = true; needs_content_reset = true; } else if (attribute.type === 'ClassDirective') { class_directives.push(attribute); @@ -1890,11 +1887,11 @@ export const template_visitors = { if (attribute.type === 'BindDirective') { if (attribute.name === 'group' || attribute.name === 'checked') { needs_special_value_handling = true; - needs_input_reset.push(b.literal('checked')); + needs_input_reset = true; } else if (attribute.name === 'value') { value_binding = attribute; needs_content_reset = true; - needs_input_reset.push(b.literal('value')); + needs_input_reset = true; } else if ( attribute.name === 'innerHTML' || attribute.name === 'innerText' || @@ -1910,7 +1907,7 @@ export const template_visitors = { if (child_metadata.namespace === 'foreign') { // input/select etc could mean something completely different in foreign namespace, so don't special-case them needs_content_reset = false; - needs_input_reset = []; + needs_input_reset = false; needs_special_value_handling = false; value_binding = null; } @@ -1919,12 +1916,8 @@ export const template_visitors = { child_metadata.bound_contenteditable = true; } - if (needs_input_reset.length > 0 && (node.name === 'input' || node.name === 'select')) { - context.state.init.push( - b.stmt( - b.call('$.remove_input_attr_defaults', context.state.node, b.array(needs_input_reset)) - ) - ); + if (needs_input_reset && (node.name === 'input' || node.name === 'select')) { + context.state.init.push(b.stmt(b.call('$.remove_input_attr_defaults', context.state.node))); } if (needs_content_reset && node.name === 'textarea') { diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index edd20df4cb53..5e39228f2b11 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -14,11 +14,10 @@ import * as w from '../../warnings.js'; * @param {HTMLInputElement | HTMLSelectElement} dom * @returns {void} */ -export function remove_input_attr_defaults(dom, to_remove = ['value', 'checked']) { +export function remove_input_attr_defaults(dom) { if (hydrating) { - for (const attr of to_remove) { - set_attribute(dom, attr, null); - } + set_attribute(dom, 'value', null); + set_attribute(dom, 'checked', null); } } From c862c984f516d3095b470b3bc690f0196b902f1a Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sat, 4 May 2024 23:29:06 +0200 Subject: [PATCH 5/7] fix: restore value after removing attribute --- .../svelte/src/internal/client/dom/elements/attributes.js | 2 ++ .../state-proxy-literal/_expected/client/index.svelte.js | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index 5e39228f2b11..b31bb12ac54e 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -16,8 +16,10 @@ import * as w from '../../warnings.js'; */ export function remove_input_attr_defaults(dom) { if (hydrating) { + const value = dom.value; set_attribute(dom, 'value', null); set_attribute(dom, 'checked', null); + dom.value = value; } } diff --git a/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js index fc05ca117c7c..455f227e09cf 100644 --- a/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/state-proxy-literal/_expected/client/index.svelte.js @@ -16,11 +16,11 @@ export default function State_proxy_literal($$anchor) { var fragment = root(); var input = $.first_child(fragment); - $.remove_input_attr_defaults(input, ["value"]); + $.remove_input_attr_defaults(input); var input_1 = $.sibling($.sibling(input, true)); - $.remove_input_attr_defaults(input_1, ["value"]); + $.remove_input_attr_defaults(input_1); var button = $.sibling($.sibling(input_1, true)); From 4baf1722a628903e6c4f9eee86a9a9bda8a8664f Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 5 May 2024 00:08:15 +0200 Subject: [PATCH 6/7] fix: use getAttribute instead of .value --- .../svelte/src/internal/client/dom/elements/attributes.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index b31bb12ac54e..e3816a733fb1 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -16,10 +16,12 @@ import * as w from '../../warnings.js'; */ export function remove_input_attr_defaults(dom) { if (hydrating) { - const value = dom.value; + // using getAttribute instead of dome.value allow us to have + // null instead of "on" if the user didn't set a value + const value = dom.getAttribute('value'); set_attribute(dom, 'value', null); set_attribute(dom, 'checked', null); - dom.value = value; + if (value) dom.value = value; } } From e795aff5e810feb21f4e5fcd0868ec263d7f5de2 Mon Sep 17 00:00:00 2001 From: Paolo Ricciuti Date: Sun, 5 May 2024 00:13:08 +0200 Subject: [PATCH 7/7] Update loud-numbers-flow.md --- .changeset/loud-numbers-flow.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/loud-numbers-flow.md b/.changeset/loud-numbers-flow.md index e04df7d6e503..68bd7cfe6f8d 100644 --- a/.changeset/loud-numbers-flow.md +++ b/.changeset/loud-numbers-flow.md @@ -2,4 +2,4 @@ "svelte": patch --- -fix: fine grained hydration attribute removal +fix: restore value after attribute removal during hydration