fix(template-compiler): fix static td/th/etc elements #2888
Conversation
| HTML_TAG.TH, | ||
| HTML_TAG.THEAD, | ||
| HTML_TAG.TR, | ||
| ]); |
There was a problem hiding this comment.
I don't see why anybody would put an <html> inside of an LWC template, but we might as well cover every case.
| isElement(element) && | ||
| // Tags like <th> and <td> are okay as static fragments, but only if they're not at the top level | ||
| !isNonTopLevelTag(element) | ||
| ) { |
There was a problem hiding this comment.
I actually noticed this was a problem because in one of the snapshots we had a fragment like:
<table><tbody><tr><td></td></tr></tbody></table>We should totally allow these kinds of fragments. There's no problem with them. The only problem is if elements like tbody and td appear at the top level of a fragment.
| // Tags that cannot be parsed at the top level of a fragment. Generated from | ||
| // https://github.com/sindresorhus/html-tags/blob/95dcdd5/index.js | ||
| // using the test: | ||
| // document.createRange().createContextualFragment(`<${tag}></${tag}>`).firstChild === null |
There was a problem hiding this comment.
I think it's important to call out that we are doing this because we still support IE11. If it wasn't for IE11, we wouldn't need to use Range.createContextFragment and use the <template> element to parse fragments.
Could we add a comment here and in createFragment under @lwc/engine-dom indicating that all of this should be dropped when we are moving away from IE11.
Another option worth calling out would be to disable entirely static VNode hoisting in compat node and transition to <template> element for parsing fragments.
Pros:
- No need to maintain this special logic in the template compiler
- Better performance for good citizens
Cons: - Support for 2 template compiler outputs at runtime
At this stage of the release, I would favor the approach with the least amount of changes (the current PR).
There was a problem hiding this comment.
Good call. I'll add the comment.
I looked back at the previous perf analysis I did (#2781 (comment)), and it looks like using a <template> and setting innerHTML on it is just as fast as createContextualFragment. So yeah, we could switch to using that, if not for IE11.
| codeGen.staticNodes.has(element) && | ||
| isElement(element) && | ||
| // Tags like <th> and <td> are okay as static fragments, but only if they're not at the top level | ||
| !isUnsafeTopLevelSerializableElement(element) |
There was a problem hiding this comment.
I'm not a fan of the double negation like this... a more elegant approach is to have something like: isSafeTopLevelSerializableElement(element) instead @nolanlawson
Details
Fixes #2887. Skips the static content optimization for elements like
<th>and<td>that cannot be parsed at the top level of a fragment.Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
Elements like
<td>and<tr>with static content will no longer throw an error.GUS work item
W-11308172