Modify NodeList.adopt() to be more in line with the rest of NodeList methods.

[Eugene Lazutkin]

In #9362 jburke proposes to change NodeList.adopt() like that:

adopt: function(/*String||Array||DomNode*/ queryOrListOrNode, /*String?*/ position){
  return this.addContent(d.query(queryOrListOrNode), position);
}

It introduces two changes:

1. This way it returns the main NodeList as other methods.
2. It copies the NodeList argument, instead of moving it.

[Eugene Lazutkin]

This change effectively aliases adopt to addContent:

dojo.NodeList.adopt = dojo.NodeList.addContent;

While it is laudable, because it reduces the codebase, I am not so sure about James' point number 2: copying DOM nodes instead of moving them. Without this functionality there is no need for adopt(). We can preserve it by moving the functionality in another direction: NodeList.place():

adopt: function(/*String||Array||DomNode*/ queryOrListOrNode, /*String?*/ position){
  d.query(queryOrNode).place(this[0], position);
  return this; // NodeList
}

Which reminds me of another deficiency in place(): it behaves strangely with different position arguments. Intuitively I expect that all elements will be inserted as one fragment in a position of my choosing. In practice:

first

Inserts all elements in the beginning of the child list reversing their order.

only

Inserts the last element of the list as the only child.

last

Behaves as I expect it.

before

Behaves as I expect it.

replace

Throws if the list contains more than 1 element.

after

Inserts all elements after the current element reversing their order.

I cannot believe that this is the intentional behavior. The test file is included.

[Eugene Lazutkin]

Replying to elazutkin:

Obviously I meant the aliasing metaphorically as "the same effect".

[James Burke]

Replying to elazutkin:

This change effectively aliases adopt to addContent:

dojo.NodeList.adopt = dojo.NodeList.addContent;

While it is laudable, because it reduces the codebase, I am not so sure about James' point number 2: copying DOM nodes instead of moving them. Without this functionality there is no need for adopt().

Well there are some slight differences, addContent cannot take a string that represents a DOM query, because of the possible conflict with the desire to insert a text node. I think you know that, based on your last comment as meaning metaphorically aliasing, but just making it explicit.

As to the copy of nodes -- it would only clone nodes if the current NodeList? had more than one node, otherwise it just does a move. So I think for most cases where adopt would be used now, there would be no functional difference -- if the node list has only one node in it, the adoption moves the new nodes under the only node in the NodeList?.

I believe NodeList? operations are group operations, so adding/moving content should work if you have multiple nodes in the current NodeList?. If just one node in the current node list, the new nodes should just be moved/inserted. However, if more than one node in the current node list, the first node gets the original new nodes, the other nodes get clones of the new nodes.

For what it is worth, I believe this is also necessary for jquery compatibility, and the original reason I used that approach for the new NodeList? mixins I added. I also think it makes sense given that the modify operations for a NodeList? should operate in some way on all the nodes.

You can see this approach used for methods that use NodeList?._place. Methods that use NodeList?._place moves/uses the new nodes directly if just one element in the current NodeList?, but specify a cloneNode for nodes in the current NodeList? at index greater than 0.

Which reminds me of another deficiency in place(): it behaves strangely with different position arguments. Intuitively I expect that all elements will be inserted as one fragment in a position of my choosing. In practice:

I believe the private dojo._place does the right thing with the nodes. I want to switch NodeList?.place over to use NodeList?._place internally to fix the problem you mention and so we get the proper "move for first node, cloneNode for rest" behavior described above. So it might look like this:

place: function(/*String||Node||NodeList*/ queryOrNode, /*String*/ position){
    var content = typeof queryOrNode == "string" || queryOrNode.nodeType ? dojo.query(queryOrNode) : queryOrNode;
    for(var i = 0, node; node = this[i]; i++){
        this._place(content, node, position, i > 0);
    }
    return this;
},

adopt: function(/*String||Array||DomNode*/ queryOrListOrNode, /*String?*/ position){
  d.query(queryOrNode).place(this, position);
  return this; // NodeList
},

Right now I favor those two implementations of place and adopt. What do you think?

[dylan]

Given that no one has shown interest in creating a patch in the past 5+ years, I'm closing this as patchwelcome.