Implement Element.insertAdjacentHTML()#1355
Implement Element.insertAdjacentHTML()#1355kasperisager wants to merge 11 commits intojsdom:masterfrom kasperisager:insert-adjacent-html
Conversation
Ref. #1219. Grabbed and slightly tweaked the implementation from https://github.com/assaf/zombie/blob/005b4726c9c8bd02a234cb26cb756d90358bf0a4/src/dom/jsdom_patches.js#L60-L90 A test case has been added, but both this as well as the implementation might need some more work. It's a start though!
|
Also, the WHATWG spec doesn't yet contain a definition of insertAdjacentHTML but I've added a reference to https://dom.spec.whatwg.org/#dom-element-insertadjacenthtml as I wasn't sure what else to put there (https://w3c.github.io/DOM-Parsing/#widl-Element-insertAdjacentHTML-void-DOMString-position-DOMString-text maybe?). |
|
Ah excellent! Could you enable https://github.com/w3c/web-platform-tests/blob/master/domparsing/insert-adjacent.html as well (and make sure it passes)? And probably move your test into to-upstream/domparsing. |
|
Ah, I totally missed that one! Should make my test case obsolete then. |
There was a problem hiding this comment.
You should use this.ownerDocument.createElement("template") as the container. Also you should add some tests showing how using "_" fails for elements like thead, tr, td.
There was a problem hiding this comment.
Hmm reading the spec it seems it does something a bit more complicated, e.g. defaulting to body in certain cases. I think it's OK to just use template as it will be most correct, but be careful and check browsers. If there are some edge cases that neither "template" nor "body" cover, then it's OK, we can just leave a TODO.
There was a problem hiding this comment.
Looks like "If context is null or a document, throw a DOMException with name "NoModificationAllowedError"." would be good to add a test for this too.
|
FYI the existing web platform test is failing; not sure exactly why... |
|
Read through the spec more thoroughly and re-did what I had copied from zombie. The tests pass on my end now 👍 |
|
I should note that not all conditions of step 2 are checked. |
There was a problem hiding this comment.
So as-is context is unused. I think the spec is not going to be very useful for a guidance on this since it includes "invoking the fragment parsing algorithm with text as markup, and context as the context element" which I don't think we have the ability to do (yet). I made a note of that in #1316 (comment)
Instead I think you should remove context, and just check this.parentNode in line 442. Lines 542 and 460-462 can go away.
|
Thank you very much; lovely work! Merged as 33b9341. |
|
Cheers! |
|
Looks like this should be trivial to extend to support |
|
@RReverser We would certainly appreciate a pull request doing so, assuming it passes the relevant tests. |
Ref. #1219. Grabbed and slightly tweaked the implementation from https://github.com/assaf/zombie/blob/005b4726c9c8bd02a234cb26cb756d90358bf0a4/src/dom/jsdom_patches.js#L60-L90
A test case has been added, but both this as well as the implementation might need some more work. It's a start though!