capricorn86 / happy-dom

A JavaScript implementation of a web browser without its graphical user interface
MIT License
3.36k stars 200 forks source link

Custom Element initialisation order is wrong (connectedCalback) #1505

Open j-o-sh opened 2 months ago

j-o-sh commented 2 months ago

Describe the bug I initially commented this on a super old Ticket (#958) but have since dug deeper and think this deserves a newer issue. (I hope you agree 🙏)

The order when the connectedCallback will get called for customElements seems incoherent and is also dependent on the order of when document.body.innerHTML is changed vs. when customElements.define is called.

Things that depend on the parent DOM (like this.closest) seem to only work when the element is defined AFTER it has been set in the html, while other aspects like this.getAttribute seem to only work when the element is defined BEFORE the html is set.

To Reproduce Steps to reproduce the behavior:

test('Custom Elements should be connected before the connectedCallback is invoked', () => {
    document.body.innerHTML = '<div id="parent"><foo-bar></foo-bar></div>'
    customElements.define('foo-bar', class extends HTMLElement {
        foo = null
        connectedCallback() {
            this.foo = this.closest('#parent')
        }
    })

    // This fails but would passs if the order of `customElements.define` and
    // `document.body.innerHTML = ` would be reversed
    expect(document.querySelector('foo-bar').foo).not.toBeNull()
})

test('Custom Elements should be able to access their own attributes', () => {
    customElements.define('foo-bar', class extends HTMLElement {
        bar = null
        connectedCallback() {
            this.bar = this.getAttribute('something')
        }
    })
    document.body.innerHTML = '<div id="parent"><foo-bar something="baz"></foo-bar></div>'

    // This passes
    expect(document.querySelector('foo-bar')?.getAttribute('something')).toEqual('baz')

    // This fails but would passs if the order of `customElements.define` and
    // `document.body.innerHTML = ` would be reversed
    expect(document.querySelector('foo-bar').bar).toEqual('baz')
})

Expected behavior The order of defining a custom element and setting the html should not matter. Rather, the element should be known in the dom as an unknown HTMLElement when the html is written before the custom element is defined and known as the custom element after.

The connectedCallback of the custom element should only be invoked after connection to the DOM has completed and parent context as well as attributes, slots and inner context are available to the element.

Screenshots I wrote two unit tests to illustrate this.

Device: doesn't matter

Additional context

I could be willing to try and fix this bug if you don't find the time to do it but I would need someone to point me in the right direction. I pocked around a bit in the HappyDOM code but wasn't able to find any relevant section, unfortunatelly.

Hope you'll find the time to look into this. Thanks for an awesome testing library.

Cheers ✌️

aearly commented 1 month ago

I ran into this same issue, at least with setting/reading properties in constructors.

I think when parsing html, it needs to defer upgrading custom elements until the entire string has been parsed and a provisional DOM has been created. Right now it eagerly upgrades custom elements, which causes constructors and connectedCallbacks to fire before the rest of the DOM is ready.

aearly commented 1 month ago

I looked into this some more, and I don't think it's possible to work around, if you're trying to access attributes in constructors.

In pseudocode HappyDom does something similar to:

Class Document {
  //...
  createElement(nodeName) {
    if (isCustomElement(nodeName)) {
       const elementClass = getElementClassFromRegistry(nodeName);
       return elem new elementClass(); // <----
    }
  }
  //...
}

createElement is called during parsing, and the attributes and child/parent nodes are set later, but the element constructor has already been called. If you are trying to read properties in the constructor they will not be set yet.

Most people will define a custom element as class MyElement extends HTMLElement {} using the ES2015 class syntax. It explicitly disallows calling a constructor outside of new. If you define your custom element using old-school class syntax like:

function MyElement () {
  this.foo = this.getAttribute('bar')
}
Object.setPrototypeOf(MyElement.prototype, HTMLElement.prototype)

HappyDom could do something like this during the element upgrade process:

    #onCustomElementConnected(): void {
        const elementClass = this[PropertySymbol.window].customElements.get(
            this.nodeName.toLowerCase()
        );
        Object.setPrototypeOf(this, elementClass.prototype);
        elementClass.apply(this); // <---
        return;
    }

But with ES2015 class syntax, it will throw an error. Browsers must do something special with classes that extend HTMLElement so they can work around this and upgrade elements while keeping the same object reference.

aearly commented 1 month ago

I also can't reproduce your issue with connectedCallback logic, only if I put that logic in the constructors. You test cases pass for me.