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
+90 −10
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
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
4 changes: 3 additions & 1 deletion packages/@lwc/engine-server/src/__tests__/fixtures.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ async function compileFixture({
// IGNORED_SLOT_ATTRIBUTE_IN_CHILD is fine; it is used in some of these tests
message.includes('LWC1201') ||
message.includes('-h-t-m-l') ||
code === 'CIRCULAR_DEPENDENCY';
code === 'CIRCULAR_DEPENDENCY' ||
// template-compiler -> index -> validateElement generates UNKNOWN_HTML_TAG_IN_TEMPLATE for MathML elements
message.includes('LWC1123');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is bizarre... we should fix this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated the comment with a TODO on that issue.

if (!shouldIgnoreWarning) {
throw new Error(message);
}
Expand Down
Empty file added packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-tags/error.txt
View file
Open in desktop
Empty file.
42 changes: 42 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-tags/expected.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<x-elements class="lwc-4q9u0sqsfc0-host" data-lwc-host-scope-token="lwc-4q9u0sqsfc0-host">
<template shadowrootmode="open">
<style class="lwc-4q9u0sqsfc0" type="text/css">
:host {background: blue;}
</style>
<a class="lwc-4q9u0sqsfc0" href="https://www.salesforce.com/">
</a>
<label class="lwc-4q9u0sqsfc0" for="textInput">
Some input
</label>
<input class="lwc-4q9u0sqsfc0" id="textInput" type="text">
<svg class="lwc-4q9u0sqsfc0">
<pattern class="lwc-4q9u0sqsfc0">
<image class="lwc-4q9u0sqsfc0" xlink:href="https://www.salesforce.com/"/>
</pattern>
<image class="lwc-4q9u0sqsfc0" xlink:title="title"/>
</svg>
<math class="lwc-4q9u0sqsfc0">
<mi class="lwc-4q9u0sqsfc0">
π
</mi>
<mo class="lwc-4q9u0sqsfc0">
⁢
</mo>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be great to have an example of a self-closing tag in MathML to make sure we do it correctly.

Also &InvisibleTimes; is very confusing here since it renders invisibly. Tricked me into thinking that this should be self-closing when it isn't.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait – this should be self-closing, right? Since it's actually just a comment?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

Wait – this should be self-closing, right? Since it's actually just a comment?

I'm not sure... it is actually appearing as an invisible character aka ZWJ (yellow box) in Visual Studio in both the component and the expected output? It passes in both engine-server and ssr-compiler (not to say that means it is correct). It is a confusing example though and I can remove it.

It'd be great to have an example of a self-closing tag in MathML to make sure we do it correctly.

Sorry I didn't add that. Unfortunately we have another engine-server bug here...

  • <plus/> and <plus></plus> is rendered as <plus></plus> by engine-server
  • <plus/> and <plus></plus> is rendered as <plus/> by ssr-compiler (expected output)

Shall I open another issue?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed that there is literally an invisible character in there, outside of the comment. Yeah I would switch to something that's visible.

Shall I open another issue?

Yes, I would open a bug for this. All non-HTML childless components should render as self-closing (with the />).

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

Ok sounds good, opened #5015 on engine-server, referenced it in the example and removed the invisible character example

<msup class="lwc-4q9u0sqsfc0">
<mi class="lwc-4q9u0sqsfc0">
r
</mi>
<mn class="lwc-4q9u0sqsfc0">
2
</mn>
</msup>
</math>
<svg class="lwc-4q9u0sqsfc0">
<pattern class="lwc-4q9u0sqsfc0">
<image xlink:href="https://www.salesforce.com/">
</image>
</pattern>
<image class="lwc-4q9u0sqsfc0" xlink:title="title"/>
</svg>
</template>
</x-elements>
3 changes: 3 additions & 0 deletions packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-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';
30 changes: 30 additions & 0 deletions ...engine-server/src/__tests__/fixtures/render-correct-tags/modules/x/elements/elements.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<template>
<!-- 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>
<math>
<mi>π<!-- p --></mi>
<mo>⁢<!-- &InvisibleTimes; --></mo>
<msup>
<mi>r</mi>
<mn>2</mn>
</msup>
</math>
<!-- Rendered with inner-html -->
<svg>
<pattern lwc:inner-html={foreignNamespaceInnerHtml}></pattern>
<image xlink:title="title"></image>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be great to add a MathML block here for completeness.

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

including <math> namespace elements passed for ssr-compiler but generates a warning when engine-server fixtures are ran as it appears MathML elements are not recognized as a knownTag by template-compiler -> validateElement. To enable testing MathML for both I suppressed the warning here.

Is that OK or would you rather modify template-compiler -> validateElement to recognize MathML elements, or...?

I suppose it could be argued that the same warning / tag-check should be there for @lwc/ssr-compiler too, or not needed? I'm not sure as tags used in the correct namespace shouldn't really generate a warning, so should we be reproducing this behavior in v2 regardless?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, opened an issue for this: #5010

MathML should be supported.

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 thanks, seems wrong. I updated the warning suppression in the fixture to reference the issue you opened.

</template>
5 changes: 5 additions & 0 deletions ...c/engine-server/src/__tests__/fixtures/render-correct-tags/modules/x/elements/elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { LightningElement, api } from 'lwc';

export default class extends LightningElement {
@api foreignNamespaceInnerHtml = '<image xlink:href="https://www.salesforce.com/"></image>';
}
3 changes: 3 additions & 0 deletions ...-server/src/__tests__/fixtures/render-correct-tags/modules/x/elements/elements.scoped.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
:host {
background: blue;
}
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
14 changes: 7 additions & 7 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 @@ -227,10 +227,6 @@ export const Element: Transformer<IrElement | IrExternalComponent | IrSlot> = fu
return result;
});

if (isVoidElement(node.name, HTML_NAMESPACE)) {
return [bYield(b.literal(`<${node.name}`)), ...yieldAttrsAndProps, bYield(b.literal(`>`))];
}

let childContent: EsStatement[];
// An element can have children or lwc:inner-html, but not both
// If it has both, the template compiler will throw an error before reaching here
Expand All @@ -246,13 +242,17 @@ export const Element: Transformer<IrElement | IrExternalComponent | IrSlot> = fu
childContent = [];
}

const isForeignSelfClosingElement =
node.namespace !== HTML_NAMESPACE && childContent.length === 0;
const isSelfClosingElement =
isVoidElement(node.name, HTML_NAMESPACE) || isForeignSelfClosingElement;

return [
bYield(b.literal(`<${node.name}`)),
// If we haven't already prefixed the scope token to an existing class, add an explicit class here
...(hasClassAttribute ? [] : [bConditionallyYieldScopeTokenClass()]),
...yieldAttrsAndProps,
bYield(b.literal(`>`)),
...childContent,
bYield(b.literal(`</${node.name}>`)),
bYield(b.literal(isForeignSelfClosingElement ? `/>` : `>`)),
...(isSelfClosingElement ? [] : [...childContent, bYield(b.literal(`</${node.name}>`))]),
].filter(Boolean);
};