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

jhefferman-sfdc · 2024-12-03T16:06:22Z

Details

Does this pull request introduce a breaking change?

😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

🤞 No, it does not introduce an observable change.

GUS work item

W-16871512

packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-closing-tags/expected.html

wjhsf · 2024-12-03T16:35:51Z

packages/@lwc/ssr-compiler/src/compile-template/transformers/element.ts

+    const isForeignElement = node.namespace !== HTML_NAMESPACE;
+    const hasChildren = childContent.length > 0;
+
+    if (isForeignElement && !hasChildren) {


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

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.

wjhsf · 2024-12-03T17:41:12Z

packages/@lwc/ssr-compiler/src/compile-template/transformers/element.ts

+    const hasChildren = childContent.length > 0;
+
+    if (isForeignElement && !hasChildren) {
+        return [bYield(b.literal(`<${node.name}`)), ...yieldAttrsAndProps, bYield(b.literal(`/>`))];


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

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.

wjhsf · 2024-12-03T17:45:16Z

...e-server/src/__tests__/fixtures/render-correct-closing-tags/modules/x/elements/elements.html

@@ -0,0 +1,19 @@
+<template>


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

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

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.

Ok sounds good, I opened this issue

nolanlawson · 2024-12-04T17:15:05Z

...wc/engine-server/src/__tests__/fixtures/render-correct-tags/modules/x/elements/elements.html

+    <svg>
+        <pattern lwc:inner-html={foreignNamespaceInnerHtml}></pattern>
+        <image xlink:title="title"></image>
+    </svg>


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

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?

Yep, opened an issue for this: #5010

MathML should be supported.

Ok thanks, seems wrong. I updated the warning suppression in the fixture to reference the issue you opened.

nolanlawson · 2024-12-04T17:16:41Z

packages/@lwc/ssr-compiler/src/compile-template/transformers/element.ts

-        bYield(b.literal(`>`)),
-        ...childContent,
-        bYield(b.literal(`</${node.name}>`)),
+        isForeignSelfClosingElement ? bYield(b.literal(`/>`)) : bYield(b.literal(`>`)),


Suggested change

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

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

nolanlawson

LGTM

…test

nolanlawson · 2024-12-05T18:13:29Z

packages/@lwc/engine-server/src/__tests__/fixtures.spec.ts

-                code === 'CIRCULAR_DEPENDENCY';
+                code === 'CIRCULAR_DEPENDENCY' ||
+                // template-compiler -> index -> validateElement generates UNKNOWN_HTML_TAG_IN_TEMPLATE for MathML elements
+                message.includes('LWC1123');


This is bizarre... we should fix this.

Spurious warning (UNKNOWN_HTML_TAG_IN_TEMPLATE) for MathML elements #5010

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

nolanlawson · 2024-12-05T18:20:11Z

packages/@lwc/engine-server/src/__tests__/fixtures/render-correct-tags/expected.html

+      </mi>
+      <mo class="lwc-4q9u0sqsfc0">
+        ⁢
+      </mo>


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

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

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

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?

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 />).

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

fix: correctly render foreign element closing tags

7287465

jhefferman-sfdc requested a review from a team as a code owner December 3, 2024 16:06

jhefferman-sfdc assigned wjhsf and nolanlawson Dec 3, 2024

wjhsf reviewed Dec 3, 2024

View reviewed changes

fix: properly handle scoped class for void and foreign elements

6ea8218

jhefferman-sfdc requested a review from wjhsf December 4, 2024 00:18

nolanlawson reviewed Dec 4, 2024

View reviewed changes

nolanlawson approved these changes Dec 4, 2024

View reviewed changes

jhefferman-sfdc added 3 commits December 4, 2024 13:36

fix: address review comments

66b4a37

fix: remove math example for now

ff5fe94

fix: ignore UNKNOWN_HTML_TAG_IN_TEMPLATE warning to allow for MathML …

35d5797

…test

jhefferman-sfdc requested a review from nolanlawson December 5, 2024 17:17

nolanlawson reviewed Dec 5, 2024

View reviewed changes

nolanlawson approved these changes Dec 5, 2024

View reviewed changes

fix: update tag warning suppression to reference issue

8d6d2e4

jhefferman-sfdc force-pushed the jhefferman/ssr/foreign-element-render-void-fix branch from cf45746 to 8d6d2e4 Compare December 6, 2024 16:51

jhefferman-sfdc added 2 commits December 6, 2024 09:56

fix: simpler MathML example

5c6bfe6

chore: merge master

0cab84d

nolanlawson approved these changes Dec 9, 2024

View reviewed changes

jhefferman-sfdc merged commit ee2edae into master Dec 10, 2024
11 checks passed

jhefferman-sfdc deleted the jhefferman/ssr/foreign-element-render-void-fix branch December 10, 2024 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jhefferman-sfdc commented Dec 3, 2024

wjhsf Dec 3, 2024

jhefferman-sfdc Dec 3, 2024

wjhsf Dec 3, 2024

jhefferman-sfdc Dec 4, 2024

wjhsf Dec 3, 2024

jhefferman-sfdc Dec 4, 2024 •

edited

Loading

nolanlawson Dec 4, 2024

jhefferman-sfdc Dec 5, 2024

nolanlawson Dec 4, 2024

jhefferman-sfdc Dec 5, 2024 •

edited

Loading

nolanlawson Dec 5, 2024

jhefferman-sfdc Dec 5, 2024

nolanlawson Dec 4, 2024

nolanlawson left a comment

nolanlawson Dec 5, 2024

nolanlawson Dec 5, 2024

jhefferman-sfdc Dec 5, 2024

nolanlawson Dec 5, 2024

nolanlawson Dec 5, 2024

jhefferman-sfdc Dec 5, 2024 •

edited

Loading

nolanlawson Dec 5, 2024

jhefferman-sfdc Dec 6, 2024 •

edited

Loading

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

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

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

Conversation

jhefferman-sfdc commented Dec 3, 2024

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhefferman-sfdc Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

jhefferman-sfdc Dec 4, 2024 •

edited

Loading

jhefferman-sfdc Dec 5, 2024 •

edited

Loading

jhefferman-sfdc Dec 5, 2024 •

edited

Loading

jhefferman-sfdc Dec 6, 2024 •

edited

Loading