Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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
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
7 changes: 5 additions & 2 deletions packages/svelte/src/compiler/phases/1-parse/state/element.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,11 @@ export default function tag(parser) {
name,
attributes: [],
fragment: create_fragment(true),
parent: null
};
parent: null,
metadata: {
svg: false
}
};

parser.allow_whitespace();

Expand Down
27 changes: 25 additions & 2 deletions packages/svelte/src/compiler/phases/2-analyze/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1092,8 +1092,31 @@ 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);

const is_svg_element =
(node.tag.type === 'Identifier' && node.tag.name && SVGElements.includes(node.tag.name)) ||

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks wrong - the name of the identifier shouldn't be used to infer whether or not this is an SVG element

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right, I thought it was the evaluated name, but it's just the variable name

however, how would you determine the tag value in this case? don't we have to do it at runtime?

<script>
	export let a = 'svg';
	export let b = 'path';
</script>

<svelte:element this={a}>
	<svelte:element this={b}></svelte:element>
</svelte:element>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can't determine it statically in this case, and determining it at runtime would mean loading a long list of tags into the runtime, which I'm not sure if it's worth it. I'm ok with this not working for now, AFAIK this also doesn't work in Svelte 4.

@eEQK eEQK Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it does not work in Svelte 4. Should I also add other tests that will be failing? using slots/snippets is not working either (both in svelte 4 and 5)

as for doing this at runtime - don't we only have to check if one of the parents is an svg without custom namespace?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It needs to be determined at runtime, yeah, but I'm pretty sure we only need to check two cases:

  • value is "svg" — it's an SVG element
  • value is "foreignObject" — it's an HTML element
  • otherwise inherit namespace from parent element

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a go at this and it's unfortantely not possible to determine in some cases:
If this is client-side only (i.e. no hydration) and the first run, then the anchor from which we'll get the parent element will not be attached to its parent yet, so the parent element is null and we can't know.

(node.tag.type === 'Literal' &&
node.tag.value &&
SVGElements.includes(node.tag.value.toString()));
Comment thread
dummdidumm marked this conversation as resolved.
Outdated

const metadata = node.metadata;
metadata.svg = !!is_svg_element;

if (metadata.svg) {
return;
}

for (const ancestor of context.path) {
if (ancestor.type === 'RegularElement' || ancestor.type === 'SvelteElement') {
metadata.svg = ancestor.metadata.svg;

if (metadata.svg) {
Comment thread
dummdidumm marked this conversation as resolved.
Outdated
break;
}
}
}
}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ import {
EACH_IS_CONTROLLED,
EACH_IS_IMMUTABLE,
EACH_ITEM_REACTIVE,
EACH_KEYED
EACH_KEYED,
namespace_svg
} from '../../../../../constants.js';
import { regex_is_valid_identifier } from '../../../patterns.js';
import { javascript_visitors_runes } from './javascript-runes.js';
Expand Down Expand Up @@ -2099,12 +2100,11 @@ export const template_visitors = {
}
};

namespace = node.metadata.svg ? namespace_svg : null;
Comment thread
dummdidumm marked this conversation as resolved.
Outdated

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;
}
Comment thread
dummdidumm marked this conversation as resolved.
} else if (attribute.type === 'SpreadAttribute') {
attributes.push(attribute);
} else if (attribute.type === 'ClassDirective') {
Expand Down Expand Up @@ -2160,12 +2160,10 @@ export const template_visitors = {
'$.element',
context.state.node,
get_tag,
b.literal(namespace),
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
4 changes: 4 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,10 @@ export interface SvelteElement extends BaseElement {
type: 'SvelteElement';
name: 'svelte:element';
tag: Expression;
metadata: {
/** `true` if this is an svg element */
svg: boolean;
};
}

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';
23 changes: 15 additions & 8 deletions packages/svelte/src/internal/client/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@ import {
create_dynamic_component_block,
create_snippet_block
} from './block.js';
import { PassiveDelegatedEvents, DelegatedEvents, AttributeAliases } from '../../constants.js';
import {
PassiveDelegatedEvents,
DelegatedEvents,
AttributeAliases,
namespace_svg,
namespace_html
} from '../../constants.js';
import { create_fragment_from_html, insert, reconcile_html, remove } from './reconciler.js';
import {
render_effect,
Expand Down Expand Up @@ -1576,11 +1582,11 @@ function swap_block_dom(block, from, to) {
/**
* @param {Comment} anchor_node
* @param {() => string} tag_fn
* @param {null | string} namespace
* @param {undefined | ((element: Element, anchor: Node) => void)} render_fn
* @param {any} is_svg
* @returns {void}
*/
export function element(anchor_node, tag_fn, render_fn, is_svg = false) {
export function element(anchor_node, tag_fn, namespace, render_fn) {
const block = create_dynamic_element_block();
hydrate_block_anchor(anchor_node);
let has_mounted = false;
Expand All @@ -1604,12 +1610,13 @@ export function element(anchor_node, tag_fn, render_fn, is_svg = false) {
// Managed effect
const render_effect_signal = render_effect(
() => {
const ns = namespace ?? tag === 'svg' ? namespace_svg : null;
Comment thread
dummdidumm marked this conversation as resolved.
Outdated
const next_element = tag
? current_hydration_fragment !== null
? /** @type {HTMLElement | SVGElement} */ (current_hydration_fragment[0])
: is_svg
? document.createElementNS('http://www.w3.org/2000/svg', tag)
: document.createElement(tag)
: ns
? document.createElementNS(ns, tag)
: document.createElement(tag)
: null;
const prev_element = element;
if (prev_element !== null) {
Expand Down Expand Up @@ -2079,7 +2086,7 @@ export function cssProps(anchor, is_html, props, component) {
tag = document.createElement('div');
tag.style.display = 'contents';
} else {
tag = document.createElementNS('http://www.w3.org/2000/svg', 'g');
tag = document.createElementNS(namespace_svg, 'g');
}
insert(tag, null, anchor);
component_anchor = empty();
Expand Down Expand Up @@ -2610,7 +2617,7 @@ export function spread_dynamic_element_attributes(node, prev, attrs, css_hash) {
/** @type {Element & ElementCSSInlineStyle} */ (node),
prev,
attrs,
node.namespaceURI !== 'http://www.w3.org/2000/svg',
node.namespaceURI !== namespace_svg,
css_hash
);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { test } from '../../test';

export default test({
test({ assert, target }) {
const svg = target.querySelector('svg');
const path = target.querySelector('path');
assert.equal(svg?.namespaceURI, 'http://www.w3.org/2000/svg');
assert.equal(path?.namespaceURI, 'http://www.w3.org/2000/svg');
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<script>
const iconNode = [["path", { "d": "M21 12a9 9 0 1 1-6.219-8.56" }]];
</script>

<svg
width="24" height="24"
viewBox="0 0 24 24"
fill="none"
stroke="currentColor" stroke-width="2">
{#each iconNode as [tag, attrs]}
<svelte:element this={tag} {...attrs}/>
{/each}
</svg>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { test } from '../../test';

export default test({
html: '<svg><path></path></svg>',

test({ assert, target }) {
const svg = target.querySelector('svg');
const rect = target.querySelector('path');
assert.equal(svg?.namespaceURI, 'http://www.w3.org/2000/svg');
assert.equal(rect?.namespaceURI, 'http://www.w3.org/2000/svg');
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<script>
// ensure these are treated as dynamic, despite whatever
// optimisations we might apply
export let svg = 'svg';
export let path = 'path';
</script>

<svelte:element this={svg}>
<svelte:element this={path}></svelte:element>
</svelte:element>
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default function Svelte_element($$anchor, $$props) {
var fragment = $.comment($$anchor);
var node = $.child_frag(fragment);

$.element(node, tag);
$.element(node, tag, null);
$.close_frag($$anchor, fragment);
$.pop();
}