Bug Tracker

Ticket #9777 (closed bug: wontfix)

Opened 3 years ago

Last modified 2 years ago

.attr() changing wrong element on cloned elements (IE7 only)

Reported by: matthew.leonhardt@… Owned by: timmywil
Priority: low Milestone: 1.next
Component: manipulation Version: 1.6.2
Keywords: Cc:
Blocking: Blocked by:

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

comment:1 Changed 3 years ago by anonymous

Ugh. Failure to follow directions. Sorry for the code post. Here's the jsFiddle link:  http://jsfiddle.net/MWfvw/

comment:2 Changed 3 years ago by timmywil

  • Owner set to matthew.leonhardt@…
  • Priority changed from undecided to low
  • Status changed from new to pending
  • Component changed from unfiled to manipulation

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 3 years ago by matthew.leonhardt@…

  • Status changed from pending to 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 3 years ago by timmywil

  • Status changed from new to open

 test case with cloning  test case without cloning

The right name is being set, but there is some weirdness with the manipulation.

Last edited 3 years ago by timmywil (previous) (diff)

comment:5 Changed 3 years ago by timmywil

comment:6 Changed 3 years ago by FND

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.

Last edited 3 years ago by FND (previous) (diff)

comment:7 Changed 3 years ago by rwaldron

  • Status changed from open to pending

Can this be reduced to simple < 5 line test case?

comment:8 Changed 3 years ago by FND

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.

Last edited 3 years ago by FND (previous) (diff)

comment:9 Changed 3 years ago by trac-o-bot

  • Status changed from pending to closed
  • Resolution set to invalid

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 3 years ago by FND

This issue is still relevant - see reduced test case above. Please reopen.

comment:11 Changed 3 years ago by dmethvin

  • Status changed from closed to reopened
  • Resolution invalid deleted

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 3 years ago by dmethvin

  • Status changed from reopened to open
  • Component changed from manipulation to attributes

comment:13 Changed 3 years ago by Martin

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 3 years ago by rwaldron

  • Owner changed from matthew.leonhardt@… to timmywil
  • Status changed from open to assigned

comment:15 Changed 3 years ago by timmywil

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/

Last edited 3 years ago by timmywil (previous) (diff)

comment:16 Changed 3 years ago by rwaldron

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:17 Changed 3 years ago by timmywil

#10453 is a duplicate of this ticket.

comment:18 Changed 3 years ago by dmethvin

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 3 years ago by timmywil

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 3 years ago by FND

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 3 years ago by anonymous

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 3 years ago by scunliffe

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 3 years ago by timmywil

  • Component changed from attributes to 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 3 years ago by scunliffe

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 3 years ago by Stephen Cunliffe <stephen.cunliffe@…>

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 2 years ago by teresa@…

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 2 years ago by timmywil

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 2 years ago by dmethvin

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 2 years ago by regisbsbittencourt@…

Could we raise it to medium at least? it looks like regression of the 1.7.x branch.

comment:31 Changed 2 years ago by dmethvin

The priority DOESN'T MATTER if nobody has a solution.

comment:32 Changed 2 years ago by anonymous

I would say that reverting back to copying the html is the solution. I'm using this workaround now.

comment:33 Changed 2 years ago by dmethvin

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 2 years ago by timmywil

  • Status changed from assigned to closed
  • Resolution set to wontfix

I don't think we're going to go back to innerHTML for cloning, which is what is required to fix this.

Note: See TracTickets for help on using tickets.