jQuery: The Write Less, Do More JavaScript Library

Ticket #1736 (new bug)

Opened 11 months ago

Last modified 7 months ago

wrap removes expandos

Reported by: Sam Assigned to: anonymous
Type: bug Priority: major
Milestone: 1.2.2 Component: core
Version: 1.2.1 Keywords:
Cc: Needs: Commit

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

1736.diff (365 bytes) - added by brandon 11 months ago.
Patch to remove clone from wrapAll

Change History

Changed 11 months ago by brandon

Patch to remove clone from wrapAll

Changed 11 months ago by brandon

  • need changed from Review to Commit

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 11 months ago by Sam

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 11 months ago by Sam

their should be there...

Changed 10 months ago by dmethvin

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 7 months ago by Sam

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).

Note: See TracTickets for help on using tickets.