Skip to content

chore: tweak html tree validation #12618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jul 27, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/svelte/messages/compile-errors/template.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,14 @@

> %thing% is invalid inside <%parent%>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we're giving enough diagnostic information here. If I saw this error message...

<tr> is invalid inside <table>

...I would tear my hair out. I think we need some supplemental information at the very least, but it'd be better still if we just handled this case. What would it take for us to do that?

Copy link
Member Author

@dummdidumm dummdidumm Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I poked a bit at it this morning but didn't get very far - just opened a draft PR for visibility #12628


HTML has some restrictions where certain elements can appear. For example, a `<div>` inside a `<p>` is invalid. Some violations "only" result in invalid HTML, others result in the HTML being repaired by the browser, resulting in content shifting around. Some examples:

- `<p>hello <div>world</div></p>` will result in `<p>hello </p><div>world</div><p></p>` for example (the `<div>` autoclosed the `<p>`)
- `<option><div>option a</div></select>` will result in `<option>option a</option>` (the `<div>` is removed)
- `<table><tr><td>cell</td></tr></table>` will result in `<table><tbody><tr><td>cell</td></tr></tbody></table>` (a `<tbody>` is auto-inserted)

Svelte throws a compiler error when it detects that it will generate the HTML in such a way that it will always be repaired and result in the runtime code not finding the nodes at the expected locations.

## render_tag_invalid_call_expression

> Calling a snippet function using apply, bind or call is not allowed
Expand Down
12 changes: 12 additions & 0 deletions packages/svelte/messages/compile-warnings/template.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,18 @@

> Using `on:%name%` to listen to the %name% event is deprecated. Use the event attribute `on%name%` instead

## 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

HTML has some restrictions where certain elements can appear. For example, a `<div>` inside a `<p>` is invalid. Some violations "only" result in invalid HTML, others result in the HTML being repaired by the browser, resulting in content shifting around. Some examples:

- `<p>hello <div>world</div></p>` will result in `<p>hello </p><div>world</div><p></p>` for example (the `<div>` autoclosed the `<p>`)
- `<option><div>option a</div></select>` will result in `<option>option a</option>` (the `<div>` is removed)
- `<table><tr><td>cell</td></tr></table>` will result in `<table><tbody><tr><td>cell</td></tr></tbody></table>` (a `<tbody>` is auto-inserted)

Svelte issues a compiler warning when it detects that it will generate the HTML in such a way that it will work on the client, but always fail when using server side rendering, because the resulting HTML will be repaired and result in the client runtime not finding the nodes at the expected locations when hydrating the DOM.

## slot_element_deprecated

> Using `<slot>` to render parent content is deprecated. Use `{@render ...}` tags instead
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/src/compiler/phases/1-parse/state/element.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ import { is_void } from '../../../../constants.js';
import read_expression from '../read/expression.js';
import { read_script } from '../read/script.js';
import read_style from '../read/style.js';
import { closing_tag_omitted, decode_character_references } from '../utils/html.js';
import { decode_character_references } from '../utils/html.js';
import * as e from '../../../errors.js';
import * as w from '../../../warnings.js';
import { create_fragment } from '../utils/create.js';
import { create_attribute } from '../../nodes.js';
import { get_attribute_expression, is_expression_attribute } from '../../../utils/ast.js';
import { closing_tag_omitted } from '../../../../html-tree-validation.js';

// eslint-disable-next-line no-useless-escape
const valid_tag_name = /^\!?[a-zA-Z]{1,}:?[a-zA-Z0-9\-]*/;
Expand Down
46 changes: 0 additions & 46 deletions packages/svelte/src/compiler/phases/1-parse/utils/html.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { interactive_elements } from '../../../../constants.js';
import entities from './entities.js';

