fix: better error messages for invalid HTML trees

dummdidumm · dummdidumm · commit 253f91a16bb1 · 2024-11-26T17:15:46.000+01:00
closes #13331
diff --git a/.changeset/green-pumpkins-ring.md b/.changeset/green-pumpkins-ring.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: better error messages for invalid HTML trees
diff --git a/documentation/docs/98-reference/.generated/compile-errors.md b/documentation/docs/98-reference/.generated/compile-errors.md
@@ -481,7 +481,7 @@ A component cannot have a default export
 ### node_invalid_placement
 
 ```
-%thing% is invalid inside `<%parent%>`
+%message%
 ```
 
 HTML restricts where certain elements can appear. In case of a violation the browser will 'repair' the HTML in a way that breaks Svelte's assumptions about the structure of your components. Some examples:
diff --git a/documentation/docs/98-reference/.generated/compile-warnings.md b/documentation/docs/98-reference/.generated/compile-warnings.md
@@ -643,7 +643,7 @@ Svelte 5 components are no longer classes. Instantiate them using `mount` or `hy
 ### node_invalid_placement_ssr
 
 ```
-%thing% is invalid inside `<%parent%>`. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning
+%message%. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning
 ```
 
 HTML restricts where certain elements can appear. In case of a violation the browser will 'repair' the HTML in a way that breaks Svelte's assumptions about the structure of your components. Some examples:
diff --git a/packages/svelte/messages/compile-errors/template.md b/packages/svelte/messages/compile-errors/template.md
@@ -190,7 +190,7 @@
 
 ## node_invalid_placement
 
-> %thing% is invalid inside `<%parent%>`
+> %message%
 
 HTML restricts where certain elements can appear. In case of a violation the browser will 'repair' the HTML in a way that breaks Svelte's assumptions about the structure of your components. Some examples:
 
diff --git a/packages/svelte/messages/compile-warnings/template.md b/packages/svelte/messages/compile-warnings/template.md
@@ -40,7 +40,7 @@
 
 ## node_invalid_placement_ssr
 
-> %thing% is invalid inside `<%parent%>`. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning
+> %message%. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning
 
 HTML restricts where certain elements can appear. In case of a violation the browser will 'repair' the HTML in a way that breaks Svelte's assumptions about the structure of your components. Some examples:
 
diff --git a/packages/svelte/src/compiler/errors.js b/packages/svelte/src/compiler/errors.js
@@ -1024,14 +1024,13 @@ export function mixed_event_handler_syntaxes(node, name) {
 }
 
 /**
- * %thing% is invalid inside `<%parent%>`
+ * %message%
  * @param {null | number | NodeLike} node
- * @param {string} thing
- * @param {string} parent
+ * @param {string} message
  * @returns {never}
  */
-export function node_invalid_placement(node, thing, parent) {
-	e(node, "node_invalid_placement", `${thing} is invalid inside \`<${parent}>\``);
+export function node_invalid_placement(node, message) {
+	e(node, "node_invalid_placement", `${message}`);
 }
 
 /**
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/ExpressionTag.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/ExpressionTag.js
@@ -12,8 +12,9 @@ export function ExpressionTag(node, context) {
 	const in_attribute = context.path.at(-1)?.type === 'Attribute';
 
 	if (!in_attribute && context.state.parent_element) {
-		if (!is_tag_valid_with_parent('#text', context.state.parent_element)) {
-			e.node_invalid_placement(node, '`{expression}`', context.state.parent_element);
+		const message = is_tag_valid_with_parent('#text', context.state.parent_element);
+		if (message) {
+			e.node_invalid_placement(node, message);
 		}
 	}
 
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/RegularElement.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/RegularElement.js
@@ -114,15 +114,12 @@ export function RegularElement(node, context) {
 
 			if (!past_parent) {
 				if (ancestor.type === 'RegularElement' && ancestor.name === context.state.parent_element) {
-					if (!is_tag_valid_with_parent(node.name, context.state.parent_element)) {
+					const message = is_tag_valid_with_parent(node.name, context.state.parent_element);
+					if (message) {
 						if (only_warn) {
-							w.node_invalid_placement_ssr(
-								node,
-								`\`<${node.name}>\``,
-								context.state.parent_element
-							);
+							w.node_invalid_placement_ssr(node, message);
 						} else {
-							e.node_invalid_placement(node, `\`<${node.name}>\``, context.state.parent_element);
+							e.node_invalid_placement(node, message);
 						}
 					}
 
@@ -131,11 +128,12 @@ export function RegularElement(node, context) {
 			} else if (ancestor.type === 'RegularElement') {
 				ancestors.push(ancestor.name);
 
-				if (!is_tag_valid_with_ancestor(node.name, ancestors)) {
+				const message = is_tag_valid_with_ancestor(node.name, ancestors);
+				if (message) {
 					if (only_warn) {
-						w.node_invalid_placement_ssr(node, `\`<${node.name}>\``, ancestor.name);
+						w.node_invalid_placement_ssr(node, message);
 					} else {
-						e.node_invalid_placement(node, `\`<${node.name}>\``, ancestor.name);
+						e.node_invalid_placement(node, message);
 					}
 				}
 			} else if (
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Text.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Text.js
@@ -12,8 +12,9 @@ export function Text(node, context) {
 	const in_attribute = context.path.at(-1)?.type === 'Attribute';
 
 	if (!in_attribute && context.state.parent_element && regex_not_whitespace.test(node.data)) {
-		if (!is_tag_valid_with_parent('#text', context.state.parent_element)) {
-			e.node_invalid_placement(node, 'Text node', context.state.parent_element);
+		const message = is_tag_valid_with_parent('#text', context.state.parent_element);
+		if (message) {
+			e.node_invalid_placement(node, message);
 		}
 	}
 }
diff --git a/packages/svelte/src/compiler/warnings.js b/packages/svelte/src/compiler/warnings.js
@@ -763,13 +763,12 @@ export function event_directive_deprecated(node, name) {
 }
 
 /**
- * %thing% is invalid inside `<%parent%>`. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning
+ * %message%. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning
  * @param {null | NodeLike} node
- * @param {string} thing
- * @param {string} parent
+ * @param {string} message
  */
