-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
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`.
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. |
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 customElements.define('hello-world', class extends HTMLElement {
constructor() {
this.innerHTML = `<b>Hello world</b>`;
}
})
document.createElement('hello-world'); How can we make the I'll check the |
Hrm:
Sounds like the |
Sadly, that doesn't work in jsdom because we don't define HTMLElement in the VM context.
|
This is awesome @tbranyen |
I don't suppose this PR is still active is it? I was really hoping to use this for testing my web components. |
@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()); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
@tbranyen do you have plans or time to continue pushing for this? |
@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 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? |
Work-in-progress that may be of interest:
|
Ok, so let's continue this in #1030 |
Let's close this and roll future work into #1030. Excited to see such progress! |
Changes here paired with jsdom/webidl2js#44 will
enable
./test-web-components.js
to work. You will need tonpm install
before runningnode ./test-web-components.js
.