Bug Tracker

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#12197 closed bug (wontfix)

.clone(true, true) does not clone DATA on text nodes

Reported by: info@… Owned by: info@…
Priority: low Milestone: 2.0
Component: manipulation Version: 1.8rc1
Keywords: Cc:
Blocked by: Blocking:

Description

As title says it all, the data is not preserved on text nodes, despite deep clone.

Below is the session from JS console.

var a=$('<p>a</p>'); //create an object with one text node
undefined
----
a.contents().data('key', 'value'); //add data to that text node
["a"]
----
a.contents().data('key'); //check presence of data, succeeds
"value"
----
var b=a.clone(true, true); //deep clone object a to b
undefined
----
b.contents().data('key');
undefined

Expected output of last query should be "value". Thanks for your interest.

Change History (13)

comment:1 Changed 7 years ago by anonymous

http://jsfiddle.net/MApSt/2/

var a=$('<p>a</p>'); //create an object with one text node
undefined

a.contents().data('key', 'value'); //add data to that text node
["a"]

a.contents().data('key'); //check presence of data, succeeds
"value"

var b=a.clone(true, true); //deep clone object a to b
undefined

b.contents().data('key');
undefined

comment:2 Changed 7 years ago by dmethvin

Owner: set to info@…
Status: newpending

Try your test on IE 6/7/8, it throws an error when trying to set data on a text node. That's why jQuery doesn't even try to do it when cloning.

The operations allowed on non-element nodes in jQuery is very limited, by design.

What exactly are you trying to do?

comment:3 Changed 7 years ago by info@…

Status: pendingnew

Oh. It is a translation script and I store some data directly to text nodes. It is cool and it works, even on very dynamic data. https://tarock.us/allow - try clicking the US flag multiple times.

For now, I am going to implement "nasty workaround". On clone, I am going to traverse complete descendant tree element by element, assigning the data from source to clone.

As far as I am concerned, this should be marked as medium priority bug. I think that if something can't be done on some weird X browser, that is not an excuse not to do it on A,B,C,D browsers. You have just shown an example - data on text nodes works on A,B,C,D and not on X.

Thank you for your time.

comment:4 Changed 7 years ago by info@…

Any updates?

And WHY did the status change from pending to new?

comment:5 Changed 7 years ago by Rick Waldron

"pending" means we're waiting for info from you; when you respond the status is changed from "pending" back to the previous status, which was "new"

comment:6 Changed 7 years ago by dmethvin

Component: unfiledmanipulation
Milestone: None2.0
Priority: undecidedlow
Status: newopen

some weird X browser

IE 6/7/8 are still something like one-third of all Internet users. If we "supported" setting data on text nodes we'd just be setting ourselves up for the inevitable bug report that it doesn't work in IE 6/7/8. In contrast, it took someone six years to report that they couldn't set data on a text node, so it's not a critical issue for too many folks.

Attaching data to text nodes cannot be done cross-browser, so it's not likely to happen until jQuery 2.0 when IE 6-8 are dropped -- assuming it works elsewhere. I'm still not convinced this is good practice; text and comment nodes are not something jQuery manipulate consistently vs. elements.

The "nasty workaround" you describe is exactly what we'll have to do inside jQuery to make this work, so if your concern is about performance it won't help to have us fix this soon anyway.

Last edited 7 years ago by dmethvin (previous) (diff)

comment:7 Changed 7 years ago by info@…

Ok, that is fine with me.

But you can do this workaround in source in O(n), while I can do it O(n²). Any plans to add callback to clone, so I could do it in O(n) like this?

$('selector').clone( function(origin, copy) {
    copy.data(origin.data());
});

comment:8 Changed 7 years ago by Rick Waldron

clone() isn't that simple—it must also handle all children of the source element as well.

comment:9 Changed 7 years ago by info@…

Oh, the callback function is ment to be called for each children/descendant separately. It enables you to do some work in paralell with origin and copy.

I might file a feature request sometime. Thanks for now!

comment:10 Changed 7 years ago by dmethvin

Resolution: wontfix
Status: openclosed

I'm not sure why I thought this would be a good idea, but I don't think it would be a good idea now. There are not a lot of good reasons to attach data to text and comment nodes.

comment:11 Changed 7 years ago by info@…

Probably because this is not an enhancement, but a defect.

It is possible for text nodes to have data, but .clone() doesn't copy it, despite that being guaranteed in the documentation.

comment:12 Changed 7 years ago by dmethvin

You're not even supposed to have text nodes in a jQuery collection, most methods don't like them. And of course oldIE won't support it. If you can point out the guarantee in the documentation we can correct it.

comment:13 Changed 6 years ago by dmethvin

#14149 is a duplicate of this ticket.

Note: See TracTickets for help on using tickets.