-export function node_invalid_placement_ssr(node, thing, parent) {
-	w(node, "node_invalid_placement_ssr", `${thing} is invalid inside \`<${parent}>\`. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a \`hydration_mismatch\` warning`);
+export function node_invalid_placement_ssr(node, message) {
+	w(node, "node_invalid_placement_ssr", `${message}. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a \`hydration_mismatch\` warning`);
 }
 
 /**
diff --git a/packages/svelte/src/html-tree-validation.js b/packages/svelte/src/html-tree-validation.js
@@ -135,58 +135,70 @@ const disallowed_children = {
 };
 
 /**
- * Returns false if the tag is not allowed inside the ancestor tag (which is grandparent and above) such that it will result
+ * Returns an error message if the tag is not allowed inside the ancestor tag (which is grandparent and above) such that it will result
  * in the browser repairing the HTML, which will likely result in an error during hydration.
  * @param {string} tag
  * @param {string[]} ancestors All nodes starting with the parent, up until the ancestor, which means two entries minimum
- * @returns {boolean}
+ * @returns {string | null}
  */
 export function is_tag_valid_with_ancestor(tag, ancestors) {
-	if (tag.includes('-')) return true; // custom elements can be anything
+	if (tag.includes('-')) return null; // custom elements can be anything
 
 	const target = ancestors[ancestors.length - 1];
 	const disallowed = disallowed_children[target];
-	if (!disallowed) return true;
+	if (!disallowed) return null;
 
 	if ('reset_by' in disallowed && disallowed.reset_by) {
 		for (let i = ancestors.length - 2; i >= 0; i--) {
 			const ancestor = ancestors[i];
-			if (ancestor.includes('-')) return true; // custom elements can be anything
+			if (ancestor.includes('-')) return null; // custom elements can be anything
 
 			// A reset means that forbidden descendants are allowed again
 			if (disallowed.reset_by.includes(ancestors[i])) {
-				return true;
+				return null;
 			}
 		}
 	}
 
-	return 'descendant' in disallowed ? !disallowed.descendant.includes(tag) : true;
+	if ('descendant' in disallowed && disallowed.descendant.includes(tag)) {
+		return `\`<${tag}>\` cannot be a child of \`<${target}>\`. \`<${target}>\` disallows these children: ${disallowed.descendant.map((d) => `\`<${d}>\``).join(', ')}`;
+	}
+
+	return null;
 }
 
 /**
- * Returns false if the tag is not allowed inside the parent tag such that it will result
+ * Returns an error message if the tag is not allowed inside the parent tag such that it will result
  * in the browser repairing the HTML, which will likely result in an error during hydration.
  * @param {string} tag
  * @param {string} parent_tag
- * @returns {boolean}
+ * @returns {string | null}
  */
 export function is_tag_valid_with_parent(tag, parent_tag) {
-	if (tag.includes('-') || parent_tag?.includes('-')) return true; // custom elements can be anything
+	if (tag.includes('-') || parent_tag?.includes('-')) return null; // custom elements can be anything
 
 	const disallowed = disallowed_children[parent_tag];
 
 	if (disallowed) {
 		if ('direct' in disallowed && disallowed.direct.includes(tag)) {
-			return false;
+			return `\`<${tag}>\` cannot be a direct child of \`<${parent_tag}>\``;
 		}
 		if ('descendant' in disallowed && disallowed.descendant.includes(tag)) {
-			return false;
+			return `\`<${tag}>\` cannot be a child of \`<${parent_tag}>\``;
 		}
 		if ('only' in disallowed && disallowed.only) {
-			return disallowed.only.includes(tag);
+			if (disallowed.only.includes(tag)) {
+				return null;
+			} else {
+				return `\`<${tag}>\` cannot be a child of \`<${parent_tag}>\`. \`<${parent_tag}>\` only allows these children: ${disallowed.only.map((d) => `\`<${d}>\``).join(', ')}`;
+			}
 		}
 	}
 
+	// These tags are only valid with a few parents that have special child
+	// parsing rules - if we're down here, then none of those matched and
+	// so we allow it only if we don't know what the parent is, as all other
+	// cases are invalid (and we only get into this function if we know the parent).
 	switch (tag) {
 		case 'body':
 		case 'caption':
@@ -196,18 +208,17 @@ export function is_tag_valid_with_parent(tag, parent_tag) {
 		case 'frame':
 		case 'head':
 		case 'html':
+			return `\`<${tag}>\` cannot be a child of \`<${parent_tag}>\``;
+		case 'thead':
 		case 'tbody':
