From e00f6f52199d5dbc08d4c15f62380422e77cde7f Mon Sep 17 00:00:00 2001
From: Kevin Schaaf <kschaaf@google.com>
Date: Tue, 14 Feb 2023 10:15:32 -0800
Subject: [PATCH] [labs/ssr] Emit lit-node marker before node to support raw
 text nodes. (#3667)

* Emit lit-node marker before node to support raw text nodes.
Fixes #3663

* Fix copy/paste mistake.
---
 .changeset/soft-turtles-protect.md            |  6 ++
 packages/labs/ssr/src/lib/render-value.ts     | 77 +++++++++++--------
 .../ssr/src/test/integration/tests/basic.ts   | 43 +++++++++++
 .../labs/ssr/src/test/lib/render-lit_test.ts  | 36 ++++-----
 packages/lit-html/src/experimental-hydrate.ts |  9 +--
 5 files changed, 115 insertions(+), 56 deletions(-)
 create mode 100644 .changeset/soft-turtles-protect.md

diff --git a/.changeset/soft-turtles-protect.md b/.changeset/soft-turtles-protect.md
new file mode 100644
index 0000000000..7a1d296446
--- /dev/null
+++ b/.changeset/soft-turtles-protect.md
@@ -0,0 +1,6 @@
+---
+'@lit-labs/ssr': patch
+'lit-html': patch
+---
+
+Improved how nodes with attribute/property/event/element bindings are rendered in SSR, to avoid adding comments inside of "raw text elements" like `<textarea>`. Fixes #3663.
diff --git a/packages/labs/ssr/src/lib/render-value.ts b/packages/labs/ssr/src/lib/render-value.ts
index 1e963c00dd..ff70a801fc 100644
--- a/packages/labs/ssr/src/lib/render-value.ts
+++ b/packages/labs/ssr/src/lib/render-value.ts
@@ -222,14 +222,18 @@ type Op =
  * ```
  *
  * - `text`
- *   - Emit run of static text: `<div><span>Hello</span><span`
+ *   - Emit run of static text: `<div><span>Hello</span>`
+ * - `possible-node-marker`
+ *   - Emit `<!--lit-node n-->` marker since there are attribute parts
+ * - `text`
+ *   - Emit run of static text: `<span`
  * - `attribute-part`
  *   - Emit an AttributePart's value, e.g. ` class="bold"`
  * - `text`
  *   - Emit run of static text: `>`
  * - `child-part`
- *   - Emit the ChildPart's value, in this case a TemplateResult, thus we recurse
- *     into that template's opcodes
+ *   - Emit the ChildPart's value, in this case a TemplateResult, thus we
+ *     recurse into that template's opcodes
  * - `text`
  *   - Emit run of static text: `/span></div>`
  *
@@ -239,6 +243,9 @@ type Op =
  * html`<x-foo staticAttr dynamicAttr=${value}><div>child</div>...</x-foo>`
  * ```
  *
+ * - `possible-node-marker`
+ *   - Emit `<!--lit-node n-->` marker since there are attribute parts and we
+ *      may emit the `defer-hydration` attribute on the node that follows
  * - `text`
  *   - Emit open tag `<x-foo`
  * - `custom-element-open`
@@ -254,9 +261,6 @@ type Op =
  *   - Emit `renderer.renderAttributes()`
  * - `text`
  *   - Emit end of of open tag `>`
- * - `possible-node-marker`
- *   - Emit `<!--lit-node n-->` marker if there were attribute parts or
- *      we needed to emit the `defer-hydration` attribute
  * - `custom-element-shadow`
  *   - Emit `renderer.renderShadow()` (emits `<template shadowroot>` +
  *     recurses to emit `render()`)
@@ -362,22 +366,15 @@ const getTemplateOpcodes = (result: TemplateResult) => {
         }
         nodeIndex++;
       } else if (isElementNode(node)) {
-        // Whether to flush the start tag. This is necessary if we're changing
-        // any of the attributes in the tag, so it's true for custom-elements
-        // which might reflect their own state, or any element with a binding.
-        let writeTag = false;
         let boundAttributesCount = 0;
 
         const tagName = node.tagName;
-        let ctor;
 
         if (tagName.indexOf('-') !== -1) {
           // Looking up the constructor here means that custom elements must be
           // registered before rendering the first template that contains them.
-          ctor = customElements.get(tagName);
+          const ctor = customElements.get(tagName);
           if (ctor !== undefined) {
-            // Write the start tag
-            writeTag = true;
             // Mark that this is a custom element
             node.isDefinedCustomElement = true;
             ops.push({
@@ -393,12 +390,35 @@ const getTemplateOpcodes = (result: TemplateResult) => {
           }
         }
         if (node.attrs.length > 0) {
+          const attrInfo = [] as Array<
+            [boolean, boolean, typeof node.attrs[0]]
+          >;
           for (const attr of node.attrs) {
             const isAttrBinding = attr.name.endsWith(boundAttributeSuffix);
             const isElementBinding = attr.name.startsWith(marker);
             if (isAttrBinding || isElementBinding) {
-              writeTag = true;
               boundAttributesCount += 1;
+            }
+            attrInfo.push([isAttrBinding, isElementBinding, attr]);
+          }
+          if (boundAttributesCount > 0 || node.isDefinedCustomElement) {
+            // We (may) need to emit a `<!-- lit-node -->` comment marker to
+            // indicate the following node needs to be identified during
+            // hydration when it has bindings or if it is a custom element (and
+            // thus may need its `defer-hydration` to be removed, depending on
+            // the `deferHydration` setting). The marker is emitted as a
+            // previous sibling before the node in question, to avoid issues
+            // with void elements (which do not have children) and raw text
+            // elements (whose children are intepreted as text).
+            flushTo(node.sourceCodeLocation!.startTag!.startOffset);
+            ops.push({
+              type: 'possible-node-marker',
+              boundAttributesCount,
+              nodeIndex,
+            });
+          }
+          for (const [isAttrBinding, isElementBinding, attr] of attrInfo) {
+            if (isAttrBinding || isElementBinding) {
               // Note that although we emit a lit-node comment marker for any
               // nodes with bindings, we don't account for it in the nodeIndex because
               // that will not be injected into the client template
@@ -430,7 +450,7 @@ const getTemplateOpcodes = (result: TemplateResult) => {
                       : AttributePart,
                   strings,
                   tagName: tagName.toUpperCase(),
-                  useCustomElementInstance: ctor !== undefined,
+                  useCustomElementInstance: node.isDefinedCustomElement,
                 });
               } else {
                 ops.push({
@@ -453,25 +473,16 @@ const getTemplateOpcodes = (result: TemplateResult) => {
           }
         }
 
-        if (writeTag) {
-          if (node.isDefinedCustomElement) {
-            flushTo(node.sourceCodeLocation!.startTag!.endOffset - 1);
-            ops.push({
-              type: 'custom-element-attributes',
-            });
-            flush('>');
-            skipTo(node.sourceCodeLocation!.startTag!.endOffset);
-          } else {
-            flushTo(node.sourceCodeLocation!.startTag!.endOffset);
-          }
+        if (node.isDefinedCustomElement) {
+          // For custom elements, add an opcode to write out attributes,
+          // close the tag, and then add an opcode to write the shadow
+          // root
+          flushTo(node.sourceCodeLocation!.startTag!.endOffset - 1);
           ops.push({
-            type: 'possible-node-marker',
-            boundAttributesCount,
-            nodeIndex,
+            type: 'custom-element-attributes',
           });
-        }
-
-        if (ctor !== undefined) {
+          flush('>');
+          skipTo(node.sourceCodeLocation!.startTag!.endOffset);
           ops.push({
             type: 'custom-element-shadow',
           });
diff --git a/packages/labs/ssr/src/test/integration/tests/basic.ts b/packages/labs/ssr/src/test/integration/tests/basic.ts
index 34b7364f62..bdd83e3d45 100644
--- a/packages/labs/ssr/src/test/integration/tests/basic.ts
+++ b/packages/labs/ssr/src/test/integration/tests/basic.ts
@@ -1628,6 +1628,49 @@ export const tests: {[name: string]: SSRTest} = {
     stableSelectors: ['input'],
   },
 
+  'AttributePart on raw text element in shadow root': {
+    // Regression test for https://github.com/lit/lit/issues/3663.
+    //
+    // Confirms that attribute bindings to raw text elements now
+    // work as expected.
+    registerElements() {
+      class RawElementHost extends LitElement {
+        @property()
+        text = 'hello';
+
+        override render() {
+          return html`<textarea .value=${this.text}></textarea>`;
+        }
+      }
+      customElements.define('raw-element-host', RawElementHost);
+    },
+    render() {
+      return html`<raw-element-host></raw-element-host>`;
+    },
+    expectations: [
+      {
+        args: [],
+        html: '<raw-element-host></raw-element-host>',
+        async check(assert: Chai.Assert, dom: HTMLElement) {
+          const host = dom.querySelector('raw-element-host') as LitElement & {
+            text: string;
+          };
+          assert.instanceOf(host, LitElement);
+          assert.equal(host.text, 'hello');
+
+          await host.updateComplete;
+          const textarea = host.shadowRoot?.querySelector('textarea');
+          assert.equal(textarea?.value, 'hello');
+
+          host.text = 'goodbye';
+          await host.updateComplete;
+          assert.equal(textarea?.value, 'goodbye');
+        },
+      },
+    ],
+    stableSelectors: ['textarea'],
+  },
+
   /******************************************************
    * PropertyPart tests
    ******************************************************/
diff --git a/packages/labs/ssr/src/test/lib/render-lit_test.ts b/packages/labs/ssr/src/test/lib/render-lit_test.ts
index 8e8e8322d0..b87d069687 100644
--- a/packages/labs/ssr/src/test/lib/render-lit_test.ts
+++ b/packages/labs/ssr/src/test/lib/render-lit_test.ts
@@ -118,7 +118,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(templateWithAttributeExpression('foo'));
     assert.is(
       result,
-      `<!--lit-part FAR9hgjJqTI=--><div class="foo"><!--lit-node 0--></div><!--/lit-part-->`
+      `<!--lit-part FAR9hgjJqTI=--><!--lit-node 0--><div class="foo"></div><!--/lit-part-->`
     );
   });
 
@@ -127,7 +127,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(inputTemplateWithAttributeExpression('foo'));
     assert.is(
       result,
-      `<!--lit-part AYwG7rAvcnw=--><input x="foo"><!--lit-node 0--><!--/lit-part-->`
+      `<!--lit-part AYwG7rAvcnw=--><!--lit-node 0--><input x="foo"><!--/lit-part-->`
     );
   });
 
@@ -144,7 +144,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     );
     assert.is(
       result,
-      `<!--lit-part BIugdiAuV4I=--><input x="foo"><!--lit-node 0--><p>hi</p></input><!--/lit-part-->`
+      `<!--lit-part BIugdiAuV4I=--><!--lit-node 0--><input x="foo"><p>hi</p></input><!--/lit-part-->`
     );
   });
 
@@ -156,7 +156,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // Has marker attribute for number of bound attributes.
     assert.is(
       result,
-      `<!--lit-part FQlA2/EioQk=--><div x="foo" y="bar" z="not-dynamic"><!--lit-node 0--></div><!--/lit-part-->`
+      `<!--lit-part FQlA2/EioQk=--><!--lit-node 0--><div x="foo" y="bar" z="not-dynamic"></div><!--/lit-part-->`
     );
   });
 
