Skip to content

Commit

Permalink
Revert "fix: improve custom element checks in html validation"
Browse files Browse the repository at this point in the history
This reverts commit 72fab2b.
  • Loading branch information
edoardocavazza committed Jan 22, 2025
1 parent 72fab2b commit 57137e6
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 115 deletions.
4 changes: 2 additions & 2 deletions packages/svelte/src/compiler/phases/2-analyze/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ export interface AnalysisState {
options: ValidatedCompileOptions;
ast_type: 'instance' | 'template' | 'module';
/**
* The parent element. `null` if the parent is `svelte:element`, `#snippet`, a component or the root.
* Tag name of the parent element. `null` if the parent is `svelte:element`, `#snippet`, a component or the root.
* Parent doesn't necessarily mean direct path predecessor because there could be `#each`, `#if` etc in-between.
*/
parent_element: AST.RegularElement | null;
parent_element: string | null;
has_props_rune: boolean;
/** Which slots the current parent component has */
component_slots: Set<string>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
/** @import { Context } from '../types' */
import { is_tag_valid_with_parent } from '../../../../html-tree-validation.js';
import * as e from '../../../errors.js';
import { is_custom_element_node } from '../../nodes.js';
import { mark_subtree_dynamic } from './shared/fragment.js';

/**
Expand All @@ -13,16 +12,7 @@ export function ExpressionTag(node, context) {
const in_template = context.path.at(-1)?.type === 'Fragment';

if (in_template && context.state.parent_element) {
const message = is_tag_valid_with_parent(
{
tag: '#text',
custom_element: false
},
{
tag: context.state.parent_element.name,
custom_element: is_custom_element_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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,7 @@ export function RegularElement(node, context) {
if (context.state.parent_element) {
let past_parent = false;
let only_warn = false;
const ancestors = [
{
tag: context.state.parent_element.name,
custom_element: is_custom_element_node(context.state.parent_element)
}
];
const ancestors = [context.state.parent_element];

for (let i = context.path.length - 1; i >= 0; i--) {
const ancestor = context.path[i];
Expand All @@ -137,20 +132,8 @@ export function RegularElement(node, context) {
}

if (!past_parent) {
if (
ancestor.type === 'RegularElement' &&
ancestor.name === context.state.parent_element.name
) {
const message = is_tag_valid_with_parent(
{
tag: node.name,
custom_element: is_custom_element_node(node)
},
{
tag: context.state.parent_element.name,
custom_element: is_custom_element_node(context.state.parent_element)
}
);
if (ancestor.type === 'RegularElement' && ancestor.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, message);
Expand All @@ -162,18 +145,9 @@ export function RegularElement(node, context) {
past_parent = true;
}
} else if (ancestor.type === 'RegularElement') {
ancestors.push({
tag: ancestor.name,
custom_element: is_custom_element_node(ancestor)
});

const message = is_tag_valid_with_ancestor(
{
tag: node.name,
custom_element: is_custom_element_node(node)
},
ancestors
);
ancestors.push(ancestor.name);

const message = is_tag_valid_with_ancestor(node.name, ancestors);
if (message) {
if (only_warn) {
w.node_invalid_placement_ssr(node, message);
Expand Down Expand Up @@ -204,7 +178,7 @@ export function RegularElement(node, context) {
w.element_invalid_self_closing_tag(node, node.name);
}

context.next({ ...context.state, parent_element: node });
context.next({ ...context.state, parent_element: node.name });

// Special case: <a> tags are valid in both the SVG and HTML namespace.
// If there's no parent, look downwards to see if it's the parent of a SVG or HTML element.
Expand Down
12 changes: 1 addition & 11 deletions packages/svelte/src/compiler/phases/2-analyze/visitors/Text.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import { is_tag_valid_with_parent } from '../../../../html-tree-validation.js';
import { regex_not_whitespace } from '../../patterns.js';
import * as e from '../../../errors.js';
import { is_custom_element_node } from '../../nodes.js';

/**
* @param {AST.Text} node
Expand All @@ -13,16 +12,7 @@ export function Text(node, context) {
const in_template = context.path.at(-1)?.type === 'Fragment';

if (in_template && context.state.parent_element && regex_not_whitespace.test(node.data)) {
const message = is_tag_valid_with_parent(
{
tag: '#text',
custom_element: false
},
{
tag: context.state.parent_element.name,
custom_element: is_custom_element_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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import { is_void } from '../../../../../utils.js';
import { dev, locator } from '../../../../state.js';
import * as b from '../../../../utils/builders.js';
import { is_custom_element_node } from '../../../nodes.js';
import { clean_nodes, determine_namespace_for_children } from '../../utils.js';
import { build_element_attributes } from './shared/element.js';
import { process_children, build_template } from './shared/utils.js';
Expand Down Expand Up @@ -63,7 +62,6 @@ export function RegularElement(node, context) {
'$.push_element',
b.id('$$payload'),
b.literal(node.name),
b.literal(is_custom_element_node(node)),
b.literal(location.line),
b.literal(location.column)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ export function SvelteElement(node, context) {
'$.push_element',
b.id('$$payload'),
tag,
b.literal(false),
b.literal(location.line),
b.literal(location.column)
)
Expand Down
53 changes: 22 additions & 31 deletions packages/svelte/src/html-tree-validation.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,3 @@
/**
* @typedef {{
* tag: string;
* custom_element: boolean;
* }} Element
*/

/**
* Map of elements that have certain elements that are not allowed inside them, in the sense that they will auto-close the parent/ancestor element.
* Theoretically one could take advantage of it but most of the time it will just result in confusing behavior and break when SSR'd.
Expand Down Expand Up @@ -144,33 +137,33 @@ const disallowed_children = {
/**
* 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 {Element} child_node
* @param {Element[]} ancestors All nodes starting with the parent, up until the ancestor, which means two entries minimum
* @param {string} child_tag
* @param {string[]} ancestors All nodes starting with the parent, up until the ancestor, which means two entries minimum
* @param {string} [child_loc]
* @param {string} [ancestor_loc]
* @returns {string | null}
*/
export function is_tag_valid_with_ancestor(child_node, ancestors, child_loc, ancestor_loc) {
if (child_node.custom_element) return null; // custom elements can be anything
export function is_tag_valid_with_ancestor(child_tag, ancestors, child_loc, ancestor_loc) {
if (child_tag.includes('-')) return null; // custom elements can be anything

const ancestor_tag = ancestors[ancestors.length - 1].tag;
const ancestor_tag = ancestors[ancestors.length - 1];
const disallowed = disallowed_children[ancestor_tag];
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.custom_element) return null; // 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].tag)) {
if (disallowed.reset_by.includes(ancestors[i])) {
return null;
}
}
}

