Opened 12 years ago
Closed 11 years ago
#9777 closed bug (wontfix)
.attr() changing wrong element on cloned elements (IE7 only)
| Reported by: | Owned by: | Timmy Willison | |
|---|---|---|---|
| Priority: | low | Milestone: | 1.next |
| Component: | manipulation | Version: | 1.6.2 |
| Keywords: | Cc: | ||
| Blocked by: | Blocking: |
Description
The following script highlights the differences. This works as expected in IE8 and FF, or using jQuery <= v1.5.2. It seems restricted to jQuery >= 1.6 and IE7.
Basically, on cloned elements, .attr() seems to update the wrong element.
<html>
<head>
<script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1.6.2/jquery.min.js"></script>
<script type="text/javascript">
var curItem = 0;
$(document).ready(function()
{
$("#newItem").click(function()
{
curItem++;
addItem($("#items"), curItem);
});
});
function addItem(elem, iter)
{
var lastRow = elem.find("tr:last");
var newRow = lastRow.clone(true);
newRow.find(":input").each(function()
{
var newName = $(this).attr("name").replace(/^(.*)_\d+$/, "$1_" + iter);
// THIS CHANGES THE WRONG ELEMENT
$(this).attr("name", newName);
// THIS WORKS
//this.name = newName;
$(this).val(newName);
$("#thisData").text($(this).serialize());
});
lastRow.after(newRow);
$("#formData").text($("form").serialize());
}
</script>
</head>
<body>
<form action="#" method="post">
<table id="items">
<tr>
<th>Item</th>
<td><input type="text" name="item_0" value="item_0"><td>
</tr>
</table>
<input type="button" id="newItem" value="Add Item">
</form>
<pre id="thisData"></pre>
<pre id="formData"></pre>
</body>
</html>
Change History (33)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
| Component: | unfiled → manipulation |
|---|---|
| Owner: | set to matthew.leonhardt@… |
| Priority: | undecided → low |
| Status: | new → pending |
The html shown is invalid. That td needs a closing tag.
This works fine: http://jsfiddle.net/timmywil/f5Pmg/
Let us know if that fixes the problem.
comment:3 Changed 12 years ago by
| Status: | pending → new |
|---|
Thanks for the input, but it doesn't really solve the problem. Updated here: http://jsfiddle.net/MWfvw/6/, but still fails in IE7. The real world scenario involves cloning an entire table with 50+ input fields, so the each() method is unavoidable, I think.
comment:4 Changed 12 years ago by
| Status: | new → open |
|---|
test case with cloning test case without cloning
The right name is being set, but there is some weirdness with the manipulation.
comment:6 Changed 12 years ago by
I can confirm this with another test case: http://jsfiddle.net/wYYUL/ https://gist.github.com/1077553
When replacing .attr("name") with .get(0).name (and the equivalent for setting values; i.e. directly accessing DOM element properties), everything works as expected.
comment:7 Changed 12 years ago by
| Status: | open → pending |
|---|
Can this be reduced to simple < 5 line test case?
comment:8 Changed 12 years ago by
Reduced test case: http://jsfiddle.net/mn7KG/2/
(Not quite down to five lines, but that's mostly because we don't have console.log in IE... )
Note that in regular browsers, each line being reported has one more "x" than the previous one - but in IE7, all lines except the first have the same name, suggesting the respective element (or object) is erroneously modified retroactively.
Again, when directly accessing the DOM element's .name property, the buggy behavior disappears.
comment:9 Changed 12 years ago by
| Resolution: | → invalid |
|---|---|
| Status: | pending → closed |
Because we get so many tickets, we often need to return them to the initial reporter for more information. If that person does not reply within 14 days, the ticket will automatically be closed, and that has happened in this case. If you still are interested in pursuing this issue, feel free to add a comment with the requested information and we will be happy to reopen the ticket if it is still valid. Thanks!
comment:10 Changed 12 years ago by
This issue is still relevant - see reduced test case above. Please reopen.
comment:11 Changed 12 years ago by
| Resolution: | invalid |
|---|---|
| Status: | closed → reopened |
Confirmed. That retroactive modification is bizarre. Here is a screen shot: http://imgur.com/Ztfv0
I tried changing to use .prop() and it showed strange and incorrect but different behavior.
The reason the ticket was auto-closed is that the original reporter did not reply.
comment:12 Changed 12 years ago by
| Component: | manipulation → attributes |
|---|---|
| Status: | reopened → open |
comment:13 Changed 12 years ago by
I also stumbled upon this today. Complete short test case for IE7 is already posted here:
http://stackoverflow.com/questions/7539354/jquery-1-6-4-cloning-problems-in-internet-explorer-7
comment:14 Changed 12 years ago by
| Owner: | changed from matthew.leonhardt@… to Timmy Willison |
|---|---|
| Status: | open → assigned |
comment:15 Changed 12 years ago by
It seems setting any attribute node on a clone sets all previous clones as well. It has a weird affect on the property as well.
set name => http://jsfiddle.net/timmywil/mn7KG/19/
set id => http://jsfiddle.net/timmywil/mn7KG/21/
using property => http://jsfiddle.net/timmywil/mn7KG/28/
comment:16 Changed 12 years ago by
What about a feature detection for this? That way we could create an element instead of cloning (only for cases meeting the detection condition)
comment:18 Changed 12 years ago by
Hmmm, perhaps this is happening because the attribute nodes are not being copied but instead are referenced from both elements? Also this somewhat newish version of the docs for .mergeAttributes() gives some more info on the API and its poorly explained Boolean argument.
http://msdn.microsoft.com/en-us/library/ms536614%28v=vs.85%29.aspx
Notice the docs specifically mention the NAME and ID attributes/properties which seem to be the problems.
comment:19 Changed 12 years ago by
Looking further at this issue, I tried an html5 element vs a div and found that the inner/outerHTML method for cloning does not have this issue.
http://jsfiddle.net/timmywil/mn7KG/30/
http://jsfiddle.net/timmywil/mn7KG/31/
rwaldron suggested using setAttribute, but that sets the property, which has the same issue as the third fiddle I've posted above:
http://jsfiddle.net/timmywil/mn7KG/32/
This is about the way we clone.
comment:20 Changed 12 years ago by
I'm not sure whether this is relevant (or even new), but it appears IE7's innerHTML includes DOM properties as HTML attributes: http://jsbin.com/ejurey/2
comment:21 Changed 12 years ago by
I too found this issue (affects jQuery 1.7.0 and 1.7.1 (not sure about before that)).
Here was my simple jsFiddle: http://jsfiddle.net/f2cxc/10/ the issue as far as I can tell is actually that there is no clear separation in IE7 from the original, and the cloned selection set.
My suspicion is that internally there is a reference to the old set that is not properly being broken in IE7 (e.g. this is still likely an IE7 bug under the covers)
comment:22 Changed 12 years ago by
Hi I'm "scunliffe" (I was the anonymous commenter above (didn't realize I wasn't logged in)... I ran some further tests and it appears that the behavior worked fine back in jQuery 1.5.2. I think it would be prudent to run a diff between how .clone() and/or .attr() changed between 1.5.2 and 1.6.4.
Again, here's the jsFiddle that FAILS in IE6/IE7 on jQuery 1.6.4,1.7.1 http://jsfiddle.net/f2cxc/10/
Yet with the same code WORKS fine in IE6/IE7 on jQuery 1.5.2 http://jsfiddle.net/f2cxc/12/
PS I have access to IE6/IE7/IE8 should anyone have a patch they'd like an extra set of eyes to test/check.
comment:23 Changed 12 years ago by
| Component: | attributes → manipulation |
|---|
@scunliffe: Yes, it's because we stopped using innerHTML to clone in IE and switched to cloneNode. We'd like to keep using cloneNode if we can.
comment:24 Changed 12 years ago by
Understood. Is it a matter then of determining which parts of cloneNode for IE6/IE7 needs "polyfills" of some sort to maintain compatibility?
I'm not sure if/when IE fixed their issues with cloneNode but I do recall that at one point (again due to related IE bugs with readonly attributes) the cloneNode method was effectively useless as all copies would auto-inherit the "name" attributes from their originator.
(this is an old report, but seems to suggest this might be impossible in IE6/IE7) http://webbugtrack.blogspot.com/2008/03/bug-199-cant-clone-form-element-in-ie.html
Combined with IE's cluster muck with ID's and NAME attributes sharing the same namespace I don't know if there even is a way to coerce IE into behaving using .cloneNode in IE6/IE7.
comment:26 Changed 12 years ago by
Is there a workaround in place for this? or a fix coming? If not, can we update the documentation http://docs.jquery.com/Clone to indicate that there are issues in IE (below IE8) if you plan to manipulate the cloned element(s)... specifically the name and id attributes.
comment:27 Changed 12 years ago by
I noticed the priority on this issue is low, but this change has introduced a significant bug into my codebase. Is there a possibility for a patch or workaround anytime soon?
comment:28 Changed 12 years ago by
The only way I can think of to fix this is to use innerHTML to clone all nodes in IE 6/7. That may not be possible, at least for a while. It is not an issue with attr or prop, but prop will give you slightly more consistency in the values. It will still not be entirely correct, but may be enough for regular use cases.
comment:29 Changed 12 years ago by
Also note that cloning via .innerHTML is the approach we abandoned as of version 1.5 due to its drawbacks, so unless it was some more complex hybrid of the two approaches we might just be bringing back all those older bugs.
https://github.com/jquery/jquery/commit/4fae75d575b20d887e4a273c7991c55f8821a62c
comment:30 Changed 11 years ago by
Could we raise it to medium at least? it looks like regression of the 1.7.x branch.
comment:32 Changed 11 years ago by
I would say that reverting back to copying the html is the solution. I'm using this workaround now.
comment:33 Changed 11 years ago by
That just opens up the old bugs that existed when we used to copy the html. Since then we've started supporting HTML5 elements in oldIE for example, and I suspect that wouldn't fly in the old code without additional work.
Patches welcome though.
comment:34 Changed 11 years ago by
| Resolution: | → wontfix |
|---|---|
| Status: | assigned → closed |
I don't think we're going to go back to innerHTML for cloning, which is what is required to fix this.
Ugh. Failure to follow directions. Sorry for the code post. Here's the jsFiddle link: http://jsfiddle.net/MWfvw/