-		case 'td':
 		case 'tfoot':
+			return `\`<${tag}>\` must be the child of a \`<table>\` (but got \`<${parent_tag}>\`)`;
+		case 'td':
 		case 'th':
-		case 'thead':
+			return `\`<${tag}>\` must be the child of a \`<tr>\` (but got \`<${parent_tag}>\`)`;
 		case 'tr':
-			// These tags are only valid with a few parents that have special child
-			// parsing rules - if we're down here, then none of those matched and
-			// so we allow it only if we don't know what the parent is, as all other
-			// cases are invalid (and we only get into this function if we know the parent).
-			return false;
+			return `\`<tr>\` must be the child of a \`<thead>\`, \`<tbody>\`, or \`<tfoot>\` (but got \`<${parent_tag}>\`)`;
 	}
 
-	return true;
+	return null;
 }
diff --git a/packages/svelte/src/internal/server/dev.js b/packages/svelte/src/internal/server/dev.js
@@ -36,10 +36,11 @@ function stringify(element) {
  * @param {Payload} payload
  * @param {Element} parent
  * @param {Element} child
+ * @param {string} message
  */
-function print_error(payload, parent, child) {
-	var message =
-		`node_invalid_placement_ssr: ${stringify(parent)} cannot contain ${stringify(child)}\n\n` +
+function print_error(payload, parent, child, message) {
+	message =
+		`node_invalid_placement_ssr: ${stringify(parent)} cannot contain ${stringify(child)} (${message})\n\n` +
 		'This can cause content to shift around as the browser repairs the HTML, and will likely result in a `hydration_mismatch` warning.';
 
 	if ((seen ??= new Set()).has(message)) return;
@@ -72,14 +73,16 @@ export function push_element(payload, tag, line, column) {
 		var ancestor = parent.parent;
 		var ancestors = [parent.tag];
 
-		if (!is_tag_valid_with_parent(tag, parent.tag)) {
-			print_error(payload, parent, child);
+		const message = is_tag_valid_with_parent(tag, parent.tag);
+		if (message) {
+			print_error(payload, parent, child, message);
 		}
 
 		while (ancestor != null) {
 			ancestors.push(ancestor.tag);
-			if (!is_tag_valid_with_ancestor(tag, ancestors)) {
-				print_error(payload, ancestor, child);
+			const message = is_tag_valid_with_ancestor(tag, ancestors);
+			if (message) {
+				print_error(payload, parent, child, message);
 			}
 			ancestor = ancestor.parent;
 		}
diff --git a/packages/svelte/tests/runtime-runes/samples/invalid-html-ssr/_config.js b/packages/svelte/tests/runtime-runes/samples/invalid-html-ssr/_config.js
@@ -12,8 +12,8 @@ export default test({
 	mode: ['hydrate'],
 
 	errors: [
-		'node_invalid_placement_ssr: `<p>` (main.svelte:6:0) cannot contain `<h1>` (h1.svelte:1:0)\n\nThis can cause content to shift around as the browser repairs the HTML, and will likely result in a `hydration_mismatch` warning.',
-		'node_invalid_placement_ssr: `<form>` (main.svelte:9:0) cannot contain `<form>` (form.svelte:1:0)\n\nThis can cause content to shift around as the browser repairs the HTML, and will likely result in a `hydration_mismatch` warning.'
+		'node_invalid_placement_ssr: `<p>` (main.svelte:6:0) cannot contain `<h1>` (h1.svelte:1:0) (`<h1>` cannot be a child of `<p>`)\n\nThis can cause content to shift around as the browser repairs the HTML, and will likely result in a `hydration_mismatch` warning.',
+		'node_invalid_placement_ssr: `<form>` (main.svelte:9:0) cannot contain `<form>` (form.svelte:1:0) (`<form>` cannot be a child of `<form>`)\n\nThis can cause content to shift around as the browser repairs the HTML, and will likely result in a `hydration_mismatch` warning.'
 	],
 
 	warnings: [
diff --git a/packages/svelte/tests/validator/samples/invalid-node-placement-2/errors.json b/packages/svelte/tests/validator/samples/invalid-node-placement-2/errors.json
@@ -1,7 +1,7 @@
 [
 	{
 		"code": "node_invalid_placement",
-		"message": "`<div>` is invalid inside `<p>`",
+		"message": "`<div>` cannot be a child of `<p>`. `<p>` disallows these children: `<address>`, `<article>`, `<aside>`, `<blockquote>`, `<div>`, `<dl>`, `<fieldset>`, `<footer>`, `<form>`, `<h1>`, `<h2>`, `<h3>`, `<h4>`, `<h5>`, `<h6>`, `<header>`, `<hgroup>`, `<hr>`, `<main>`, `<menu>`, `<nav>`, `<ol>`, `<p>`, `<pre>`, `<section>`, `<table>`, `<ul>`",
 		"start": {
 			"line": 4,
 			"column": 3
diff --git a/packages/svelte/tests/validator/samples/invalid-node-placement-4/errors.json b/packages/svelte/tests/validator/samples/invalid-node-placement-4/errors.json
@@ -1,7 +1,7 @@
 [
 	{
 		"code": "node_invalid_placement",
-		"message": "`<form>` is invalid inside `<form>`",
+		"message": "`<form>` cannot be a child of `<form>`. `<form>` disallows these children: `<form>`",
 		"start": {
 			"line": 4,
 			"column": 3
diff --git a/packages/svelte/tests/validator/samples/invalid-node-placement-5/warnings.json b/packages/svelte/tests/validator/samples/invalid-node-placement-5/warnings.json
@@ -1,7 +1,7 @@
 [
 	{
 		"code": "node_invalid_placement_ssr",
-		"message": "`<form>` is invalid inside `<form>`. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning",
+		"message": "`<form>` cannot be a child of `<form>`. When rendering this component on the server, the resulting HTML will be modified by the browser, likely resulting in a `hydration_mismatch` warning",
 		"start": {
 			"line": 4,
 			"column": 3
diff --git a/packages/svelte/tests/validator/samples/invalid-node-placement-6/errors.json b/packages/svelte/tests/validator/samples/invalid-node-placement-6/errors.json
@@ -1,7 +1,7 @@
 [
 	{
 		"code": "node_invalid_placement",
-		"message": "`<dt>` is invalid inside `<dd>`",
+		"message": "`<dt>` cannot be a child of `<dd>`. `<dd>` disallows these children: `<dt>`, `<dd>`",
 		"start": {
 			"line": 16,
 			"column": 3
diff --git a/packages/svelte/tests/validator/samples/invalid-node-placement-7/errors.json b/packages/svelte/tests/validator/samples/invalid-node-placement-7/errors.json
@@ -1,7 +1,7 @@
 [
 	{
 		"code": "node_invalid_placement",
-		"message": "`<tbody>` is invalid inside `<div>`",
+		"message": "`<tbody>` must be the child of a `<table>` (but got `<div>`)",
 		"start": {
 			"line": 8,
 			"column": 1
diff --git a/packages/svelte/tests/validator/samples/invalid-node-placement/errors.json b/packages/svelte/tests/validator/samples/invalid-node-placement/errors.json
@@ -1,7 +1,7 @@
 [
 	{
 		"code": "node_invalid_placement",
-		"message": "`<a>` is invalid inside `<a>`",
+		"message": "`<a>` cannot be a child of `<a>`. `<a>` disallows these children: `<a>`",
 		"start": {
 			"line": 4,
 			"column": 6

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'svelte': patch
 +---
++
 +fix: better error messages for invalid HTML trees
Original file line number	Diff line number	Diff line change
`@@ -12,8 +12,9 @@ export function ExpressionTag(node, context) {`
`12`	`12`	`const in_attribute = context.path.at(-1)?.type === 'Attribute';`
`13`	`13`
`14`	`14`	`if (!in_attribute && context.state.parent_element) {`
`15`		`- if (!is_tag_valid_with_parent('#text', context.state.parent_element)) {`
`16`		- e.node_invalid_placement(node, '`{expression}`', context.state.parent_element);
	`15`	`+ const message = is_tag_valid_with_parent('#text', context.state.parent_element);`
	`16`	`+ if (message) {`
	`17`	`+ e.node_invalid_placement(node, message);`
`17`	`18`	`}`
`18`	`19`	`}`
`19`	`20`
Original file line number	Diff line number	Diff line change
`@@ -12,8 +12,9 @@ export function Text(node, context) {`
`12`	`12`	`const in_attribute = context.path.at(-1)?.type === 'Attribute';`
`13`	`13`
`14`	`14`	`if (!in_attribute && context.state.parent_element && regex_not_whitespace.test(node.data)) {`
`15`		`- if (!is_tag_valid_with_parent('#text', context.state.parent_element)) {`
`16`		`- e.node_invalid_placement(node, 'Text node', context.state.parent_element);`
	`15`	`+ const message = is_tag_valid_with_parent('#text', context.state.parent_element);`
	`16`	`+ if (message) {`
	`17`	`+ e.node_invalid_placement(node, message);`
`17`	`18`	`}`
`18`	`19`	`}`
`19`	`20`	`}`