Add custom element support by pmdartus · Pull Request #2548 · jsdom/jsdom

pmdartus · 2019-04-01T12:23:05Z

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

lib/jsdom/living/helpers/html-element-constructor.js

lib/jsdom/living/helpers/ce-reactions.js

domenic · 2019-04-20T23:22:46Z

test/web-platform-tests/to-run.yaml

+DIR: custom-elements
+
+CustomElementRegistry.html: [fail, Promise identity discontinuity,
+                                   webidl2js doesn't deal well with tests using Proxies to verify properties access]


Can you say more about these failures? webidl2js strives to work well in these ways, so fixing it should not be too hard.

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.

WebIDL2JS should probably use a WeakMap to map wrappers to implementations.

I don't it is the case: https://github.com/jsdom/webidl2js/blob/master/lib/output/utils.js#L34-L36

I’ve created jsdom/webidl2js#155 to update WebIDL2JS to use WeakMaps for the wrapper → impl mapping.

domenic · 2019-04-20T23:24:14Z

test/web-platform-tests/to-run.yaml

+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]


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.

domenic · 2019-04-20T23:25:50Z

lib/jsdom/browser/parser/html.js

@@ -158,10 +173,16 @@ class JSDOMParse5Adapter {
 Object.assign(JSDOMParse5Adapter.prototype, serializationAdapter);

 function parseFragment(markup, contextElement) {


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...

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.

alexgwolff · 2019-07-23T12:15:05Z

So, this PR will be merged?

domenic · 2019-07-23T14:40:02Z

As you can see there are unaddressed review comments.

marioandrest · 2019-07-23T17:41:37Z

@pmdartus @domenic
thank you so much for the addition of customElements to jsdom
this PR has enabled our test stack to use jsdom
would it be possible to rebase your branch to a jsdom 15 release?
(this would remove the need to polyfill MutationObserver)
also, what is the status of moving this PR forward?
thanks!

tibysko · 2019-09-29T08:47:13Z

Hi all,
I understand that there is some issues left to be solved before this PR can be merged. However, is it possible to say roughly when / if ?

Thanks!

domenic · 2019-09-29T08:51:18Z

No, sorry. Everyone involved is doing this in their free time, not on a schedule.

pmdartus · 2019-10-14T05:57:34Z

package.json

    "watchify": "^3.11.1",
    "wd": "^1.11.2",
-    "webidl2js": "^9.2.2"
+    "webidl2js": "https://github.com/pmdartus/webidl2js.git#pmdartus/add-processor"


⚠️Use the released version of webidl2js before merging ⚠️
Using the WIP version to run the test suite on the CI.

pmdartus · 2019-10-14T17:01:52Z

Finally, the PR is in pretty good shape.

However, I just realized that the [HTMLConstructor] processor introduces a major regression with the constructor check. The following snippet will start failing:

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.

gusnaughton · 2019-10-16T03:27:33Z

Thank you to all working on this, it is very helpful!

TimothyGu · 2019-10-16T05:45:14Z

However, I just realized that the [HTMLConstructor] processor introduces a major regression with the constructor check.

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.

pmdartus · 2019-10-18T04:34:39Z

Sound good to me, I will hide the custom elements behind a flag.

pmdartus · 2019-10-19T07:22:11Z

Let's put on hold this PR, until the constructor reform lands.
Discussion: jsdom/webidl2js#133 (comment)

thernstig · 2019-11-27T10:16:11Z

@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.

pmdartus · 2019-11-27T10:54:25Z

@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.

thernstig · 2019-12-22T12:07:51Z

@pmdartus Eager and all, did you manage to continue on this or did life come in the way? :)

test/web-platform-tests/to-run.yaml

lib/jsdom/living/webstorage/Storage-impl.js

ExE-Boss · 2020-01-18T16:06:07Z

package.json

    "watchify": "^3.11.1",
    "wd": "^1.11.2",
-    "webidl2js": "^13.0.0"
+    "webidl2js": "pmdartus/webidl2js#pmdartus/add-processor"


Don’t forget to use the production WebIDL2JS release before merging.

These will be used for jsdom to implement custom elements (jsdom/jsdom#2548).

domenic

Spotted a few things, but really amazing stuff.

lib/jsdom/browser/Window.js

lib/jsdom/browser/parser/html.js

lib/jsdom/living/attributes.js

lib/jsdom/living/custom-elements/CustomElementRegistry-impl.js

domenic · 2020-01-20T07:10:31Z

lib/jsdom/living/custom-elements/CustomElementRegistry-impl.js

+  define(name, ctor, options) {
+    const { _globalObject } = this;
+
+    if (typeof ctor !== "function" || !isConstructor(ctor)) {


Suggested change

if (typeof ctor !== "function" || !isConstructor(ctor)) {

if (!isConstructor(ctor)) {

(or, move that check into isConstructor)

lib/jsdom/living/helpers/html-constructor.js

domenic · 2020-01-20T07:18:34Z

lib/jsdom/living/interfaces.js

-  require("./generated/AbortController"),
-  require("./generated/AbortSignal")
-];
+const generatedInterfaces = {


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.

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.

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.

lib/jsdom/living/nodes/Document-impl.js

domenic · 2020-01-20T07:22:29Z

test/web-platform-tests/to-run.yaml

+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]


Can we file an issue for this?

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.

test/web-platform-tests/to-run.yaml

domenic

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.

domenic · 2020-01-25T22:57:40Z

lib/jsdom/living/custom-elements/CustomElementRegistry-impl.js

+  // 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"));


new DOMException doesn't seem right here. I guess this isn't tested?

lib/jsdom/living/domparsing/parse5-adapter-serialization.js

lib/jsdom/living/helpers/create-element.js

lib/jsdom/living/helpers/custom-elements.js

lib/jsdom/living/interfaces.js

lib/jsdom/living/nodes/Document-impl.js

domenic · 2020-01-25T23:08:04Z

test/web-platform-tests/to-run.yaml

+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"]


It seems like there's been some progress on the spec issue here; is this still right?

We have multiple tests that have been disabled because of this issue. I would be happy to work on this in a different PR.

domenic · 2020-01-25T23:08:58Z

test/web-platform-tests/to-run.yaml

+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]


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.

It's fixable, however, it would require some work on the cssstyle package: jsdom/cssstyle#113

test/web-platform-tests/to-run.yaml

domenic · 2020-01-25T23:12:21Z

Hmm, well, I guess we still have jsdom/webidl2js#159, so I won't do a release until we get that settled...

pmdartus · 2020-01-30T16:48:32Z

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 globalObject is available in all the wrapper methods via the install closure. Since the Proxy traps object is created outside the closure, the globalObject is not accessible anymore.

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 globalObject that is needed for the [CEReactions]. @ExE-Boss what are your thoughts on this?

ExE-Boss · 2020-01-30T22:53:01Z

@pmdartus You need to add the following to prolog of proxyHandler setters:

const globalObject = ${O}[impl]._globalObject;

I’m doing that in jsdom/webidl2js#163.

The _globalObject property is available on EventTargetImpl objects, and several other JSDOM objects:

jsdom/lib/jsdom/living/events/EventTarget-impl.js

Lines 21 to 25 in 1ebb1fc

    
           class EventTargetImpl { 
        
             constructor(globalObject) { 
        
               this._globalObject = globalObject; 
        
               this._eventListeners = Object.create(null); 
        
             }

I’m also doing jsdom/webidl2js#162 to allow processors to add package imports.

domenic

If CI is happy then...

bellindj · 2020-02-24T16:31:21Z

@pmdartus thanks so much for your work on this!! Congrats on merging!

pmdartus commented Apr 1, 2019

View reviewed changes

lib/jsdom/living/helpers/html-element-constructor.js Outdated Show resolved Hide resolved

lib/jsdom/living/helpers/ce-reactions.js Outdated Show resolved Hide resolved

tbranyen mentioned this pull request Apr 15, 2019

Cover Salesforce "Lightning Web Components" webcomponents/custom-elements-everywhere#479

Closed

pmdartus marked this pull request as ready for review April 16, 2019 06:01

domenic reviewed Apr 20, 2019

View reviewed changes

SimenB mentioned this pull request Aug 14, 2019

Please provide window.customElements to be able to test custom Web Components jestjs/jest#8818

Closed

pmdartus force-pushed the pmdartus/custom-element branch from a0cb9cf to 7194048 Compare September 18, 2019 09:16

pmdartus mentioned this pull request Sep 25, 2019

Add [CEReactions] and [HTMLConstructor] processors jsdom/webidl2js#130

Merged

MarcusNotheis mentioned this pull request Sep 26, 2019

@ui5/webcomponents-react: jest configuration to transpile ui5 web component code UI5/webcomponents-react#150

Closed

pmdartus force-pushed the pmdartus/custom-element branch from 4cca088 to b8415de Compare September 28, 2019 09:37

pmdartus commented Oct 14, 2019

View reviewed changes

domenic mentioned this pull request Oct 18, 2019

Constructor & prototype reform jsdom/webidl2js#133

Closed

pmdartus force-pushed the pmdartus/custom-element branch from b79a423 to 32fb6e4 Compare November 28, 2019 08:05

apalaniuk mentioned this pull request Dec 13, 2019

Feature request: Shadow DOM support testing-library/dom-testing-library#413

Open

domenic force-pushed the master branch from 7d3b123 to ae038bf Compare January 2, 2020 00:12

ExE-Boss mentioned this pull request Jan 2, 2020

fix(Window): Avoid re‑assigning module.exports #2771

Closed

benjamind mentioned this pull request Jan 8, 2020

[RFC] feat(testing-library-dom): add esm mirror of testing-library-dom adobe/spectrum-web-components#420

Closed

10 tasks

pmdartus force-pushed the pmdartus/custom-element branch from c1e7857 to e5d0414 Compare January 15, 2020 07:35

ExE-Boss mentioned this pull request Jan 15, 2020

feat: Use WeakMap for wrapper to impl conversion jsdom/webidl2js#155

Closed

ExE-Boss reviewed Jan 15, 2020

View reviewed changes

test/web-platform-tests/to-run.yaml Outdated Show resolved Hide resolved

lib/jsdom/living/webstorage/Storage-impl.js Show resolved Hide resolved

ExE-Boss approved these changes Jan 18, 2020

View reviewed changes

domenic pushed a commit to jsdom/webidl2js that referenced this pull request Jan 20, 2020

Add [CEReactions] and [HTMLConstructor] processors

fda810e

These will be used for jsdom to implement custom elements (jsdom/jsdom#2548).

domenic reviewed Jan 20, 2020

View reviewed changes

pmdartus requested a review from domenic January 21, 2020 07:20

domenic reviewed Jan 25, 2020

View reviewed changes

pmdartus force-pushed the pmdartus/custom-element branch from 4800853 to 57b17a4 Compare January 30, 2020 16:06

ExE-Boss mentioned this pull request Jan 30, 2020

fix: Expose globalObject on proxyHandler [CEReactions] methods jsdom/webidl2js#163

Merged

edimitchel approved these changes Feb 10, 2020

View reviewed changes

pmdartus requested a review from domenic February 10, 2020 08:38

pmdartus and others added 5 commits February 16, 2020 16:48

Custom element implementation

fe77f7f

Address PR feedback

0022515

fix createElement post rename

f20ef94

add reference to cssstyle issue

458552a

Last (!?) fixups

925f1b0

domenic force-pushed the pmdartus/custom-element branch from a983cb3 to 925f1b0 Compare February 16, 2020 21:56

domenic approved these changes Feb 16, 2020

View reviewed changes

domenic self-assigned this Feb 16, 2020

domenic merged commit 09301da into jsdom:master Feb 16, 2020

ExE-Boss mentioned this pull request Feb 16, 2020

refactor: Rename Window.js to Window‑impl.js #2837

Closed

RyanHirsch mentioned this pull request Feb 18, 2020

First Shadow DOMd webcomponent tbremer/1990to.win#6

Merged

coryrylan mentioned this pull request Apr 23, 2020

feat(modal): core modal vmware-archive/clarity#4479

Merged

14 tasks

snuggs mentioned this pull request Jan 12, 2023

Current Status of Webcomponents devpunks/snuggsi#71

Open

		@@ -158,10 +173,16 @@ class JSDOMParse5Adapter {
		Object.assign(JSDOMParse5Adapter.prototype, serializationAdapter);

		function parseFragment(markup, contextElement) {

	if (typeof ctor !== "function" \|\| !isConstructor(ctor)) {
	if (!isConstructor(ctor)) {

Uh oh!

Conversation

pmdartus commented Apr 1, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexgwolff commented Jul 23, 2019

Uh oh!

domenic commented Jul 23, 2019

Uh oh!

marioandrest commented Jul 23, 2019

Uh oh!

tibysko commented Sep 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic commented Sep 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmdartus commented Oct 14, 2019

Uh oh!

gusnaughton commented Oct 16, 2019

Uh oh!

TimothyGu commented Oct 16, 2019

Uh oh!

pmdartus commented Oct 18, 2019

Uh oh!

pmdartus commented Oct 19, 2019

Uh oh!

thernstig commented Nov 27, 2019

Uh oh!

pmdartus commented Nov 27, 2019

Uh oh!

thernstig commented Dec 22, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domenic left a comment

ExE-Boss Jan 15, 2020 •

edited

Loading

tibysko commented Sep 29, 2019 •

edited

Loading

ExE-Boss commented Jan 30, 2020 •

edited

Loading