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.

Sign up for GitHub

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(ssr): correctly render foreign namespace element closing tags #4992

Merged
merged 8 commits into from
Dec 10, 2024
Merged

fix(ssr): correctly render foreign namespace element closing tags #4992

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7287465
fix: correctly render foreign element closing tags
jhefferman-sfdc Dec 3, 2024
6ea8218
fix: properly handle scoped class for void and foreign elements
jhefferman-sfdc Dec 4, 2024
66b4a37
fix: address review comments
jhefferman-sfdc Dec 4, 2024
ff5fe94
fix: remove math example for now
jhefferman-sfdc Dec 5, 2024
35d5797
fix: ignore UNKNOWN_HTML_TAG_IN_TEMPLATE warning to allow for MathML …
jhefferman-sfdc Dec 5, 2024
8d6d2e4
fix: update tag warning suppression to reference issue
jhefferman-sfdc Dec 5, 2024
5c6bfe6
fix: simpler MathML example
jhefferman-sfdc Dec 6, 2024
0cab84d
chore: merge master
jhefferman-sfdc Dec 6, 2024
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
Empty file added packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-closing-tags/error.txt
View file
Open in desktop
Empty file.
24 changes: 24 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-closing-tags/expected.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<x-elements>
<template shadowrootmode="open">
<a href="https://www.salesforce.com/">
</a>
<label for="textInput">
Some input
</label>
<input id="textInput" type="text">
<svg>
<pattern>
<image xlink:href="https://www.salesforce.com/"/>
</pattern>
<image xlink:title="title"/>
</svg>
<svg>
<pattern>
<image xlink:href="https://www.salesforce.com/">
</image>
</pattern>
<image xlink:title="title">
</image>
</svg>
wjhsf marked this conversation as resolved.
Show resolved Hide resolved
</template>
</x-elements>
3 changes: 3 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-closing-tags/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const tagName = 'x-elements';
export { default } from 'x/elements';
export * from 'x/elements';
19 changes: 19 additions & 0 deletions ...erver/src/__tests__/fixtures/render-correct-closing-tags/modules/x/elements/elements.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<template>
Copy link
Contributor

Choose a reason for hiding this comment

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

Add test cases for class attribute and lwc:inner-html on input or textarea or somthing.

Copy link
Contributor Author

@jhefferman-sfdc jhefferman-sfdc Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Adding lwc:inner-html to a void element like input throws an exception in v1 (engine-server) and renders nothing so I didn't add it. For v2 it doesn't throw but it doesn't render anything. Should we open an issue or make a change to throw when inner-html is used on void elements? Or is it enough that it is a sort of noop

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should open an issue to make it throw an error. lwc:inner-html doesn't make any sense for void elements, so throwing an error is a good behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sounds good, I opened this issue

<!-- Empty non-void HTML namespace element -->
<a href="https://www.salesforce.com/"></a>
<!-- Non-empty non-void HTML namespace element -->
<label for="textInput">Some input</label>
<!-- Void HTML namespace element -->
<input id="textInput" type="text">
<!-- Foreign namespace elements -->
<svg>
<!-- Non-empty foreign element -->
<pattern>
<image xlink:href="https://www.salesforce.com/"></image>
</pattern>
<!-- Empty foreign element -->
<image xlink:title="title"></image>
</svg>
<!-- Rendered with inner-html -->
<svg lwc:inner-html={innerHtml}></svg>
</template>
6 changes: 6 additions & 0 deletions ...-server/src/__tests__/fixtures/render-correct-closing-tags/modules/x/elements/elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { LightningElement, api } from 'lwc';

export default class extends LightningElement {
@api innerHtml =
'<pattern><image xlink:href="https://www.salesforce.com/"></image></pattern><image xlink:title="title"></image>';
}
2 changes: 0 additions & 2 deletions packages/@lwc/ssr-compiler/src/__tests__/utils/expected-failures.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export const expectedFailures = new Set([
'attribute-component-global-html/index.js',
'attribute-global-html/as-component-prop/undeclared/index.js',
'attribute-global-html/as-component-prop/without-@api/index.js',
'attribute-namespace/index.js',
'attribute-style/basic/index.js',
'attribute-style/dynamic/index.js',
'dynamic-slots/index.js',
Expand All @@ -38,7 +37,6 @@ export const expectedFailures = new Set([
'superclass/render-in-superclass/no-template-in-subclass/index.js',
'superclass/render-in-superclass/unused-default-in-subclass/index.js',
'superclass/render-in-superclass/unused-default-in-superclass/index.js',
'svgs/index.js',
'wire/errors/throws-when-computed-prop-is-expression/index.js',
'wire/errors/throws-when-computed-prop-is-let-variable/index.js',
'wire/errors/throws-when-computed-prop-is-regexp-literal/index.js',
Expand Down
7 changes: 7 additions & 0 deletions packages/@lwc/ssr-compiler/src/compile-template/transformers/element.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,13 @@ export const Element: Transformer<IrElement | IrExternalComponent | IrSlot> = fu
childContent = [];
}

const isForeignElement = node.namespace !== HTML_NAMESPACE;
const hasChildren = childContent.length > 0;

if (isForeignElement && !hasChildren) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any non-HTML elements that are not self-closing when empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, or at least I couldn't find any SVG or MathML cases. The spec just says "Foreign elements must either have a start tag and an end tag, or a start tag that is marked as self-closing, in which case they must not have an end tag." but that is not exactly what you are asking.

return [bYield(b.literal(`<${node.name}`)), ...yieldAttrsAndProps, bYield(b.literal(`/>`))];
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be merged as a ternary in the return statement below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also removed the separate return for void html elements and they return here too. They were also missing the scope class and failed the test once that was added.

}

return [
bYield(b.literal(`<${node.name}`)),
// If we haven't already prefixed the scope token to an existing class, add an explicit class here
Expand Down