Replace deprecated method getInnerHTML with getHTML

[DannyMoerkerke]

Type of Change

Alignment with specs

Summary

Replace the deprecated Element.prototype.getInnerHTML with Element.prototype.getHTML in dom-shim.js

Details

This issue is in addition to my PR #171

Element.prototype.getInnerHTML has been deprecated in favor of Element.prototype.getHTML (https://developer.mozilla.org/en-US/docs/Web/API/Element/getHTML).

Currently in dom-shim.js, the setter for innerHTML for the HTMLTemplateElement automatically and incorrectly adds a <template> tag with a shadowrootmode attribute to the HTML content of the HTMLTemplateElement itself.

The result is that the innerHTML property of the HTMLTemplateElement will return its HTML contents including the <template> tag which is incorrect and not according to the specs as it should only return its contents without this <template> tag.

Element.prototype.getInnerHTML should be replaced with Element.prototype.getHTML which provides an options argument that enables the serialization of child nodes that are shadow roots:

<section>
  <template shadowrootmode="open" shadowrootserializable>
    <p>foo</p>
  </template>
  <p>bar</p>
</section>

section.getHTML();

// returns:
<p>bar</p>

section.getHTML({{serializableShadowRoots: true});

// returns:
<template shadowrootmode="open" shadowrootserializable>
   <p>foo</p>
</template>
<p>bar</p>

This also makes sure that Custom Elements that have their shadow roots set through innerHTML can be server-side rendered:

const shadowRoot = this.attachShadow({mode: 'open'});
shadowRoot.innerHTML = `...`;

[thescientist13]

Hey @DannyMoerkerke thanks for much for reporting this and providing that example of getHTML, definitely helping me to better understand the issue.

So if I were to summarize in my own words what you are proposing:

1. move auto-wrapping out of the setter
2. instead, enable auto-wrapping through the getter (now using a combination of getHTML + serializableShadowRoots: true)

Regarding point 1, I think this aligns with an observation @briangrider had while working on #170 that he saw some double wrapping of <template> tags, like in this test case. Does that align with your testing / validation as well, and was double wrapping happening in those two other test cases in your PR as well? Since it seems like the auto-wrapping is just moving, just wanted to make sure it explains those other two test case changes in your PR, for my own understanding

So in general, with the changes in your PR, no user would ever have to manually do this anymore, yes?

this.shadowRoot.innerHTML = `<template shadowrootmode="open"> ... </template>`

And the example from the README (the "Get HTML!" output) of manually appending a template would still work? (which based on no other test cases changing in your PR, seems like it would?)

const template = document.createElement('template');

template.innerHTML = `
  <footer class="footer">
    <h4>My Blog &copy; ${new Date().getFullYear()}</h4>
  </footer>
`;

class Footer extends HTMLElement {
  connectedCallback() {
    if (!this.shadowRoot) {
      this.attachShadow({ mode: 'open' });
      this.shadowRoot.appendChild(template.content.cloneNode(true));
    }
  }
}

export default Footer;

customElements.define('wcc-footer', Footer);

And so if WCC is going to do auto-wrapping now, at least it would based on actual standard API specifications?

---

Reading the MDN docs, I wasn't sure how to interpret what they were saying, if it would auto-wrap, or just include the <template> tag in the return value if a <template> tag already existed, be it manually through innerHTML or appending a template.

I did create a demo to familiarize with getHTML behavior but couldn't get it to work in returning a <template> tag when calling getHTML({ serializableShadowRoots: true }), but I'm probably just doing it wrong 😅
https://github.com/thescientist13/serializable-shadow-roots

If it is intended to do auto-wrapping (or we're proposing this as an explicit value added feature of WCC) I think it would be important to call that out in the documentation / examples as part of the PR, since it's probably not obvious from the outside that setting innerHTML would generate a <template shadowrootmode="open"> automatically.

Just want to avoid any "self-inflicted" double wrapping by user's (still) doing this by educating and informing of WCC's behavior

this.shadowRoot.innerHTML = `<template shadowrootmode="open">...</template>`

---

As a bit of an aside, I'm curious if you have any thoughts on this issue - #65?

At the time I was stuck on how to get access to mode, so just wanted to throw that one out there to see if you have any thoughts on how to make that possible (and we can discuss over in that issue if it's of interest to you). 🤞

getHTML({ serializableShadowRoots = false }) {
  const mode = this.shadowRoot.mode ?? false;
   
  return this.shadowRoot && serializableShadowRoots ?
    `<template shadowrootmode="${mode}">${this.shadowRoot.innerHTML}</template>` : this.innerHTML;
}

---

Thanks again for putting all this together, and apologies in advance for having such a lengthy reply, but definitely excited to see what we can achieve here, and certainly appreciate the opportunity to get learn about new web APIs! 🧠

[DannyMoerkerke]

Regarding point 1, I think this aligns with an observation @briangrider had while working on #170 that he saw some double wrapping of <template> tags, like in this test case. Does that align with your testing / validation as well, and was double wrapping happening in those two other test cases in your PR as well? Since it seems like the auto-wrapping is just moving, just wanted to make sure it explains those other two test case changes in your PR, for my own understanding

I haven't been able to log the HTML of those two test cases but I noticed that they failed because the elements that were asserted were not found in the DOM. Removing the hard-coded adding of the <template> fixed the tests.

So in general, with the changes in your PR, no user would ever have to manually do this anymore, yes?

this.shadowRoot.innerHTML = `<template shadowrootmode="open"> ... </template>`

Exactly, the <template> will now be returned when getHTML({serializableShadowRoots: true}) is invoked.

And the example from the README (the "Get HTML!" output) of manually appending a template would still work? (which based on no other test cases changing in your PR, seems like it would?)

const template = document.createElement('template');

template.innerHTML = `
  <footer class="footer">
    <h4>My Blog &copy; ${new Date().getFullYear()}</h4>
  </footer>
`;

class Footer extends HTMLElement {
  connectedCallback() {
    if (!this.shadowRoot) {
      this.attachShadow({ mode: 'open' });
      this.shadowRoot.appendChild(template.content.cloneNode(true));
    }
  }
}

export default Footer;

customElements.define('wcc-footer', Footer);

Yes, this works as expected.

And so if WCC is going to do auto-wrapping now, at least it would based on actual standard API specifications?

Yes, it is: https://developer.mozilla.org/en-US/docs/Web/API/Element/getHTML

Reading the MDN docs, I wasn't sure how to interpret what they were saying, if it would auto-wrap, or just include the <template> tag in the return value if a <template> tag already existed, be it manually through innerHTML or appending a template.

No, the <template shadowrootmode="open"> is returned when the custom element has a serializable shadow root.

I did create a demo to familiarize with getHTML behavior but couldn't get it to work in returning a <template> tag when calling getHTML({ serializableShadowRoots: true }), but I'm probably just doing it wrong 😅 https://github.com/thescientist13/serializable-shadow-roots

There are some issues here. First, a shadow root is only serializable (https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/serializable) if it's created with the options.serializable parameter set to true when created with Element.attachShadow():

this.attachShadow({ mode: "open", serializable: true });

or when the shadowrootserializable attribute is set on the <template>:

<template shadowrootmode="open" shadowrootserializable>

Also, getHTML() is called on the element itself, not its shadow root:

const htmlSerializedTemplate = cardWithTemplate.getHTML({ serializableShadowRoots: true });

If it is intended to do auto-wrapping (or we're proposing this as an explicit value added feature of WCC) I think it would be important to call that out in the documentation / examples as part of the PR, since it's probably not obvious from the outside that setting innerHTML would generate a <template shadowrootmode="open"> automatically.

Setting innerHTML does not add a <template> it's only returned when getHTML() is called with the right options and the shadow root is serializable.

Just want to avoid any "self-inflicted" double wrapping by user's (still) doing this by educating and informing of WCC's behavior

this.shadowRoot.innerHTML = `<template shadowrootmode="open">...</template>`

They should never do this for the reason stated above.

Strictly speaking, we should only render the shadow root when it's serializable but users may not specify this when calling attachShadow and only the component's light DOM would then be server-side rendered. I'm afraid this will lead to unexpected results.

On the other hand, we should follow the specs so we should consider adding this to the docs.

As a bit of an aside, I'm curious if you have any thoughts on this issue - #65?

At the time I was stuck on how to get access to mode, so just wanted to throw that one out there to see if you have any thoughts on how to make that possible (and we can discuss over in that issue if it's of interest to you). 🤞

getHTML({ serializableShadowRoots = false }) {
  const mode = this.shadowRoot.mode ?? false;
   
  return this.shadowRoot && serializableShadowRoots ?
    `<template shadowrootmode="${mode}">${this.shadowRoot.innerHTML}</template>` : this.innerHTML;
}

I'll have a look.

Thanks again for putting all this together, and apologies in advance for having such a lengthy reply, but definitely excited to see what we can achieve here, and certainly appreciate the opportunity to get learn about new web APIs! 🧠

You're welcome! Thanks for this great project, I'm excited about it as well!

[briangrider]

Hi @DannyMoerkerke,

It looks like we're tackling some of the same issues and I wanted to show you what I've been working on with @thescientist13 and get your take.

In an attempt to get child element properties working in wcc (and in turn many features of various frameworks like litHTML's child element properties, etc.), I've spent a lot of time updating the DOM shim to work more like the browser implementation of the DOM, specifically, storing childNodes as objects and serializing them when accessing innerHTML rather than storing everything as strings. This simplifies many operations in wcc itself, improves performance by cutting down on the number of parses and serializes in wcc when appending templates and childNodes (instead of using innerHTML), and ultimately, makes it easier to align things with the spec.

For example, in regards to simplifying wcc, I was able to simplify renderComponentRoots (and renderToString) pretty significantly. You can see we can remove getInnerHTML (or getHTML), innerHTML, elementHTML, elementTree, hasLight, renderLightDomChildren, and VOID_ELEMENTS with this new implementation:
https://github.com/briangrider/wcc/blob/master/src/wcc.js

Here's the new dom shim:
https://github.com/briangrider/wcc/blob/master/src/dom-shim.js

With this implementation, we don't even need to shim getHTML (but we can if there's a good reason).

I would argue that in working toward true isomorphic web components, we should generally be matching the user's browser-based expectations and be forgiving of leaving out the serializable property when instantiating a shadowRoot but maybe respect it if it's specifically set to false.

btw, this line here: https://github.com/briangrider/wcc/blob/master/src/dom-shim.js#L230
is just there to pass those tests where innerHTML strings are already wrapped in template tags. When those tests are fixed like you proposed, that will be removed

If we don't shim getHTML, we could achieve a forgiving implementation of the spec like this:

class ShadowRoot extends DocumentFragment {
  constructor(options) {
    super();
    this.mode = options.mode ?? 'closed';
    // Unlike the spec, default to true to better match browser experience/expectations
    this.serializable = options.serializable ?? true;
    this.adoptedStyleSheets = [];
  }

  get innerHTML() {
    return this.childNodes ? serialize({ childNodes: this.childNodes }) : '';
  }

  set innerHTML(html) {
    html = this.serializable ? `<template shadowrootmode="${this.mode}" shadowrootserializable>${html}</template>` : html;
    this.childNodes = parseFragment(html).childNodes;
  }
}

Again, I think users should be able to take a complex project (with many web components) that was built for the browser, and render it on the server without modifying each and every component. There are also frameworks people may be using where they don't even have access to the serializable option for their shadow roots.

In my opinion, it is worth the slight deviation from spec on this (specifically, defaulting serializable to true instead of false). As a user coming to wcc, I would expect I can just take my working code in the browser and then render it on the server without issue. I imagine having to remind everyone to set serializable to true whenever attaching a shadow seems like it could hurt user adoption. Let me know your thoughts!

[thescientist13]

@DannyMoerkerke

There are some issues here. First, a shadow root is only serializable (https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/serializable) if it's created with the options.serializable parameter set to true when created with Element.attachShadow():

Ah, this is definitely the part I was missing, and was able to confirm as such in @briangrider 's demo, so this all excellent to hear!

---

So know that I've had a chance to dig into #178 a bit deeper, it does seem to cover everything we've been discussing so far with a bonus

• refactor to use parse5
• Replaced deprecated method getInnerHTML with getHTML #171
• support configurable shadowroot attribute for <template> tags #65

I can understand that #178 certainly makes it much easier to achieve all those, and in fact might make it "harder" to not implement them, so while I would traditionally prefer to see mode and serializable land as their own PRs in addition, I do see how taking them on as a big-bang is probably the happiest path towards the end state we are all looking for.

So I'll leave it up to you both @briangrider and @DannyMoerkerke how you feel about landing this all at once, and that if we do go with the big bang, that @briangrider you would also need to absorb some of the extra feedback for #171 , mainly adding documentation and / or updating any examples on the WCC website, and validating all the sandbox examples still work and / or need updating.

Thoughts? Also happy to setup a chat or start a thread in the Project Evergreen Discord if that will help, but sounds like we have everything we need, and just want to make sure we're all aligned on the path to get there. If we feel good about #178 then I am happy to give it a thorough test against Greenwood's test suite and a couple sample projects, and then we can land that and incorporate part 2 of @briangrider work, which I can revalidate in the same way. From there seems like we should be on fast track to release a new minor version of WCC (v0.16.0). 💯

---

So cool and super excited for both of your contributions and input here, thank you so much! 🙇‍♂️

[briangrider]

So I'll leave it up to you both @briangrider and @DannyMoerkerke how you feel about landing this all at once, and that if we do go with the big bang, that @briangrider you would also need to absorb some of the extra feedback for #171 , mainly adding documentation and / or updating any examples on the WCC website, and validating all the sandbox examples still work and / or need updating.

Sounds good! So @DannyMoerkerke, what I'd love to take from your PR (since getInnerHTML/getHTML won't exist in WCC anymore) is:

1. Updating the ShadowRoot class as mentioned above so that serializable can be set to false when attaching a shadowRoot. This deviates from spec slightly since serializable usually defaults to false but it a) better matches user expectations when server rendering web components built for the browser, b) cuts down on potential issue reports, and c) more easily allows for isomorphic web components without refactoring for server rendering. Since serializable can only be set when attaching a shadow root, and a shadow root can only be attached once (we should probably throw an error for this in a future PR btw), that means we can determine right away whether the declarative shadow dom should be serialized for a particular element. I put together a little codepen here testing various cases:
   https://codepen.io/scmrecordings/pen/NWQQqOy?editors=1111 If we were to do things exactly to spec, it seems like setting serializable to false, or setting serializableShadowRoots to false with getHTML leads to nothing being rendered in most cases, not just a lack of a wrapping template, so maybe down the line we should look at reproducing each of those conditions with the dom shim. But since getHTML won't be a part of the dom shim for now, I think we can let it slide.

2. Remove the wrapping template tag from all of the tests mentioned in your PR. When I issued my PR, I put in code to pass those tests by de-wrapping strings that start with a template tag:
   https://github.com/briangrider/wcc/blob/master/src/dom-shim.js#L232
   This was with the intention of fixing the tests and removing that later but if we want to push all of this in a single PR, we can just incorporate all of your modified tests in one go.

3. The missing closing bracket that you caught in counter.js. Good find!

Let me know if this sounds good to everyone!

[thescientist13]

@briangrider

Just to make sure I understand what you are saying in point 1 above, auto-wrapping should only happen if the user authors their custom element as such:

this.attachShadow({ serializable: true })

If that condition is present, than I think we are ok to match that internally with getHTML + serializableShadowRoots: true, and then then just document this as a compliment / convenience as opposed to using document.createElement('template') and appending into the shadow root.

---

As far as the order of operations, I would just like to clarify that I would not like the refactor introducing any new behavior / fixes that are not already in the system. I would prefer fixes and changes in behavior of this significance / meaning get isolated to their own change, to avoid having too much context going on as part of one review.

So I see two options in regards negotiating the two open PRs that I think we would all want to be on the same page for:

Either:

1. Land @DannyMoerkerke PR first, in which case the refactor can now rebase and account for this new serialization behavior

or

1. Land the refactor first, and have Danny rebase and introduce the new serialization detection / intent behavior

My hunch is that we probably want the refactor to land first, which I can handle since it is so foundational, (even if it means slight disruption to the serializable PR) and from there new PRs can come in parallel (element properties, mode, etc).

I know it might sounds a little cumbersome, but I just generally prefer atomic commits where possible, in particular with so many moving parts and especially with these new behaviors we're adding / fixing, it will be preferable to only have to review / test / validate one thing at a time. (and also we can start moving individual conversations / feedback back to individual issues as now this issue has kind of taken on the burden of co-ordinating multiple moving parts, and so kind of getting caught in the crossfire).

---

Let's move this operational chat to Discord and / or a call if needed, since I think we all mainly agree on implementation details and outcomes, and so just need to sequence the open items together.

[briangrider]

Let's move this operational chat to Discord and / or a call if needed, since I think we all mainly agree on implementation details and outcomes, and so just need to sequence the open items together.

Yes, I think moving this to Discord is a good idea!

[DannyMoerkerke]

Hi @briangrider,
I think it ultimately comes down to what parts of dom-shim.js we expect to be used when server-side rendering web components.

What I mean by that is if we expect to server-side render web components that use a method like getHTML internally, it should follow the specs. Having said that, I do think we should be pragmatic and make shadow roots serializable by default or not use getHTML altogether.
However, a good reason to implement getHTML according to the specs is that web components that are server-side rendered with wcc might use it internally. That chance might be slim, but I think we should account for it.

My initial motivation to create the PR was that web components that have their shadow root set through this.shadowRoot.innerHTML = '...' were not properly server-side rendered and when I noticed the dom shim was using the deprecated getInnerHTML I decided to replace it with getHTML.