Skip to main content

Bug Tracker

Side navigation

#1736 closed bug (patchwelcome)

Opened September 26, 2007 03:43PM UTC

Closed December 24, 2010 02:40AM UTC

Last modified March 14, 2012 03:26AM UTC

wrap removes expandos

Reported by: Sam Owned by:
Priority: low Milestone:
Component: core Version: 1.2.1
Keywords: Cc:
Blocked by: Blocking:
Description

When you create an element, add expandos to it, then use wrap, the expandos that have been added are removed. This happens with all the browsers I have tried it on.

var wrapper = document.createElement("div");
wrapper.sometext = "Some text";
wrapper.clickcss = {border: "2px solid #eee", padding: "6px"};
$("#foo").wrap(wrapper).click(
	function(e)
	{
		e.preventDefault();
		alert(this.parentNode.sometext);
		$(this.parentNode).css(this.parentNode.clickcss);
	}
);

Example

The CSS is set in Internet Explorer when jQuery 1.1.2 (the default on the example page) is used, but not when 1.2.1 is used.

The alert is 'undefined' in Firefox 2, but shows the correct text in Internet Explorer 6 and 7.

So:

Firefox: both expandos are removed

Internet Explorer: object expando (clickcss) removed, text expando still present

Attachments (1)
  • 1736.diff (0.4 KB) - added by brandon September 27, 2007 01:50PM UTC.

    Patch to remove clone from wrapAll

Change History (7)

Changed September 27, 2007 01:50PM UTC by brandon comment:1

need: ReviewCommit

The wrap method (actually wrapAll) does a clone on the supplied HTML. Doing a clone does not clone expandos and is why you are getting this error. I'm not sure what the purpose is of doing a clone ... maybe to avoid moving an existing DOM element, but who is to say I don't want to move that element. I suggest we remove the call to clone and allow the user to decide if they want to clone the element or not. However, I could be missing something simple as to why we are doing a clone. I've attached a patch to remove the call to clone and allow this code to work as expected.

Changed September 27, 2007 04:37PM UTC by Sam comment:2

I'm not entirely sure why there is a clone in their as well.

Perhaps clone can be fixed to enable expandos to be copied as well?

Changed September 27, 2007 04:38PM UTC by Sam comment:3

their should be there...

Changed September 30, 2007 09:26PM UTC by dmethvin comment:4

The .wrapAll documentation says:

This works by going through the first element provided (which is generated,
on the fly, from the provided HTML) and finds the deepest ancestor element
within its structure -- it is that element that will enwrap everything else.

This patch fixes an undocumented case; the arguments can actually be a DOM node or array-like collection of DOM nodes and I guess you want that to work.

Sam's example at http://www.texotela.co.uk/wrapexpandos.php only selects a single element #foo, but if it selected multiple elements then removing the .clone from .wrapAll would break it:

var wrapper = $("<div>").attr("special", "very")[0];
$("p").wrap(wrapper);

In FF with the patch installed, I get a message "Node cannot be inserted at this point in the hierarchy" and in IE something similar but less coherent.

It seems like the .clone was there for the benefit of .wrap and .wrapInner. Perhaps they can be implemented through .domManip with .wrapAll in the callback? Something like this works for the case above:

   wrap: function() {
        return this.domManip(arguments, false, 1, function(elem){
            jQuery( this ).wrapAll( elem );
        });
   },

This behaves differently for the currently undocumented case where multiple nodes are provided for the wrapper though; it would wrap the jQuery nodes in a successively nested wrapper for each top-level argument, with the last argument being the outermost wrapper. I think that's an equally reasonable interpretation of a weird case. I wasn't quite sure how execScripts might factor into this, that's new code since the last time I looked at .domManip.

None of this solves the problem of .clone clobbering expandos, including the "hidden" uses of clone by append/prepend/domManip/etc. when multiple nodes are selected. I don't know of any cross-browser way to enumerate custom expandos and copy them over.

Changed January 02, 2008 10:00AM UTC by Sam comment:5

This still does not work in jQuery 1.2.2b2.

While I only need to preserve expandos in Internet Explorer, it would be good if they were also preserved in Firefox and other browsers (they are removed in Firefox, regardless of which version of jQuery you use).

Changed November 17, 2010 04:31AM UTC by dmethvin comment:6

milestone: 1.2.2
priority: majorlow
status: newopen

Changed December 24, 2010 02:40AM UTC by dmethvin comment:7

resolution: → patchwelcome
status: openclosed

I guess we've been leaving this open in the hopes that some solution will arrive. However, none of the underlying DOM methods preserve properties/expandos so it's unlikely we could do it either without a lot of overhead. If anyone comes up with a bettersolution let us know and we can reopen the ticket.