Opened 10 years ago

Closed 3 years ago

#9406 closed enhancement (patchwelcome)

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

Reported by: Eugene Lazutkin Owned by: Eugene Lazutkin
Priority: low Milestone: 1.13
Component: Core Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by 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.

Attachments (1)

place.html (2.2 KB) - added by Eugene Lazutkin 10 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 10 years ago by Eugene Lazutkin

Description: modified (diff)

comment:2 Changed 10 years ago by Eugene Lazutkin

Description: modified (diff)
Type: defectenhancement

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.

Changed 10 years ago by Eugene Lazutkin

Attachment: place.html added

comment:3 in reply to:  2 Changed 10 years ago by Eugene Lazutkin

Replying to elazutkin:

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

comment:4 in reply to:  2 Changed 10 years ago by 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?

comment:5 Changed 10 years ago by Eugene Lazutkin

Owner: changed from anonymous to Eugene Lazutkin
Status: newassigned

comment:6 Changed 7 years ago by ben hockey

Priority: highlow

comment:7 Changed 3 years ago by dylan

Milestone: future1.12
Resolution: patchwelcome
Status: assignedclosed

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

Note: See TracTickets for help on using tickets.