Skip to content
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

fix: create <svelte:element> instances with the correct namespace #10006

Merged
merged 19 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ jobs:
- name: type check
run: pnpm check
- name: lint
if: (${{ success() }} || ${{ failure() }}) # ensures this step runs even if previous steps fail (avoids multiple runs uncovering different issues at different steps)
run: pnpm lint
- name: build and check generated types
if: (${{ success() }} || ${{ failure() }}) # ensures this step runs even if previous steps fail
run: pnpm build && { [ "`git status --porcelain=v1`" == "" ] || (echo "Generated types have changed — please regenerate types locally and commit the changes after you have reviewed them"; git diff; exit 1); }
3 changes: 2 additions & 1 deletion packages/svelte/src/compiler/phases/1-parse/read/options.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { namespace_svg } from '../../../../constants.js';
import { error } from '../../../errors.js';

const regex_valid_tag_name = /^[a-zA-Z][a-zA-Z0-9]*-[a-zA-Z0-9-]+$/;
Expand Down Expand Up @@ -156,7 +157,7 @@ export default function read_options(node) {
error(attribute, 'invalid-svelte-option-namespace');
}

if (value === 'http://www.w3.org/2000/svg') {
if (value === namespace_svg) {
component_options.namespace = 'svg';
} else if (value === 'html' || value === 'svg' || value === 'foreign') {
component_options.namespace = value;
Expand Down
5 changes: 4 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 @@ -139,7 +139,10 @@ export default function tag(parser) {
name,
attributes: [],
fragment: create_fragment(true),
parent: null
parent: null,
metadata: {
svg: false
}
};

parser.allow_whitespace();
Expand Down
45 changes: 42 additions & 3 deletions packages/svelte/src/compiler/phases/2-analyze/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { warn } from '../../warnings.js';
import check_graph_for_cycles from './utils/check_graph_for_cycles.js';
import { regex_starts_with_newline } from '../patterns.js';
import { create_attribute, is_element_node } from '../nodes.js';
import { DelegatedEvents } from '../../../constants.js';
import { DelegatedEvents, namespace_svg } from '../../../constants.js';
import { should_proxy_or_freeze } from '../3-transform/client/utils.js';

/**
Expand Down Expand Up @@ -1092,8 +1092,47 @@ const common_visitors = {

context.state.analysis.elements.push(node);
},
SvelteElement(node, { state }) {
state.analysis.elements.push(node);
SvelteElement(node, context) {
context.state.analysis.elements.push(node);

if (
context.state.options.namespace !== 'foreign' &&
node.tag.type === 'Literal' &&
typeof node.tag.value === 'string' &&
SVGElements.includes(node.tag.value)
) {
node.metadata.svg = true;
return;
}

for (const attribute of node.attributes) {
if (attribute.type === 'Attribute') {
if (attribute.name === 'xmlns' && is_text_attribute(attribute)) {
node.metadata.svg = attribute.value[0].data === namespace_svg;
return;
}
}
}

for (let i = context.path.length - 1; i >= 0; i--) {
const ancestor = context.path[i];
if (
ancestor.type === 'Component' ||
ancestor.type === 'SvelteComponent' ||
ancestor.type === 'SvelteFragment' ||
ancestor.type === 'SnippetBlock'
) {
// Inside a slot or a snippet -> this resets the namespace, so we can't determine it
return;
}
if (ancestor.type === 'SvelteElement' || ancestor.type === 'RegularElement') {
node.metadata.svg =
ancestor.type === 'RegularElement' && ancestor.name === 'foreignObject'
? false
: ancestor.metadata.svg;
return;
}
}
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { binding_properties } from '../../../bindings.js';
import {
clean_nodes,
determine_element_namespace,
determine_namespace_for_children,
escape_html,
infer_namespace
} from '../../utils.js';
Expand Down Expand Up @@ -43,11 +43,7 @@ import { sanitize_template_string } from '../../../../utils/sanitize_template_st
*/
function get_attribute_name(element, attribute, context) {
let name = attribute.name;
if (
element.type === 'RegularElement' &&
!element.metadata.svg &&
context.state.metadata.namespace !== 'foreign'
) {
if (!element.metadata.svg && context.state.metadata.namespace !== 'foreign') {
name = name.toLowerCase();
if (name in AttributeAliases) {
name = AttributeAliases[name];
Expand Down Expand Up @@ -1854,7 +1850,7 @@ export const template_visitors = {
const metadata = context.state.metadata;
const child_metadata = {
...context.state.metadata,
namespace: determine_element_namespace(node, context.state.metadata.namespace, context.path)
namespace: determine_namespace_for_children(node, context.state.metadata.namespace)
};

context.state.template.push(`<${node.name}`);
Expand Down Expand Up @@ -2079,9 +2075,6 @@ export const template_visitors = {
/** @type {import('estree').ExpressionStatement[]} */
const lets = [];

/** @type {string | null} */
let namespace = null;

// Create a temporary context which picks up the init/update statements.
// They'll then be added to the function parameter of $.element
const element_id = b.id(context.state.scope.generate('$$element'));
Expand All @@ -2102,9 +2095,6 @@ export const template_visitors = {
for (const attribute of node.attributes) {
if (attribute.type === 'Attribute') {
attributes.push(attribute);
if (attribute.name === 'xmlns' && is_text_attribute(attribute)) {
namespace = attribute.value[0].data;
}
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
} else if (attribute.type === 'SpreadAttribute') {
attributes.push(attribute);
} else if (attribute.type === 'ClassDirective') {
Expand Down Expand Up @@ -2153,19 +2143,32 @@ export const template_visitors = {
}
}
inner.push(...inner_context.state.after_update);
inner.push(...create_block(node, 'dynamic_element', node.fragment.nodes, context));
inner.push(
...create_block(node, 'dynamic_element', node.fragment.nodes, {
...context,
state: {
...context.state,
metadata: {
...context.state.metadata,
namespace: determine_namespace_for_children(node, context.state.metadata.namespace)
}
}
})
);
context.state.after_update.push(
b.stmt(
b.call(
'$.element',
context.state.node,
get_tag,
node.metadata.svg === true
? b.true
: node.metadata.svg === false
? b.false
: b.literal(null),
inner.length === 0
? /** @type {any} */ (undefined)
: b.arrow([element_id, b.id('$$anchor')], b.block(inner)),
namespace === 'http://www.w3.org/2000/svg'
? b.literal(true)
: /** @type {any} */ (undefined)
: b.arrow([element_id, b.id('$$anchor')], b.block(inner))
)
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
} from '../../constants.js';
import {
clean_nodes,
determine_element_namespace,
determine_namespace_for_children,
escape_html,
infer_namespace,
transform_inspect_rune
Expand Down Expand Up @@ -482,11 +482,7 @@ function serialize_set_binding(node, context, fallback) {
*/
function get_attribute_name(element, attribute, context) {
let name = attribute.name;
if (
element.type === 'RegularElement' &&
!element.metadata.svg &&
context.state.metadata.namespace !== 'foreign'
) {
if (!element.metadata.svg && context.state.metadata.namespace !== 'foreign') {
name = name.toLowerCase();
// don't lookup boolean aliases here, the server runtime function does only
// check for the lowercase variants of boolean attributes
Expand Down Expand Up @@ -742,10 +738,10 @@ function serialize_element_spread_attributes(
}

const lowercase_attributes =
element.type !== 'RegularElement' || element.metadata.svg || is_custom_element_node(element)
element.metadata.svg || (element.type === 'RegularElement' && is_custom_element_node(element))
? b.false
: b.true;
const is_svg = element.type === 'RegularElement' && element.metadata.svg ? b.true : b.false;
const is_svg = element.metadata.svg ? b.true : b.false;
/** @type {import('estree').Expression[]} */
const args = [
b.array(values),
Expand Down Expand Up @@ -1146,7 +1142,7 @@ const template_visitors = {
RegularElement(node, context) {
const metadata = {
...context.state.metadata,
namespace: determine_element_namespace(node, context.state.metadata.namespace, context.path)
namespace: determine_namespace_for_children(node, context.state.metadata.namespace)
};

context.state.template.push(t_string(`<${node.name}`));
Expand Down Expand Up @@ -1236,11 +1232,16 @@ const template_visitors = {
context.state.init.push(b.stmt(b.call('$.validate_dynamic_element_tag', b.thunk(tag))));
}

const metadata = {
...context.state.metadata,
namespace: determine_namespace_for_children(node, context.state.metadata.namespace)
};
/** @type {import('./types').ComponentContext} */
const inner_context = {
...context,
state: {
...context.state,
metadata,
template: [],
init: []
}
Expand All @@ -1257,7 +1258,10 @@ const template_visitors = {
inner_context.state.template.push(t_string('>'));

const before = serialize_template(inner_context.state.template);
const main = create_block(node, node.fragment.nodes, context);
const main = create_block(node, node.fragment.nodes, {
...context,
state: { ...context.state, metadata }
});
const after = serialize_template([
t_expression(inner_id),
t_string('</'),
Expand Down
62 changes: 28 additions & 34 deletions packages/svelte/src/compiler/phases/3-transform/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ export function clean_nodes(
}

/**
* Infers the new namespace for the children of a node.
* Infers the namespace for the children of a node that should be used when creating the `$.template(...)`.
* @param {import('#compiler').Namespace} namespace
* @param {import('#compiler').SvelteNode} parent
* @param {import('#compiler').SvelteNode[]} nodes
Expand All @@ -201,19 +201,28 @@ export function infer_namespace(namespace, parent, nodes, path) {
path.at(-1)
: parent;

if (
namespace !== 'foreign' &&
if (namespace !== 'foreign') {
if (parent_node?.type === 'RegularElement' && parent_node.name === 'foreignObject') {
return 'html';
}

if (parent_node?.type === 'RegularElement' || parent_node?.type === 'SvelteElement') {
return parent_node.metadata.svg ? 'svg' : 'html';
}

// Re-evaluate the namespace inside slot nodes that reset the namespace
(parent_node === undefined ||
if (
parent_node === undefined ||
parent_node.type === 'Root' ||
parent_node.type === 'Component' ||
parent_node.type === 'SvelteComponent' ||
parent_node.type === 'SvelteFragment' ||
parent_node.type === 'SnippetBlock')
) {
const new_namespace = check_nodes_for_namespace(nodes, 'keep');
if (new_namespace !== 'keep' && new_namespace !== 'maybe_html') {
namespace = new_namespace;
parent_node.type === 'SnippetBlock'
) {
const new_namespace = check_nodes_for_namespace(nodes, 'keep');
if (new_namespace !== 'keep' && new_namespace !== 'maybe_html') {
return new_namespace;
}
}
}

Expand All @@ -229,7 +238,7 @@ export function infer_namespace(namespace, parent, nodes, path) {
*/
function check_nodes_for_namespace(nodes, namespace) {
for (const node of nodes) {
if (node.type === 'RegularElement') {
if (node.type === 'RegularElement' || node.type === 'SvelteElement') {
if (!node.metadata.svg) {
namespace = 'html';
break;
Expand Down Expand Up @@ -279,36 +288,21 @@ function check_nodes_for_namespace(nodes, namespace) {
}

/**
* @param {import('#compiler').RegularElement} node
* Determines the namespace the children of this node are in.
* @param {import('#compiler').RegularElement | import('#compiler').SvelteElement} node
* @param {import('#compiler').Namespace} namespace
* @param {import('#compiler').SvelteNode[]} path
* @returns {import('#compiler').Namespace}
*/
export function determine_element_namespace(node, namespace, path) {
if (namespace !== 'foreign') {
let parent = path.at(-1);
if (parent?.type === 'Fragment') {
parent = path.at(-2);
}
export function determine_namespace_for_children(node, namespace) {
if (namespace === 'foreign') {
return namespace;
}

if (node.name === 'foreignObject') {
return 'html';
} else if (
namespace !== 'svg' ||
parent?.type === 'Component' ||
parent?.type === 'SvelteComponent' ||
parent?.type === 'SvelteFragment' ||
parent?.type === 'SnippetBlock'
) {
if (node.metadata.svg) {
return 'svg';
} else {
return 'html';
}
}
if (node.name === 'foreignObject') {
return 'html';
}

return namespace;
return node.metadata.svg ? 'svg' : 'html';
}

/**
Expand Down
7 changes: 7 additions & 0 deletions packages/svelte/src/compiler/types/template.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,13 @@ export interface SvelteElement extends BaseElement {
type: 'SvelteElement';
name: 'svelte:element';
tag: Expression;
metadata: {
/**
* `true`/`false` if this is definitely (not) an svg element.
* `null` means we can't know statically.
*/
svg: boolean | null;
};
}

export interface SvelteFragment extends BaseElement {
Expand Down
3 changes: 3 additions & 0 deletions packages/svelte/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,6 @@ export const DOMBooleanAttributes = [
'seamless',
'selected'
];

export const namespace_svg = 'http://www.w3.org/2000/svg';
export const namespace_html = 'http://www.w3.org/1999/xhtml';
Loading
Loading