Conversation
domenic
left a comment
There was a problem hiding this comment.
I don't understand the reason behind this whole generic node-building/testing framework. It should be simply a matter of updating the Web IDL to do : Node, updating the impl to do extends NodeImpl, and then a few other minor tweaks.
|
|
||
| NamedNodeMap.prototype.setNamedItem = function (attr) { | ||
| if (!attrGenerated.is(attr)) { | ||
| if (!this[INTERNAL].element._ownerDocument._isNodeType(attr, NODE_TYPE.ATTRIBUTE_NODE)) { |
There was a problem hiding this comment.
Why this change? The old code should still work.
| const attribute = exports.getAttributeByNameNS(element, namespace, localName); | ||
| if (attribute === null) { | ||
| const newAttribute = attrGenerated.createImpl([], { namespace, namespacePrefix: prefix, localName, value }); | ||
| const newAttribute = element._ownerDocument._createNode(NODE_TYPE.ATTRIBUTE_NODE, [], { |
There was a problem hiding this comment.
Again, the old code should still work here. There's no advantage to using _createNode (here and below).
|
The reason was the whole cyclic-dependency problem you also ran into earlier: |
|
Oh, I see :(. OK, let me reconsider... |
|
The circular dependency problem should be gone after #2052. |
|
@TimothyGu thanks a bunch with the info on #2052 . That should help out with #1641 @Robbert what do we do next to move forward on this? |
This is my proposal for the approach to fix #1641 by extending
AttrfromNodeagain, soattr.nodeTypeexists again etcetera. snuggs requested to send a pull request so it would be easier to review my changes and judge the approach.I am aware that several unit tests are now failing, and I plan to spend time on fixing those when the general idea of this fix is okay with you all.