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

WIP: Initial work for Custom Elements support #1872

Closed
wants to merge 4 commits into from

Conversation

tbranyen
Copy link

Changes here paired with jsdom/webidl2js#44 will
enable ./test-web-components.js to work. You will need to npm install before running node ./test-web-components.js.

Changes here paired with jsdom/webidl2js#44 will
enable `./test-web-components.js` to work. You will need to `npm
install` before running `node ./test-web-components.js`.
@domenic
Copy link
Member

domenic commented May 29, 2017

Please be sure to follow the HTML and DOM Standards when making these changes. For example they do not look up properties on the instance, but instead store them at definition time. And CustomElementsRegistry needs to be defined with Web IDL, not inline in the Window constructor. Etc.

This is a big ambitious project, but it sounds like you're interested in sinking some time into it, so that's great! Just please follow the spec, and use official web platform tests, not ad-hoc ones.

@tbranyen
Copy link
Author

tbranyen commented May 29, 2017

Awesome @domenic. I'll work on spec-compliance and cleaning up my implementation. I wanted to at least make sure it was even do-able and what the path of integration might look like. So I didn't take into account standards or doing things the-right-way.

That said, I'm a bit stuck on how to get the associated defaultView from the HTMLElement constructor or document.createElement. Lets say you have code that looks like this:

customElements.define('hello-world', class extends HTMLElement {
  constructor() {
    this.innerHTML = `<b>Hello world</b>`;
  }
})

document.createElement('hello-world');

How can we make the HTMLElement-impl and Document-impl aware of where customElements are defined? Assuming I tie everything in with the IDL and let the generator scaffolding out the source, is there any clean way to reference the customElements registry from the output? I'm doing global.window currently which sucks since end users need to do that global.window = window.

I'll check the HTMLConstructor specification and see if it lends any clues as to how it finds the associated window.

@tbranyen
Copy link
Author

Hrm:

Let registry be the current global object's CustomElementRegistry object.

Similarly, the current global object is the global object of the current Realm Record.

Before it is evaluated, all ECMAScript code must be associated with a realm. Conceptually, a realm consists of a set of intrinsic objects, an ECMAScript global environment, all of the ECMAScript code that is loaded within the scope of that global environment, and other associated state and resources.

Sounds like the vm.createContext(window) code, vm == realm?

@Sebmaster
Copy link
Member

Sounds like the vm.createContext(window) code, vm == realm?

Sadly, that doesn't work in jsdom because we don't define HTMLElement in the VM context.

document.createElement is easy, since it goes through our own code. Anything where the on-page code can directly new up an interface directly, we have to cheat in some way. We mostly do it by wrapping the constructor. The code that implements Window contains a lot of these tricks. See the definition of Image and XMLHttpRequest for example.

@snuggs
Copy link
Contributor

snuggs commented May 31, 2017

This is awesome @tbranyen

@durandj
Copy link

durandj commented Jul 19, 2017

I don't suppose this PR is still active is it? I was really hoping to use this for testing my web components.

@tbranyen
Copy link
Author

@durandj you should be able to use jsdom-wc (from npm) to test certain features. I will be taking another stab at this soon. Real life in the way ATM though.

let Constructor = null;

if (Constructor = this._global.window.customElements.get(name)) {
return idlUtils.tryImplForWrapper(new Constructor());
Copy link

Choose a reason for hiding this comment

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

@domenic
Hi, I trying to test similar implementation by web-platform-tests, and I have strange problem.
If I will run in node code like

import {JSDOM} from 'jsdom-wc';
const dom = new JSDOM(`<!DOCTYPE html>`);
global.HTMLElement = dom.window.HTMLElement;
global.window = dom.window;
global.document = dom.window.document;
class A extends HTMLElement {
    constructor() {
        super();
        console.log('AAAA');
    }
}
new A();

Everything works fine.
But if I want to run similar code from web-platform-tests I have a error:

TypeError: Illegal constructor
    at CustomElement.HTMLElement (...\jsdom\living\generated\HTMLElement.js:14:10)
    at ...
    at Object.<anonymous> (http://w3c-test.org/custom-elements/attribute-changed-callback.html:6:31)

I putted similar code to create-jsdom.js just before window.shimTest = () => { and I see similar result.
The 'window.HTMLElement' is a function from ...\jsdom\living\generated\HTMLElement.js so calling of new is throwing same exception.
In most of wpt there is no direct call of the elements constructors (instead of this document.creatElement is called) that why I think this is not occurs on other tests. But for custom-elements we have to call the constructor directly, and I do not know why on wpt this is no working. Any suggestion ?

Copy link
Member

Choose a reason for hiding this comment

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

@majo44 We do not support jsdom-wc.

Copy link

Choose a reason for hiding this comment

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

@TimothyGu
Hi, I do not ask you support jsdom-wc. Forget about the previous question it was my mistake (this is expected behavior which fallow spec).

@@ -28,6 +29,20 @@ class HTMLElementImpl extends ElementImpl {
});
}

attachShadow(options) {
switch (options.mode) {
Copy link

Choose a reason for hiding this comment

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

This appears to be missing the delegateFocus option that the spec allows for to allow focus changes through the shadow root.

@caridy
Copy link

caridy commented Oct 3, 2017

@tbranyen do you have plans or time to continue pushing for this?

@tbranyen
Copy link
Author

tbranyen commented Oct 3, 2017

@caridy at this point not really, my implementation with jsdom-wc works pretty well and if others want to improve on that I'd be happy to push out releases on npm, but I don't have the time to go through the IDL/specification dance to get this implemented properly to land.

@mraerino
Copy link

I'd be willing to pick up on this.
@domenic do you recommend continuing the work done by @tbranyen or rather start from scratch?

@caridy
Copy link

caridy commented Oct 25, 2017

@mraerino it seems that @TimothyGu and @domenic are in agreement on how to move forward based on #1030 (comment). We can also help once we have a clear direction on how to move forward. Can you work on a concrete plan?

@TimothyGu
Copy link
Member

Work-in-progress that may be of interest:

@mraerino
Copy link

mraerino commented Oct 26, 2017

Ok, so let's continue this in #1030
My current progress is in https://github.com/mraerino/jsdom/tree/custom-elements-spec

@domenic
Copy link
Member

domenic commented Oct 29, 2017

Let's close this and roll future work into #1030. Excited to see such progress!

@domenic domenic closed this Oct 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants