Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#9362 closed defect (fixed)

dojo.NodeList.adopt() looks like broken on 1.3.0 and 1.3.1

Reported by: remy damour Owned by: Eugene Lazutkin
Priority: high Milestone: 1.4
Component: Core Version: 1.3.0
Keywords: Cc:
Blocked By: Blocking:

Description

Hi,

I'm very surprised by this bug (I hope I am not wrong) since it's on a core function.

dojo.NodeList?.adopt() works great on dojo 1.1 & 1.2 but ends up in an error with dojo 1.3.0 & 1.3.1 (retrieved through CDN).

Here is a simple test: <html>

<head>

<title>dojo.NodeList?.adopt() error</title> <script type="text/javascript" src="​http://o.aolcdn.com/dojo/1.3/dojo/dojo.xd.js"></script>

<head> <body class="tundra">

<div id="bar">bar</div> <script type="text/javascript">

dojo.addOnLoad(function() {

var n = document.createElement("div"); n.innerHTML="foo"; dojo.query("#bar").adopt(n, "last");

});

</script>

</body>

</html>

Expected result:

  • Foo node adopted into Bar node

Actual result:

  • js error in firebug: "item is not defined

​http://o.aolcdn.com/dojo/1.3/dojo/dojo.xd.js Line 16" (refering to line 6518 of uncompressed js file from nightly build, where item[0] is referenced)

Note: no error occurs if I load v 1.2 of dojo from same CDN.

Regards, Remy

Change History (12)

comment:1 Changed 11 years ago by Eugene Lazutkin

Milestone: tbd → future
Owner: changed from anonymous to Eugene Lazutkin
Status: new → assigned

comment:2 Changed 11 years ago by davidmark

Yes, it is a bug. Looks like a typo. I have this in my branch, but need to look at 1.2 to make sure the logic is right. 

var query = d.query(queryOrListOrNode); 
var item = query[0];
return query.place(item, position);	// dojo.NodeList

comment:3 Changed 11 years ago by James Burke

yeah, looks like a typo, instead of item[0], it should be this[0]: 

return d.query(queryOrListOrNode).place(this[0], position)._stash(this);

This would have been caught if we actually had a unit test for place.

comment:4 in reply to:  3 Changed 11 years ago by davidmark

Replying to jburke:

yeah, looks like a typo, instead of item[0], it should be this[0]: 

return d.query(queryOrListOrNode).place(this[0], position)._stash(this);

Thanks. I updated my branch.

This would have been caught if we actually had a unit test for place.

What I have found is that the strict check-in is too strict for some files (e.g. disallows anonymous functions that don't always return values.) Turning it off entirely with !strict allows real defects to slip through, so I've taken to using JSLint for verification.

comment:5 Changed 11 years ago by Eugene Lazutkin

Resolution: → fixed
Status: assigned → closed

(In [17932]) NodeList?: correcting the unfortunate typo, adding a test for adopt(), !strict, fixes #9362.

comment:6 Changed 11 years ago by James Burke

Thanks for the fix, elazutkin. Do you want to push it to the 1.3 branch? Seems like we should have it ready on there in case we do another 1.3.x drop.

comment:7 Changed 11 years ago by Eugene Lazutkin

(In [17938]) Removing unnecessary FIXME, !strict, refs #9362.

comment:8 Changed 11 years ago by Eugene Lazutkin

(In [17939]) NodeList?: propagating adopt() changes to 1.3, !strict, refs #9362.

comment:9 Changed 11 years ago by Eugene Lazutkin

One thing that bothers me about adopt() is that it returns its argument as a return value. It seems counterintuitive and breaks chaining (I had this problem in a real life application. 

nodes.doSmthWithNodes().adopt(children).doSmthWithNodesNotChildren();
// the last one modifies children, not nodes as expected

The situation is kind of fixed with _stash(), which is called on the argument. It appears that currently, if we are to continue chaining, we should do something like that: 

nodes.adopt(children).end().doSmthWithNodesNotChildren();

It feels awkward to me. I understand that the root of the problem is the initial decision to return children. Can we change it in 1.4? I don't think it is wildly used, especially given this ticket as the evidence.

comment:10 Changed 11 years ago by Eugene Lazutkin

Milestone: future → 1.4

comment:11 in reply to:  9 ; Changed 11 years ago by James Burke

Replying to elazutkin:

One thing that bothers me about adopt() is that it returns its argument as a return value. It seems counterintuitive and breaks chaining (I had this problem in a real life application.

Yeah, looking at adopt(), it really is like addContent but operates on a query to select the nodes for the content. So, with the latest changes in NodeList?, we could rewrite adopt() as so: 

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

It is tempting to just change addContent to accept a string query. We would have to branch on if query starts with "<" do the toDom stuff, otherwise do a query. But that makes it hard then to insert text content, so we will not do that. So the above implementation is probably good.

With the function definition above, addContent returns the regular node list, not the adopted children (what elazutkin proposed) and it will then clone the adopted nodes for all items of the current node list and insert them, vs. just adopting the nodes to the first item in the node list (what it does now by using this[0]). That is another small change to the behavior of the function, but as elazutkin pointed out, I think this may be safe given that no one complained about this initial bug earlier. And I think it fits better with the general NodeList? operations, where modifications affect all members of the NodeList?, not just the first one.

It would be good to get feedback from others on the change, if they think it affects backward compatibility, but I am favoring making the change.

comment:12 in reply to:  11 Changed 11 years ago by Eugene Lazutkin

Replying to jburke:

This task is tracked as #9406.

Note: See TracTickets for help on using tickets.