@@ -167,7 +167,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     );
     assert.is(
       result,
-      `<!--lit-part D+PQMst9obo=--><div test="a foo b bar c"><!--lit-node 0--></div><!--/lit-part-->`
+      `<!--lit-part D+PQMst9obo=--><!--lit-node 0--><div test="a foo b bar c"></div><!--/lit-part-->`
     );
   });
 
@@ -178,7 +178,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(inputTemplateWithValueProperty('foo'));
     assert.is(
       result,
-      `<!--lit-part AxWziS+Adpk=--><input value="foo"><!--lit-node 0--><!--/lit-part-->`
+      `<!--lit-part AxWziS+Adpk=--><!--lit-node 0--><input value="foo"><!--/lit-part-->`
     );
   });
 
@@ -187,7 +187,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(elementTemplateWithClassNameProperty('foo'));
     assert.is(
       result,
-      `<!--lit-part I7NxrdZ/Zxo=--><div class="foo"><!--lit-node 0--></div><!--/lit-part-->`
+      `<!--lit-part I7NxrdZ/Zxo=--><!--lit-node 0--><div class="foo"></div><!--/lit-part-->`
     );
   });
 
@@ -196,7 +196,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(elementTemplateWithClassnameProperty('foo'));
     assert.is(
       result,
-      `<!--lit-part I7NxrbZzZGA=--><div ><!--lit-node 0--></div><!--/lit-part-->`
+      `<!--lit-part I7NxrbZzZGA=--><!--lit-node 0--><div ></div><!--/lit-part-->`
     );
   });
 
@@ -205,7 +205,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(elementTemplateWithIDProperty('foo'));
     assert.is(
       result,
-      `<!--lit-part IgnmhhM3LsA=--><div id="foo"><!--lit-node 0--></div><!--/lit-part-->`
+      `<!--lit-part IgnmhhM3LsA=--><!--lit-node 0--><div id="foo"></div><!--/lit-part-->`
     );
   });
 
@@ -276,7 +276,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // TODO: we'd like to remove the extra space in the start tag
     assert.is(
       result,
-      `<!--lit-part v2CxGIW+qHI=--><test-property ><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
+      `<!--lit-part v2CxGIW+qHI=--><!--lit-node 0--><test-property ><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
     );
   });
 
@@ -286,7 +286,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // TODO: we'd like to remove the extra space in the start tag
     assert.is(
       result,
-      `<!--lit-part ZI1U/5CYP1o=--><test-property  foo="bar"><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
+      `<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property  foo="bar"><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->bar<!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
     );
   });
 
@@ -296,7 +296,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // TODO: we'd like to remove the extra space in the start tag
     assert.is(
       result,
-      `<!--lit-part ZI1U/5CYP1o=--><test-property  foo><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
+      `<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property  foo><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
     );
   });
 
@@ -306,7 +306,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // TODO: we'd like to remove the extra space in the start tag
     assert.is(
       result,
-      `<!--lit-part ZI1U/5CYP1o=--><test-property  foo><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
+      `<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property  foo><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
     );
   });
 
