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

aomarks · 2022-05-25T18:18:11Z

Background

In SSR, when an element has an attribute binding, we insert a  comment to tell hydrate() about the attribute binding.

However, if that element was a void element, such as <input> or anything from this list, then when the HTML parser encounters it, all of the children it has will be hoisted up as siblings.

For example:

render() {
  return html`<input max=${this.maxLen}>`;
}

Is SSR'd as:

<input max="42"><!--lit-node 0--></input>

But parsed by the browser as:

<input max="42" />
<!--lit-node 0-->

This means when hydrate() encounters a case like this, we need to check previousSibling instead of parentElement, to find the element to receive the attribute binding.

Bug

We already accounted for void elements where we actually create the attribute parts:

lit/packages/lit-html/src/experimental-hydrate.ts

Line 340 in ac35699

const node = comment.previousSibling ?? comment.parentElement;

But we did not account for it where we remove the defer-hydration attribute:

lit/packages/lit-html/src/experimental-hydrate.ts

Line 161 in ac35699

const parent = marker.parentElement!;

This actually only crashed hydrate() if the void element was an immediate child of a shadow root, because in every other case, element.parentElement would return something, even if it wasn't the correct node.

Fix

We now also account for void elements when we remove the defer-hydration attribute in the same way that we already did for creating attribute parts.

Note as part of this I switched from previousSibling to previousElementSibling, so that we can assume removeAttribute is defined. Seems like this should be safe?

Hopefully fixes #2946

cc @daKmoR

Alternative idea

I also thought about an alternative way to handle void elements generally, which is to detect them up-front during SSR rendering, since they are a small fixed set of tag names, so that we get an explicit signal about whether we need to check the parent or the sibling during hydration, instead of doing the previousSibling -> parentElement fallback. See 66b702b for how that would look. This could potentially have slightly better performance during hydration, since it replaces a DOM call with a string check. Not sure it's worth it though, and I think it would be a breaking change because it changes the rendering scheme.

changeset-bot · 2022-05-25T18:18:14Z

🦋 Changeset detected

Latest commit: a3a1294

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-05-25T18:22:00Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -3% - +5% (-0.91ms - +1.60ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -3% - +1% (-3.08ms - +1.22ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -2% - +3% (-0.94ms - +1.23ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -4% - +7% (-0.53ms - +0.98ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +1% (-2.20ms - +1.04ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -4% - +9% (-2.76ms - +6.52ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -3% - +4% (-31.05ms - +39.79ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -4% - +3% (-4.18ms - +3.08ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +11% (-12.27ms - +45.12ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +3% (-1.83ms - +4.36ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -11% - -0% (-227.58ms - +0.66ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -1% - +2% (-12.27ms - +22.41ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +2% (-36.35ms - +20.90ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 101.0.4951.64
Sample size: 50
Built by: Benchmarks #3229
Commit: 0cd76eb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	105.15ms - 107.94ms	-	unsure 🔍 -3% - +1% -3.08ms - +1.22ms	faster ✔ 21% - 24% 28.94ms - 33.48ms
tip-of-tree tip-of-tree	105.84ms - 109.10ms	unsure 🔍 -1% - +3% -1.22ms - +3.08ms	-	faster ✔ 20% - 24% 27.86ms - 32.71ms
previous-release previous-release	135.96ms - 139.55ms	slower ❌ 27% - 32% 28.94ms - 33.48ms	slower ❌ 26% - 31% 27.86ms - 32.71ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1050.22ms - 1107.83ms	-	unsure 🔍 -3% - +4% -31.05ms - +39.79ms	faster ✔ 31% - 35% 507.45ms - 576.73ms
tip-of-tree tip-of-tree	1054.04ms - 1095.26ms	unsure 🔍 -4% - +3% -39.79ms - +31.05ms	-	faster ✔ 32% - 35% 518.27ms - 574.65ms
previous-release previous-release	1601.87ms - 1640.35ms	slower ❌ 46% - 55% 507.45ms - 576.73ms	slower ❌ 47% - 54% 518.27ms - 574.65ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1046.49ms - 1070.44ms	-	unsure 🔍 -1% - +2% -12.27ms - +22.41ms	faster ✔ 7% - 10% 79.53ms - 115.49ms
tip-of-tree tip-of-tree	1040.86ms - 1065.94ms	unsure 🔍 -2% - +1% -22.41ms - +12.27ms	-	faster ✔ 7% - 10% 84.22ms - 120.94ms
previous-release previous-release	1142.57ms - 1169.38ms	slower ❌ 7% - 11% 79.53ms - 115.49ms	slower ❌ 8% - 12% 84.22ms - 120.94ms	-

lit-html-kitchen-sink

Browser: chrome-headless 101.0.4951.64
Sample size: 30
Built by: Benchmarks #3229
Commit: 0cd76eb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	43.37ms - 44.92ms	-	unsure 🔍 -2% - +3% -0.94ms - +1.23ms	faster ✔ 9% - 17% 4.55ms - 8.92ms
tip-of-tree tip-of-tree	43.24ms - 44.75ms	unsure 🔍 -3% - +2% -1.23ms - +0.94ms	-	faster ✔ 10% - 17% 4.70ms - 9.06ms
previous-release previous-release	48.84ms - 52.92ms	slower ❌ 10% - 20% 4.55ms - 8.92ms	slower ❌ 11% - 21% 4.70ms - 9.06ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	109.00ms - 114.31ms	-	unsure 🔍 -4% - +3% -4.18ms - +3.08ms	faster ✔ 1% - 7% 1.05ms - 8.33ms
tip-of-tree tip-of-tree	109.73ms - 114.68ms	unsure 🔍 -3% - +4% -3.08ms - +4.18ms	-	faster ✔ 1% - 7% 0.63ms - 7.65ms
previous-release previous-release	113.85ms - 118.84ms	slower ❌ 1% - 8% 1.05ms - 8.33ms	slower ❌ 1% - 7% 0.63ms - 7.65ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.44ms - 35.33ms	-	unsure 🔍 -3% - +5% -0.91ms - +1.60ms	faster ✔ 4% - 11% 1.52ms - 4.08ms
tip-of-tree tip-of-tree	33.21ms - 34.87ms	unsure 🔍 -5% - +3% -1.60ms - +0.91ms	-	faster ✔ 5% - 12% 1.96ms - 4.34ms
previous-release previous-release	36.33ms - 38.05ms	slower ❌ 4% - 12% 1.52ms - 4.08ms	slower ❌ 6% - 13% 1.96ms - 4.34ms	-

lit-html-repeat

Browser: chrome-headless 101.0.4951.64
Sample size: 30
Built by: Benchmarks #3229
Commit: 0cd76eb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	14.74ms - 15.88ms	-	unsure 🔍 -4% - +7% -0.53ms - +0.98ms	faster ✔ 3% - 11% 0.48ms - 1.87ms
tip-of-tree tip-of-tree	14.59ms - 15.58ms	unsure 🔍 -6% - +3% -0.98ms - +0.53ms	-	faster ✔ 5% - 12% 0.76ms - 2.03ms
previous-release previous-release	16.09ms - 16.87ms	slower ❌ 3% - 12% 0.48ms - 1.87ms	slower ❌ 5% - 14% 0.76ms - 2.03ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	424.46ms - 464.48ms	-	unsure 🔍 -3% - +11% -12.27ms - +45.12ms	faster ✔ 15% - 25% 82.56ms - 139.70ms
tip-of-tree tip-of-tree	407.48ms - 448.61ms	unsure 🔍 -10% - +3% -45.12ms - +12.27ms	-	faster ✔ 18% - 28% 98.60ms - 156.52ms
previous-release previous-release	535.21ms - 575.99ms	slower ❌ 18% - 32% 82.56ms - 139.70ms	slower ❌ 22% - 38% 98.60ms - 156.52ms	-

lit-html-template-heavy

Browser: chrome-headless 101.0.4951.64
Sample size: 50
Built by: Benchmarks #3229
Commit: 0cd76eb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	72.00ms - 74.06ms	-	unsure 🔍 -3% - +1% -2.20ms - +1.04ms	faster ✔ 13% - 17% 11.47ms - 14.95ms
tip-of-tree tip-of-tree	72.37ms - 74.86ms	unsure 🔍 -1% - +3% -1.04ms - +2.20ms	-	faster ✔ 13% - 17% 10.76ms - 14.50ms
previous-release previous-release	84.85ms - 87.63ms	slower ❌ 16% - 21% 11.47ms - 14.95ms	slower ❌ 14% - 20% 10.76ms - 14.50ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	166.88ms - 171.56ms	-	unsure 🔍 -1% - +3% -1.83ms - +4.36ms	faster ✔ 10% - 13% 18.67ms - 24.87ms
tip-of-tree tip-of-tree	165.93ms - 169.98ms	unsure 🔍 -3% - +1% -4.36ms - +1.83ms	-	faster ✔ 11% - 13% 20.16ms - 25.91ms
previous-release previous-release	188.95ms - 193.03ms	slower ❌ 11% - 15% 18.67ms - 24.87ms	slower ❌ 12% - 16% 20.16ms - 25.91ms	-

reactive-element-list

Browser: chrome-headless 101.0.4951.64
Sample size: 50
Built by: Benchmarks #3229
Commit: 0cd76eb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	74.65ms - 82.38ms	-	unsure 🔍 -4% - +9% -2.76ms - +6.52ms	unsure 🔍 -2% - +9% -1.26ms - +6.86ms
tip-of-tree tip-of-tree	74.06ms - 79.20ms	unsure 🔍 -8% - +3% -6.52ms - +2.76ms	-	unsure 🔍 -3% - +5% -1.94ms - +3.78ms
previous-release previous-release	74.46ms - 76.97ms	unsure 🔍 -9% - +1% -6.86ms - +1.26ms	unsure 🔍 -5% - +2% -3.78ms - +1.94ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1761.49ms - 1951.48ms	-	unsure 🔍 -11% - -0% -227.58ms - +0.66ms	unsure 🔍 -9% - +4% -177.92ms - +68.40ms
tip-of-tree tip-of-tree	1906.69ms - 2033.19ms	unsure 🔍 -0% - +13% -0.66ms - +227.58ms	-	unsure 🔍 -2% - +8% -42.03ms - +159.42ms
previous-release previous-release	1832.85ms - 1989.63ms	unsure 🔍 -4% - +10% -68.40ms - +177.92ms	unsure 🔍 -8% - +2% -159.42ms - +42.03ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1182.91ms - 1224.44ms	-	unsure 🔍 -3% - +2% -36.35ms - +20.90ms	unsure 🔍 -4% - +1% -42.93ms - +16.35ms
tip-of-tree tip-of-tree	1191.70ms - 1231.10ms	unsure 🔍 -2% - +3% -20.90ms - +36.35ms	-	unsure 🔍 -3% - +2% -34.47ms - +23.34ms
previous-release previous-release	1195.81ms - 1238.12ms	unsure 🔍 -1% - +4% -16.35ms - +42.93ms	unsure 🔍 -2% - +3% -23.34ms - +34.47ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2022-05-25T18:31:56Z

packages/labs/ssr/src/test/integration/tests/basic.ts

+    render() {
+      return html`<void-element-host></void-element-host>`;
+    },
+    expectations: [


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

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

augustjk

Nice!
The alternate approach looks interesting but feels not worth it compared to this solution. Checking against a set of void elements would be useful if we're trying not to rely on the parser's hoisting behavior (is this stable behavior?) and place the marker as sibling ourselves to make it stable, but that also seems unnecessarily extra and breaking for the moment.

Found in #2954 - Delete `@lit-labs/gen-wrapper-angular` changeset, because it's not released. - Also bump `lit`, should have been included in #2952.

aomarks added 3 commits May 25, 2022 10:39

Add SSR tests for attribute bindings on void elements

a8cb911

Fix defer-hydration removal on void element attribute bindings

202f3db

Add changeset

014699e

aomarks requested review from justinfagnani, kevinpschaaf and augustjk May 25, 2022 18:18

justinfagnani approved these changes May 25, 2022

View reviewed changes

Remove cast that we no longer need

e214050

augustjk approved these changes May 25, 2022

View reviewed changes

Add test that attribute hydration worked

a3a1294

aomarks merged commit a78cc3b into main May 25, 2022

aomarks deleted the defer-hydration-attr branch May 25, 2022 19:54

lit-robot mentioned this pull request May 25, 2022

Version Packages #2954

Merged

aomarks mentioned this pull request May 25, 2022

Fix a couple of release issues #2956

Merged

aomarks added a commit that referenced this pull request May 25, 2022

Fix a couple of release issues

b26313b

Found in #2954 - Delete `@lit-labs/gen-wrapper-angular` changeset, because it's not released. - Also bump `lit`, should have been included in #2952.

augustjk mentioned this pull request Jun 1, 2022

[ssr] Hydration on self closing tags with properties error #2802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aomarks commented May 25, 2022 •

edited

changeset-bot bot commented May 25, 2022 •

edited

github-actions bot commented May 25, 2022 •

edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

justinfagnani May 25, 2022

aomarks May 25, 2022

augustjk left a comment

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

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

Conversation

aomarks commented May 25, 2022 • edited

Background

Bug

Fix

Alternative idea

changeset-bot bot commented May 25, 2022 • edited

🦋 Changeset detected

github-actions bot commented May 25, 2022 • edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

justinfagnani May 25, 2022

Choose a reason for hiding this comment

aomarks May 25, 2022

Choose a reason for hiding this comment

augustjk left a comment

Choose a reason for hiding this comment

aomarks commented May 25, 2022 •

edited

changeset-bot bot commented May 25, 2022 •

edited

github-actions bot commented May 25, 2022 •

edited