From a8cb91132c29821b888bb3ed4bd9c6b5a9f3bbed Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 25 May 2022 10:39:42 -0700 Subject: [PATCH 1/5] Add SSR tests for attribute bindings on void elements --- .../ssr/src/test/integration/tests/basic.ts | 31 +++++++++++++++++++ .../labs/ssr/src/test/lib/render-lit_test.ts | 27 +++++++++++++++- .../src/test/test-files/render-test-module.ts | 6 ++++ 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/packages/labs/ssr/src/test/integration/tests/basic.ts b/packages/labs/ssr/src/test/integration/tests/basic.ts index 5f7d4c6d5e..a2a6650521 100644 --- a/packages/labs/ssr/src/test/integration/tests/basic.ts +++ b/packages/labs/ssr/src/test/integration/tests/basic.ts @@ -1580,6 +1580,37 @@ export const tests: {[name: string]: SSRTest} = { stableSelectors: ['input'], }, + 'AttributePart on void element in shadow root': { + // Regression test for https://github.com/lit/lit/issues/2946. + // + // Confirms that we do not crash when hydrating a shadow root containing an + // immediate child that is a void element with an attribute binding. This is + // an edge case because when the HTML parser encounters a void element, any + // children it has, including our comments, become + // siblings instead of children. + registerElements() { + class VoidElementHost extends LitElement { + @property() + maxLen = 64; + + override render() { + return html``; + } + } + customElements.define('void-element-host', VoidElementHost); + }, + render() { + return html``; + }, + expectations: [ + { + args: [], + html: '', + }, + ], + stableSelectors: ['input'], + }, + /****************************************************** * 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 1b3a6690c4..ee640700ba 100644 --- a/packages/labs/ssr/src/test/lib/render-lit_test.ts +++ b/packages/labs/ssr/src/test/lib/render-lit_test.ts @@ -111,13 +111,38 @@ test('text expression with null value', async () => { test('attribute expression with string value', async () => { const {render, templateWithAttributeExpression} = await setup(); const result = await render(templateWithAttributeExpression('foo')); - // TODO: test for the marker comment for attribute binding assert.is( result, `
` ); }); +test('input element with attribute expression with string value', async () => { + const {render, inputTemplateWithAttributeExpression} = await setup(); + const result = await render(inputTemplateWithAttributeExpression('foo')); + assert.is( + result, + `` + ); +}); + +test('input element with attribute expression with string value and child element', async () => { + // Void elements like input will have their children hoisted to become + // siblings by the HTML parser. In this case, we rely on the fact that any + // comments we create are prepended instead of appended, + // so that they will be hoisted as the next sibling, so that we can use + // .previousElementSibling to find the effective parent. + const {render, inputTemplateWithAttributeExpressionAndChildElement} = + await setup(); + const result = await render( + inputTemplateWithAttributeExpressionAndChildElement('foo') + ); + assert.is( + result, + `

hi

` + ); +}); + test('multiple attribute expressions with string value', async () => { const {render, templateWithMultipleAttributeExpressions} = await setup(); const result = await render( diff --git a/packages/labs/ssr/src/test/test-files/render-test-module.ts b/packages/labs/ssr/src/test/test-files/render-test-module.ts index 61bf8e82f2..7d0fd3f107 100644 --- a/packages/labs/ssr/src/test/test-files/render-test-module.ts +++ b/packages/labs/ssr/src/test/test-files/render-test-module.ts @@ -35,6 +35,12 @@ export const templateWithMultiBindingAttributeExpression = ( x: string, y: string ) => html`
`; +// prettier-ignore +export const inputTemplateWithAttributeExpression = (x: string) => +html``; +// prettier-ignore +export const inputTemplateWithAttributeExpressionAndChildElement = (x: string) => + html`

hi

`; /* Reflected Property Expressions */ From 202f3dbca075df38ea5cc4e2250d6cae95a75681 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 25 May 2022 10:40:14 -0700 Subject: [PATCH 2/5] Fix defer-hydration removal on void element attribute bindings --- packages/lit-html/src/experimental-hydrate.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/lit-html/src/experimental-hydrate.ts b/packages/lit-html/src/experimental-hydrate.ts index 7b7ffaec67..1e298f8f40 100644 --- a/packages/lit-html/src/experimental-hydrate.ts +++ b/packages/lit-html/src/experimental-hydrate.ts @@ -157,11 +157,6 @@ export const hydrate = ( // Create and hydrate attribute parts into the current ChildPart on the // stack createAttributeParts(marker, stack, options); - // Remove `defer-hydration` attribute, if any - const parent = marker.parentElement!; - if (parent.hasAttribute('defer-hydration')) { - parent.removeAttribute('defer-hydration'); - } } else if (markerText.startsWith('/lit-part')) { // Close the current ChildPart, and pop the previous one off the stack if (stack.length === 1 && currentChildPart !== rootPart) { @@ -337,7 +332,12 @@ const createAttributeParts = ( // 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.previousSibling ?? comment.parentElement; + const node = comment.previousElementSibling ?? comment.parentElement; + if (node === null) { + throw new Error('could not find node for attribute parts'); + } + // Remove `defer-hydration` attribute, if any + node.removeAttribute('defer-hydration'); const state = stack[stack.length - 1]; if (state.type === 'template-instance') { From 014699eff380bf7cb03aab5166b34a7bdd1611a3 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 25 May 2022 10:41:21 -0700 Subject: [PATCH 3/5] Add changeset --- .changeset/modern-parrots-talk.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/modern-parrots-talk.md diff --git a/.changeset/modern-parrots-talk.md b/.changeset/modern-parrots-talk.md new file mode 100644 index 0000000000..5153a8c39e --- /dev/null +++ b/.changeset/modern-parrots-talk.md @@ -0,0 +1,5 @@ +--- +'lit-html': patch +--- + +Fix SSR hydration bug relating to and other void elements having attribute bindings. From e214050f36a1b970b4e254d5784e0871c828d4a9 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 25 May 2022 11:32:58 -0700 Subject: [PATCH 4/5] Remove cast that we no longer need --- packages/lit-html/src/experimental-hydrate.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/lit-html/src/experimental-hydrate.ts b/packages/lit-html/src/experimental-hydrate.ts index 1e298f8f40..11f6cf44ae 100644 --- a/packages/lit-html/src/experimental-hydrate.ts +++ b/packages/lit-html/src/experimental-hydrate.ts @@ -391,11 +391,7 @@ const createAttributeParts = ( instance._parts.push(instancePart); } else { // templatePart.type === PartType.ELEMENT - const instancePart = new ElementPart( - node as HTMLElement, - state.instance, - options - ); + const instancePart = new ElementPart(node, state.instance, options); resolveDirective( instancePart, state.result.values[state.instancePartIndex++] From a3a129435dcf97e0fe704e2d883393c2d40b6e49 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 25 May 2022 12:34:46 -0700 Subject: [PATCH 5/5] Add test that attribute hydration worked --- .../ssr/src/test/integration/tests/basic.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/labs/ssr/src/test/integration/tests/basic.ts b/packages/labs/ssr/src/test/integration/tests/basic.ts index a2a6650521..ae758bdc22 100644 --- a/packages/labs/ssr/src/test/integration/tests/basic.ts +++ b/packages/labs/ssr/src/test/integration/tests/basic.ts @@ -1606,6 +1606,23 @@ export const tests: {[name: string]: SSRTest} = { { args: [], html: '', + async check(assert: Chai.Assert, dom: HTMLElement) { + const host = dom.querySelector('void-element-host') as LitElement & { + maxLen: number; + }; + assert.instanceOf(host, LitElement); + assert.equal(host.maxLen, 64); + + await host.updateComplete; + // eslint-disable-next-line @typescript-eslint/no-non-null-asserted-optional-chain + const input = host.shadowRoot?.querySelector('input')!; + assert.instanceOf(input, HTMLElement); + assert.equal(input.getAttribute('max'), '64'); + + host.maxLen++; + await host.updateComplete; + assert.equal(input.getAttribute('max'), '65'); + }, }, ], stableSelectors: ['input'],