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.

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

[labs/ssr] Binding to textarea results comment node errantly added inside textarea #3663

Closed
kevinpschaaf opened this issue Feb 11, 2023 · 4 comments · Fixed by #3667 or #3717
Closed

Comments

@kevinpschaaf
Copy link
Member

kevinpschaaf commented Feb 11, 2023

Which package(s) are affected?

SSR (@lit-labs/ssr)

Description

Server rendering a <textarea> with a binding in property/attribute/element position results in a comment node being added inside the textarea content, which is rendered as plain text rather than being parsed as an HTML comment.

Reported by user Levi in Discord thread: https://discord.com/channels/1012791295170859069/1073542653402157076

Reproduction

When server-rendering the following template using @lit-labs/ssr:

renderInput(id: string) {
  return html`<textarea name="${id}"></textarea>`;
}

A comment node shows up inside the textarea:

<textarea id="name"><!--lit-node 0--></textarea>

Workaround

Avoid binding to the textarea node.

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

@lit-labs/ssr 3.0.1

Browser/OS/Node environment

Any

@kevinpschaaf
Copy link
Member Author

kevinpschaaf commented Feb 11, 2023

We need a special case for the "raw text element" set (script, style, textarea, title).

The hydration code already supports adding a node after an element (as opposed to the first child) and looking at the previous sibling first (before the parent) to handle the case of void elements like <input> that have no children:

// For void elements, the node the comment was referring to will be
// the previousSibling; for non-void elements, the comment is guaranteed
// to be the first child of the element (i.e. it won't have a previousSibling
// meaning it should use the parentElement)
const node = comment.previousElementSibling ?? comment.parentElement;

If we disallow bindings within raw text elements, then a fix would be as simple as skipping to the end of the raw text element and adding the node marker after the close tag. Not sure if that's viable (style, and maybe title are both common things you might want to bind into?).

We need to see a part marker for the node before any other bindings, otherwise the part/value order won't agree. Perhaps we could get away with saying you can either bind to a raw text element's properties/attrs or to its child content, but not both?

It seems like supporting both would require a big change to either use an attribute rather than a comment marker for identifying property/attribute/element bindings (and can't use the current comment-based TreeWalker) 👎, or move the comment marker ahead of the element, which requires buffering the open tag 👎 , or rearrange parts during hydration somehow 👎 .

@justinfagnani
Copy link
Member

I suspect we really do want to keep a comment-only system, but we should measure. I wonder how expensive it is to do a qSA for raw text elements?

I think moving the marker to be in front of the node is an interesting option. I don't see how buffering is an issue... In SSR we know what tags have bindings after template prep, add in hydration we'll know the next tag has bindings.

This seems good as you can't validly have bound tags inside a raw text element, so the comment will parse as a comment is all valid templates.

@kevinpschaaf
Copy link
Member Author

Ah right, I was thinking it affected the streaming side, but you're right we can work it out during template prep.

Concretely, during template prep we'd need to insert a node-marker opcode before the e.g. attribute-part opcodes. That's still out of order processing-wise, but that's easier to deal with since it's in the one-time work, in a data structure we can more easily mess with.

@kevinpschaaf
Copy link
Member Author

I tried a quick fix for this and it was pretty chill (re-arranging the opcodes so possible-node-marker comes first. I'll make a PR.

cc @augustjk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants