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
Prev Previous commit
Next Next commit
fix: properly handle scoped class for void and foreign elements
  • Loading branch information
@jhefferman-sfdc
jhefferman-sfdc committed Dec 4, 2024
commit 6ea8218cf8ba13627e7890613255506371e9c134
24 changes: 0 additions & 24 deletions packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-closing-tags/expected.html
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions ...-server/src/__tests__/fixtures/render-correct-closing-tags/modules/x/elements/elements.js
View file
Open in desktop

This file was deleted.

0 ...res/render-correct-closing-tags/error.txt → ...__/fixtures/render-correct-tags/error.txt
View file
Open in desktop
File renamed without changes.
26 changes: 26 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,26 @@
<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>
<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>
0 ...ures/render-correct-closing-tags/index.js → ...s__/fixtures/render-correct-tags/index.js
View file
Open in desktop
File renamed without changes.
5 changes: 4 additions & 1 deletion ...ing-tags/modules/x/elements/elements.html → ...ect-tags/modules/x/elements/elements.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,8 @@
<image xlink:title="title"></image>
</svg>
<!-- Rendered with inner-html -->
<svg lwc:inner-html={innerHtml}></svg>
<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;
}
19 changes: 6 additions & 13 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,20 +242,17 @@ export const Element: Transformer<IrElement | IrExternalComponent | IrSlot> = fu
childContent = [];
}

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

if (isForeignElement && !hasChildren) {
return [bYield(b.literal(`<${node.name}`)), ...yieldAttrsAndProps, bYield(b.literal(`/>`))];
}
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}>`)),
isForeignSelfClosingElement ? bYield(b.literal(`/>`)) : bYield(b.literal(`>`)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
isForeignSelfClosingElement ? bYield(b.literal(`/>`)) : bYield(b.literal(`>`)),
bYield(b.literal(isForeignSelfClosingElement ? `/>` : `>`)),

...(isSelfClosingElement ? [] : [...childContent, bYield(b.literal(`</${node.name}>`))]),
].filter(Boolean);
};