@@ -316,7 +316,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // TODO: we'd like to remove the extra space in the start tag
     assert.is(
       result,
-      `<!--lit-part ZI1U/5CYP1o=--><test-property  foo><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
+      `<!--lit-part ZI1U/5CYP1o=--><!--lit-node 0--><test-property  foo><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part--><!--/lit-part--></main><!--/lit-part--></template></test-property><!--/lit-part-->`
     );
   });
 
@@ -326,7 +326,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // TODO: we'd like to remove the extra space in the start tag
     assert.is(
       result,
-      `<!--lit-part 7z41MJchKXM=--><test-reflected-properties   bar baz="default reflected string" reflect-foo="badazzled"><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part--><!--/lit-part--></template></test-reflected-properties><!--/lit-part-->`
+      `<!--lit-part 7z41MJchKXM=--><!--lit-node 0--><test-reflected-properties   bar baz="default reflected string" reflect-foo="badazzled"><template shadowroot="open" shadowrootmode="open"><!--lit-part--><!--/lit-part--></template></test-reflected-properties><!--/lit-part-->`
     );
   });
 
@@ -345,7 +345,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     // TODO: we'd like to remove the extra space in the start tag
     assert.is(
       result,
-      `<!--lit-part Q0bbGrx71ic=--><test-will-update  ><!--lit-node 0--><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->Foo Bar<!--/lit-part--></main><!--/lit-part--></template></test-will-update><!--/lit-part-->`
+      `<!--lit-part Q0bbGrx71ic=--><!--lit-node 0--><test-will-update  ><template shadowroot="open" shadowrootmode="open"><!--lit-part UNbWrd8S5FY=--><main><!--lit-part-->Foo Bar<!--/lit-part--></main><!--/lit-part--></template></test-will-update><!--/lit-part-->`
     );
   });
 
@@ -442,7 +442,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(classMapDirective);
     assert.is(
       result,
-      '<!--lit-part PkF/hiJU4II=--><div class=" a c "><!--lit-node 0--></div><!--/lit-part-->'
+      '<!--lit-part PkF/hiJU4II=--><!--lit-node 0--><div class=" a c "></div><!--/lit-part-->'
     );
   });
 
@@ -451,7 +451,7 @@ for (const global of [emptyVmGlobal, shimmedVmGlobal]) {
     const result = await render(classMapDirectiveMultiBinding);
     assert.is(
       result,
-      '<!--lit-part pNgepkKFbd0=--><div class="z hi a c"><!--lit-node 0--></div><!--/lit-part-->'
+      '<!--lit-part pNgepkKFbd0=--><!--lit-node 0--><div class="z hi a c"></div><!--/lit-part-->'
     );
   });
 
diff --git a/packages/lit-html/src/experimental-hydrate.ts b/packages/lit-html/src/experimental-hydrate.ts
index 4df91a1a34..a8f3478d01 100644
--- a/packages/lit-html/src/experimental-hydrate.ts
+++ b/packages/lit-html/src/experimental-hydrate.ts
@@ -348,11 +348,10 @@ const createAttributeParts = (
   const match = /lit-node (\d+)/.exec(comment.data)!;
   const nodeIndex = parseInt(match[1]);
 
-  // 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;
+  // Node markers are added as a previous sibling to identify elements
+  // with attribute/property/element/event bindings or custom elements
+  // whose `defer-hydration` attribute needs to be removed
+  const node = comment.nextElementSibling;
   if (node === null) {
     throw new Error('could not find node for attribute parts');
   }