const windows_1252 = [
Expand Down Expand Up @@ -119,48 +118,3 @@ function validate_code(code) {

return NUL;
}

// based on http://developers.whatwg.org/syntax.html#syntax-tag-omission

/** @type {Record<string, Set<string>>} */
const disallowed_contents = {
li: new Set(['li']),
dt: new Set(['dt', 'dd']),
dd: new Set(['dt', 'dd']),
p: new Set(
'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'.split(
' '
)
),
rt: new Set(['rt', 'rp']),
rp: new Set(['rt', 'rp']),
optgroup: new Set(['optgroup']),
option: new Set(['option', 'optgroup']),
thead: new Set(['tbody', 'tfoot']),
tbody: new Set(['tbody', 'tfoot']),
tfoot: new Set(['tbody']),
tr: new Set(['tr', 'tbody']),
td: new Set(['td', 'th', 'tr']),
th: new Set(['td', 'th', 'tr'])
};

for (const interactive_element of interactive_elements) {
disallowed_contents[interactive_element] = interactive_elements;
}

// can this be a child of the parent element, or does it implicitly
// close it, like `<li>one<li>two`?

/**
* @param {string} current
* @param {string} [next]
*/
export function closing_tag_omitted(current, next) {
if (disallowed_contents[current]) {
if (!next || disallowed_contents[current].has(next)) {
return true;
}
}

return false;
}
78 changes: 45 additions & 33 deletions packages/svelte/src/compiler/phases/2-analyze/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@
/** @import { NodeLike } from '../../errors.js' */
/** @import { AnalysisState, Context, Visitors } from './types.js' */
import is_reference from 'is-reference';
import {
disallowed_paragraph_contents,
interactive_elements,
is_tag_valid_with_parent
} from '../../../constants.js';
import * as e from '../../errors.js';
import {
extract_identifiers,
Expand Down Expand Up @@ -37,6 +32,10 @@ import {
import { Scope, get_rune } from '../scope.js';
import { merge } from '../visitors.js';
import { a11y_validators } from './a11y.js';
import {
is_tag_valid_with_ancestor,
is_tag_valid_with_parent
} from '../../../html-tree-validation.js';

/**
* @param {Attribute} attribute
Expand Down Expand Up @@ -602,40 +601,53 @@ const validation = {
validate_element(node, context);

if (context.state.parent_element) {
if (!is_tag_valid_with_parent(node.name, context.state.parent_element)) {
e.node_invalid_placement(node, `<${node.name}>`, context.state.parent_element);
}
}
let past_parent = false;
let only_warn = false;

// can't add form to interactive elements because those are also used by the parser
// to check for the last auto-closing parent.
if (node.name === 'form') {
const path = context.path;
for (let parent of path) {
if (parent.type === 'RegularElement' && parent.name === 'form') {
e.node_invalid_placement(node, `<${node.name}>`, parent.name);
}
}
}
for (let i = context.path.length - 1; i >= 0; i--) {
const ancestor = context.path[i];

if (interactive_elements.has(node.name)) {
const path = context.path;
for (let parent of path) {
if (
parent.type === 'RegularElement' &&
parent.name === node.name &&
interactive_elements.has(parent.name)
ancestor.type === 'IfBlock' ||
ancestor.type === 'EachBlock' ||
ancestor.type === 'AwaitBlock' ||
ancestor.type === 'KeyBlock'
) {
e.node_invalid_placement(node, `<${node.name}>`, parent.name);
// We're creating a separate template string inside blocks, which means client-side this would work
only_warn = true;
}
}
}

if (disallowed_paragraph_contents.includes(node.name)) {
const path = context.path;
for (let parent of path) {
if (parent.type === 'RegularElement' && parent.name === 'p') {
e.node_invalid_placement(node, `<${node.name}>`, parent.name);
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)) {
if (only_warn) {
w.node_invalid_placement_ssr(node, `<${node.name}>`, context.state.parent_element);
} else {
e.node_invalid_placement(node, `<${node.name}>`, context.state.parent_element);
}
}

past_parent = true;
}
} else if (ancestor.type === 'RegularElement') {
if (!is_tag_valid_with_ancestor(node.name, ancestor.name)) {
if (only_warn) {
w.node_invalid_placement_ssr(node, `<${node.name}>`, ancestor.name);
} else {
e.node_invalid_placement(node, `<${node.name}>`, ancestor.name);
}
}
} else if (
ancestor.type === 'Component' ||
ancestor.type === 'SvelteComponent' ||
ancestor.type === 'SvelteElement' ||
ancestor.type === 'SvelteSelf' ||
ancestor.type === 'SnippetBlock'
) {
break;
}
}
}
Expand Down
11 changes: 11 additions & 0 deletions packages/svelte/src/compiler/warnings.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export const codes = [
"component_name_lowercase",
"element_invalid_self_closing_tag",
"event_directive_deprecated",
"node_invalid_placement_ssr",
"slot_element_deprecated",
"svelte_element_invalid_this"
];
Expand Down Expand Up @@ -739,6 +740,16 @@ export function event_directive_deprecated(node, name) {
w(node, "event_directive_deprecated", `Using \`on:${name}\` to listen to the ${name} event is deprecated. Use the event attribute \`on${name}\` instead`);
}

/**
* %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
* @param {null | NodeLike} node
* @param {string} thing
* @param {string} parent
*/
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`);
}

/**
* Using `<slot>` to render parent content is deprecated. Use `{@render ...}` tags instead
* @param {null | NodeLike} node
Expand Down
Loading
Loading