if ('descendant' in disallowed && disallowed.descendant.includes(child_node.tag)) {
const child = child_loc ? `\`<${child_node.tag}>\` (${child_loc})` : `\`<${child_node.tag}>\``;
if ('descendant' in disallowed && disallowed.descendant.includes(child_tag)) {
const child = child_loc ? `\`<${child_tag}>\` (${child_loc})` : `\`<${child_tag}>\``;
const ancestor = ancestor_loc
? `\`<${ancestor_tag}>\` (${ancestor_loc})`
: `\`<${ancestor_tag}>\``;
Expand All @@ -184,38 +177,36 @@ export function is_tag_valid_with_ancestor(child_node, ancestors, child_loc, anc
/**
* 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 {Element} child_node
* @param {Element} parent_node
* @param {string} child_tag
* @param {string} parent_tag
* @param {string} [child_loc]
* @param {string} [parent_loc]
* @returns {string | null}
*/
export function is_tag_valid_with_parent(child_node, parent_node, child_loc, parent_loc) {
if (child_node.custom_element || parent_node?.custom_element) return null; // custom elements can be anything
export function is_tag_valid_with_parent(child_tag, parent_tag, child_loc, parent_loc) {
if (child_tag.includes('-') || parent_tag?.includes('-')) return null; // custom elements can be anything

if (parent_node.tag === 'template') return null; // no errors or warning should be thrown in immediate children of template tags
if (parent_tag === 'template') return null; // no errors or warning should be thrown in immediate children of template tags

const disallowed = disallowed_children[parent_node.tag];
const disallowed = disallowed_children[parent_tag];

const child = child_loc ? `\`<${child_node.tag}>\` (${child_loc})` : `\`<${child_node.tag}>\``;
const parent = parent_loc
? `\`<${parent_node.tag}>\` (${parent_loc})`
: `\`<${parent_node.tag}>\``;
const child = child_loc ? `\`<${child_tag}>\` (${child_loc})` : `\`<${child_tag}>\``;
const parent = parent_loc ? `\`<${parent_tag}>\` (${parent_loc})` : `\`<${parent_tag}>\``;

if (disallowed) {
if ('direct' in disallowed && disallowed.direct.includes(child_node.tag)) {
if ('direct' in disallowed && disallowed.direct.includes(child_tag)) {
return `${child} cannot be a direct child of ${parent}`;
}

if ('descendant' in disallowed && disallowed.descendant.includes(child_node.tag)) {
if ('descendant' in disallowed && disallowed.descendant.includes(child_tag)) {
return `${child} cannot be a child of ${parent}`;
}

if ('only' in disallowed && disallowed.only) {
if (disallowed.only.includes(child_node.tag)) {
if (disallowed.only.includes(child_tag)) {
return null;
} else {
return `${child} cannot be a child of ${parent}. \`<${parent_node.tag}>\` only allows these children: ${disallowed.only.map((d) => `\`<${d}>\``).join(', ')}`;
return `${child} cannot be a child of ${parent}. \`<${parent_tag}>\` only allows these children: ${disallowed.only.map((d) => `\`<${d}>\``).join(', ')}`;
}
}
}
Expand All @@ -224,7 +215,7 @@ export function is_tag_valid_with_parent(child_node, parent_node, child_loc, par
// 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 (child_node.tag) {
switch (child_tag) {
case 'body':
case 'caption':
case 'col':
Expand Down
30 changes: 6 additions & 24 deletions packages/svelte/src/internal/server/dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { current_component } from './context.js';
/**
* @typedef {{
* tag: string;
* custom_element: boolean;
* parent: null | Element;
* filename: null | string;
* line: number;
Expand Down Expand Up @@ -61,49 +60,32 @@ export function reset_elements() {
/**
* @param {Payload} payload
* @param {string} tag
* @param {boolean} custom_element
* @param {number} line
* @param {number} column
*/
export function push_element(payload, tag, custom_element, line, column) {
export function push_element(payload, tag, line, column) {
var filename = /** @type {Component} */ (current_component).function[FILENAME];
var child = { tag, custom_element, parent, filename, line, column };
var child = { tag, parent, filename, line, column };

if (parent !== null) {
var ancestor = parent.parent;
var ancestors = [parent];
var ancestors = [parent.tag];

const child_loc = filename ? `${filename}:${line}:${column}` : undefined;
const parent_loc = parent.filename
? `${parent.filename}:${parent.line}:${parent.column}`
: undefined;

const message = is_tag_valid_with_parent(
{
tag,
custom_element
},
parent,
child_loc,
parent_loc
);
const message = is_tag_valid_with_parent(tag, parent.tag, child_loc, parent_loc);
if (message) print_error(payload, message);

while (ancestor != null) {
ancestors.push(ancestor);
ancestors.push(ancestor.tag);
const ancestor_loc = ancestor.filename
? `${ancestor.filename}:${ancestor.line}:${ancestor.column}`
: undefined;

const message = is_tag_valid_with_ancestor(
{
tag,
custom_element
},
ancestors,
child_loc,
ancestor_loc
);
const message = is_tag_valid_with_ancestor(tag, ancestors, child_loc, ancestor_loc);
if (message) print_error(payload, message);

ancestor = ancestor.parent;
Expand Down

0 comments on commit 57137e6

Please sign in to comment.