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

[ssr] Fix hydration bug with attribute bindings on void elements #2952

Merged
merged 5 commits into from May 25, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/modern-parrots-talk.md
@@ -0,0 +1,5 @@
---
'lit-html': patch
---

Fix SSR hydration bug relating to <input> and other void elements having attribute bindings.
31 changes: 31 additions & 0 deletions packages/labs/ssr/src/test/integration/tests/basic.ts
Expand Up @@ -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 <!--lit-node 0--> comments, become
// siblings instead of children.
registerElements() {
class VoidElementHost extends LitElement {
@property()
maxLen = 64;

override render() {
return html`<input max=${this.maxLen} />`;
}
}
customElements.define('void-element-host', VoidElementHost);
},
render() {
return html`<void-element-host></void-element-host>`;
},
expectations: [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No action necessary now, but it might be nice to have some check that this actually hydrated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added confirmation that we actually hydrated, by making sure if we change the property, the attribute updates.

{
args: [],
html: '<void-element-host></void-element-host>',
},
],
stableSelectors: ['input'],
},

/******************************************************
* PropertyPart tests
******************************************************/
Expand Down
27 changes: 26 additions & 1 deletion packages/labs/ssr/src/test/lib/render-lit_test.ts
Expand Up @@ -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,
`<!--lit-part FAR9hgjJqTI=--><div class="foo"><!--lit-node 0--></div><!--/lit-part-->`
);
});

test('input element with attribute expression with string value', async () => {
const {render, inputTemplateWithAttributeExpression} = await setup();
const result = await render(inputTemplateWithAttributeExpression('foo'));
assert.is(
result,
`<!--lit-part AYwG7rAvcnw=--><input x="foo"><!--lit-node 0--><!--/lit-part-->`
);
});

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
// <!--lit-node 0--> 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,
`<!--lit-part BIugdiAuV4I=--><input x="foo"><!--lit-node 0--><p>hi</p></input><!--/lit-part-->`
);
});

test('multiple attribute expressions with string value', async () => {
const {render, templateWithMultipleAttributeExpressions} = await setup();
const result = await render(
Expand Down
6 changes: 6 additions & 0 deletions packages/labs/ssr/src/test/test-files/render-test-module.ts
Expand Up @@ -35,6 +35,12 @@ export const templateWithMultiBindingAttributeExpression = (
x: string,
y: string
) => html`<div test="a ${x} b ${y} c"></div>`;
// prettier-ignore
export const inputTemplateWithAttributeExpression = (x: string) =>
html`<input x=${x}>`;
// prettier-ignore
export const inputTemplateWithAttributeExpressionAndChildElement = (x: string) =>
html`<input x=${x}><p>hi</p></input>`;

/* Reflected Property Expressions */

Expand Down
12 changes: 6 additions & 6 deletions packages/lit-html/src/experimental-hydrate.ts
Expand Up @@ -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) {
Expand Down Expand Up @@ -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') {
Expand Down