#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 12 years ago by
Milestone: | tbd → future |
---|---|
Owner: | changed from anonymous to Eugene Lazutkin |
Status: | new → assigned |
comment:2 Changed 12 years ago by
comment:3 follow-up: 4 Changed 12 years ago by
comment:4 Changed 12 years ago by
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:6 Changed 12 years ago by
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:8 Changed 12 years ago by
comment:9 follow-up: 11 Changed 12 years ago by
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 12 years ago by
Milestone: | future → 1.4 |
---|
comment:11 follow-up: 12 Changed 12 years ago by
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.
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.