-
-
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
Add custom element support #2548
Conversation
test/web-platform-tests/to-run.yaml
Outdated
DIR: custom-elements | ||
|
||
CustomElementRegistry.html: [fail, Promise identity discontinuity, | ||
webidl2js doesn't deal well with tests using Proxies to verify properties access] |
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.
Can you say more about these failures? webidl2js strives to work well in these ways, so fixing it should not be too hard.
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.
The test wraps the custom element constructor into a Proxy and checks all the property access on it (constructor
, prototype
, attributeChangedCallback
, ...). Because webidl2js uses a Symbol to like the wrapper to the implementation, the Proxy catches the Symbol property access making the test fail.
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.
WebIDL2JS should probably use a WeakMap to map wrappers to implementations.
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.
I don't it is the case: https://github.com/jsdom/webidl2js/blob/master/lib/output/utils.js#L34-L36
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.
I’ve created jsdom/webidl2js#155 to update WebIDL2JS to use WeakMaps for the wrapper → impl mapping.
test/web-platform-tests/to-run.yaml
Outdated
reactions/Range.html: [fail, Range is not implemented, https://github.com/jsdom/jsdom/issues/317] | ||
reactions/Selection.html: [fail, Selection is not implemented, https://github.com/jsdom/jsdom/issues/937] | ||
throw-on-dynamic-markup-insertion-counter-construct.html: [timeout, Document.write implementation is not spec compliant] | ||
throw-on-dynamic-markup-insertion-counter-reactions.html: [timeout, Document.write implementation is not spec compliant] |
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.
I didn't see an implementation of the flag in our document.write implementation. Even if we can't get the tests fully passing, maybe it's worth adding? Not sure.
@@ -158,10 +173,16 @@ class JSDOMParse5Adapter { | |||
Object.assign(JSDOMParse5Adapter.prototype, serializationAdapter); | |||
|
|||
function parseFragment(markup, contextElement) { |
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.
These changes look independently valuable. Would it make sense to split them out? Hopefully there are some targeted tests they fix, separate from custom elements, although we might have to write them...
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.
I can extract the <template>
parsing related changes if you feel that's really important. That being said I will need to write additional tests since all the tests covering this case are using custom elements.
So, this PR will be merged? |
As you can see there are unaddressed review comments. |
@pmdartus @domenic |
a0cb9cf
to
7194048
Compare
4cca088
to
b8415de
Compare
Hi all, Thanks! |
No, sorry. Everyone involved is doing this in their free time, not on a schedule. |
package.json
Outdated
@@ -82,7 +83,7 @@ | |||
"st": "^1.2.2", | |||
"watchify": "^3.11.1", | |||
"wd": "^1.11.2", | |||
"webidl2js": "^9.2.2" | |||
"webidl2js": "https://github.com/pmdartus/webidl2js.git#pmdartus/add-processor" |
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.
Using the WIP version to run the test suite on the CI.
Finally, the PR is in pretty good shape. However, I just realized that the const elm = document.createElement('div');
elm.constructor === HTMLDivElement
// Expected: true, Actual: false I don't see any way to get around this without creating a brand new constructor and a brand new prototype chain per realm (jsdom/webidl2js#133: part. 1 + part. 2). I would be happy to take a stab at this it's a blocker. |
Thank you to all working on this, it is very helpful! |
I would really prefer this not to be the case on the default setting. For the purpose of getting this merged in a reasonable timeline, however, I'd be happy with an “experimental flag” that enables custom elements at the cost of such regression, and iterate on the design of webidl2js. |
Sound good to me, I will hide the custom elements behind a flag. |
Let's put on hold this PR, until the constructor reform lands. |
@pmdartus Seems you fixed jsdom/webidl2js#133 (in PR jsdom/webidl2js#140) which is awesome! Will this PR here be continued? :) Just thankful you are doing all this. |
@thernstig Thanks for the kind words. I can confirm you that I am still working on this, I am curerntly working on integrating the latest changes in this branch. |
b79a423
to
32fb6e4
Compare
@pmdartus Eager and all, did you manage to continue on this or did life come in the way? :) |
c1e7857
to
e5d0414
Compare
package.json
Outdated
@@ -81,7 +82,7 @@ | |||
"st": "^2.0.0", | |||
"watchify": "^3.11.1", | |||
"wd": "^1.11.2", | |||
"webidl2js": "^13.0.0" | |||
"webidl2js": "pmdartus/webidl2js#pmdartus/add-processor" |
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.
Don’t forget to use the production WebIDL2JS release before merging.
These will be used for jsdom to implement custom elements (jsdom/jsdom#2548).
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.
Spotted a few things, but really amazing stuff.
define(name, ctor, options) { | ||
const { _globalObject } = this; | ||
|
||
if (typeof ctor !== "function" || !isConstructor(ctor)) { |
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.
if (typeof ctor !== "function" || !isConstructor(ctor)) { | |
if (!isConstructor(ctor)) { |
(or, move that check into isConstructor)
require("./generated/AbortController"), | ||
require("./generated/AbortSignal") | ||
]; | ||
const generatedInterfaces = { |
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.
You could probably leave this as-is and then do a single pass over the array to build a hash table from arrayMember.interface.name
or something like that... seems a bit nicer for maintenance.
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.
The interface
is not exposed anymore on the generated interfaces since the constructor and prototype reform. And there is, unfortunately, no other way today to retrieve the interface name.
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.
Good point. Can we add a comment explaining that this needs to stay as-is in order to make bundlers not freak out? Right now it seems like a really tempting refactoring target.
test/web-platform-tests/to-run.yaml
Outdated
parser/parser-fallsback-to-unknown-element.html: [fail, Usage of external scripts doesn't block HTML parsing, https://github.com/jsdom/jsdom/issues/2413] | ||
parser/parser-sets-attributes-and-children.html: [fail, Usage of external scripts doesn't block HTML parsing, https://github.com/jsdom/jsdom/issues/2413] | ||
parser/parser-uses-constructed-element.html: [fail, Usage of external scripts doesn't block HTML parsing, https://github.com/jsdom/jsdom/issues/2413] | ||
parser/serializing-html-fragments.html: [fail, parse5 doesn't support is attribute for serialization] |
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.
Can we file an issue for this?
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.
I realized as I was writing the issue for this, that adding a new parse5 tree adapter method is probably overkill for handing the is
attribute serialization. The parse5 API is already complex, I don't think adding more complexity into the mix is the right option. For this reason, I added some special logic to handle the is
attribute in the getAttrList
handler to add the is
attribute if necessary there.
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 continues to look great. I added a few more minor comments, but I could fix them myself. Unfortunately, in merging #2770, I created a decent number of merge conflicts, which will require your expertise to hash out :(.
It does seem like things are stable over in webidl2js land though, so I'll go release a new version of that at least, so you can point to it properly.
// https://html.spec.whatwg.org/#dom-customelementregistry-whendefined | ||
whenDefined(name) { | ||
if (!isValidCustomElementName(name)) { | ||
return Promise.reject(new DOMException("Name argument is not a valid custom element name.", "SyntaxError")); |
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.
new DOMException
doesn't seem right here. I guess this isn't tested?
Document-createElement.html: [fail, :defined is not defined and throws] | ||
HTMLElement-attachInternals.html: [fail, Not implemented] | ||
HTMLElement-constructor.html: [fail, webidl2js doesn't deal well with tests using Proxies to verify properties access] | ||
adopted-callback.html: [fail, "Failing test due to https://github.com/whatwg/dom/issues/813"] |
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.
It seems like there's been some progress on the spec issue here; is this still right?
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.
test/web-platform-tests/to-run.yaml
Outdated
HTMLElement-attachInternals.html: [fail, Not implemented] | ||
HTMLElement-constructor.html: [fail, webidl2js doesn't deal well with tests using Proxies to verify properties access] | ||
adopted-callback.html: [fail, "Failing test due to https://github.com/whatwg/dom/issues/813"] | ||
attribute-changed-callback.html: [fail, attributeChangedCallback doesn't work with CSSStyleDeclaration] |
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.
Is this fixable? We already do some fun stuff to synchronize the style="" attribute and CSSStyleDeclaration, if I recall.
If it's fixable but more work than you want to tackle right now, then filing a follow-up issue with a description of the problem/solution space, and pointing to it, would work well.
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.
It's fixable, however, it would require some work on the cssstyle
package: jsdom/cssstyle#113
Hmm, well, I guess we still have jsdom/webidl2js#159, so I won't do a release until we get that settled... |
4800853
to
57b17a4
Compare
I rebase the PR and upgraded the version of webidl2js. However, I realized that the Proxy performance improvement broke (jsdom/webidl2js@7c8037f) broke [CEReactions] and [HTMLConstructor] processor (jsdom/webidl2js@fda810e) only after upgrading webidl2js 😢. In short, the [CEReactions] changes rely on the fact that the I don't see yet how we can preserve the performance optimization by hoisting the trap object out of the closure and at the same time be able to retrieve the associated |
@pmdartus You need to add the following to const globalObject = ${O}[impl]._globalObject; I’m doing that in jsdom/webidl2js#163. The jsdom/lib/jsdom/living/events/EventTarget-impl.js Lines 21 to 25 in 1ebb1fc
I’m also doing jsdom/webidl2js#162 to allow processors to add package imports. |
a983cb3
to
925f1b0
Compare
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.
If CI is happy then...
@pmdartus thanks so much for your work on this!! Congrats on merging! |
This PR introduces native support for custom elements in jsdom.
While the core of the implementation is in place, the PR is currently at the draft stage to flush out some design decisions mainly around
[HTMLConstructor]
and[CEReactions]
.Fixes #1030