Lit-html: Modifying DOM during render causes off by one error

[pjtatlow]

If I have a custom element that has a "setAttributeCallback", or a setter for a property that changes the DOM inside that element, the document walker will be off by one and can potentially put attributes, properties, or event listeners on the wrong elements (at least I'm pretty sure that's what's happening).

Say I have a class like this:

class FooBar extends HTMLElement {

    public static get observedAttributes(): string[] {
        return ['foo']; // attributeChangedCallback is invoked on any changes for these attributes.
    }

    constructor() {
        super();
    }

    public attributeChangedCallback(attrName, oldVal, newVal): void {
        const bar = this.getAttribute('foo');
        if (bar && bar.length > 0) {
            this.innerText = bar;
        }
    }

}

window.customElements.define('foo-bar', FooBar);

and I use it with lit-html like this:

render(html`
    <foo-bar foo="bar"></foo-bar>
    <div class="${'my-favorite-class'}"></div>
`, document.body);

you'll notice an error that says Uncaught TypeError: this.element.setAttribute is not a function. This is because the element that is given to the AttributeCommitter is actually a Text (with a value equal to the whitespace between the </foo-bar and the <div>, not the HTMLDivElement that it was supposed to be.

I'm not sure I understand the code well enough to see an obvious solution, but it seems like it might be challenging to detect when a custom element modifies it's own DOM and throws off the DOM walker. It seems like this should at least be documented so another poor soul doesn't have to spend 3 hours tracking this down!

Let me know if you need any more details.

[ruphin]

First the obligatory message: _You should never modify DOM that is rendered by lit-html, or you will probably break things_.

With that out of the way, the problem here is with the implementation of the custom element. Generally lit-html is fairly robust but unfortunately several things have gone wrong here.

First, the custom element is not using ShadowDOM, and is writing to the LightDOM instead. This creates a leaky abstraction, and potentially causes problems like this. ShadowDOM was invented specifically for this reason, and the whole problem would be avoided if the custom element uses ShadowDOM. This example goes to show why using ShadowDOM should always be the default choice for custom elements (even for a case as trivial as this).

Second, the custom element is modifying DOM before the connectedCallback (_attributeChangedCallback_ is called before _connectedCallback_ if attributes are present in the document source string). Lit-html assumes (as per the custom element specification) that elements do not modify the lightDOM before the connectedCallback is called. To guarantee this, the attributeChangedCallback should only store element properties, trigger deferred renders, or set some flag that marks the element as 'dirty' to some render system, and have renders deferred to after the connectedCallback.

Both of these patterns are important for Web Components for a reason, because without them we have no encapsulation and things can (and will) easily break. Right now it's breaking lit-html, but it could just as easy cause problems for any framework or tool that is using this element.

If either of these problems are resolved, lit-html will work as designed. Here's a very basic version of the FooBar element with both of the above issues addressed. Note that base elements for custom elements (like Lit-Element or PolymerElement or Gluon) will generally do these things for you automatically.

class FooBar extends HTMLElement {
  static get observedAttributes() {
    return ['foo'];
  }

  constructor() {
    super();
    this.foo = "";
    this.attachShadow({mode: 'open'});
    this.textNode = document.createElement('span');
    this.shadowRoot.appendChild(this.textNode);
  }

  async render() {
    await 0;
    this.textNode.innerText = this.foo;
  }

  attributeChangedCallback(attrName, oldVal, newVal) {
    if (attrName == 'foo' && newVal && newVal.length > 0) {
      this.foo= newVal;
      this.render();
    }
  }

  connectedCallback() {
    this.render();
  }
}

[pjtatlow]

Haha I figured it was probably the way I was doing things, but that makes a lot of sense. Thanks for explaining that for me!

[aadamsx]

Why a great answer @ruphin!! Thank you!