Bug Tracker

Opened 13 years ago

Closed 12 years ago

#1143 closed bug (fixed)

jQuery adds mergenum attribute on matched elements in MSIE

Reported by: jf.hovinne Owned by:
Priority: major Milestone: 1.2
Component: core Version: 1.1.4
Keywords: mergenum Cc:
Blocked by: Blocking:

Description (last modified by brandon)

Using latest revision (r1775), the following code seems to add a 'mergenum' attribute to each matched element in MSIE:

$j(this._doc.body).find("*").bind("mouseup", this.mouseup);

If I comment this line, the attribute isn't added. If I comment the whole mouseup function, the attribute is added, so I guess the problem comes from $.find().

Same problem with the following:

$j(this._doc.body).find("*").show();

Note: this._doc refers to an iframe document.

An example is available here, when you click on the 'HTML' button to display the editor's HTML content.

The above code is located at line 848 in the file wymeditor/jquery.wymeditor.js.

Attachments (5)

uniquebug.patch (1.4 KB) - added by vmx 13 years ago.
Patch for 1.1.3a
uniquebug.htm (671 bytes) - added by vmx 13 years ago.
Example for the bugfix
uniquebugv2.patch (1.2 KB) - added by vmx 13 years ago.
New version that passes the test suite
html.diff (421 bytes) - added by brandon 13 years ago.
Patch for html
uniquebugv3.patch (788 bytes) - added by vmx 13 years ago.
Faster version

Download all attachments as: .zip

Change History (20)

comment:1 Changed 13 years ago by brandon

Interesting. The offending code can be found in the jQuery.unique method. I wonder if changing the name of the expando to $mergeNum or just something else in general would fix this. We use expandos with events also ($events) but I do not see this behavior with those.

Could you try replacing your jquery.js with this one: http://brandon.jquery.com/testing/1143/jquery.lite.js

Feel free to ping me on the results: brandon . aaron (at) gmail . com

comment:2 Changed 13 years ago by jf.hovinne

Replacing the name doesn't fix the issue. On the other hand, if I comment out line 612:

first[i].mergeNum = num;

in jquery.lite.js, function unique(), jQuery works fine, and the mergenum attribute isn't added.

comment:3 Changed 13 years ago by vmx

The problem is that Internet Explorer adds every custom property (like "mergeNum") as attribute to a DOM node if you use "innerHTML". The only solution I found was removing the property from the nodes after you found the uniques. Put the following two lines of code in the unique function just before "r" gets returned.

for ( var i = 0, rl = r.length; i < rl; i++ )
	r[i].removeAttribute("mergeNum");

comment:4 Changed 13 years ago by vmx

I should have read the discussion about it in google groups before I post that fix. It is undesired to have an extra loop through the array as it would be a major slowdown for almost all selectors. As this is only a problem when you use innerHTML in Internet Explorer a possible solution might be to remove all properties starting with "$" within the "html" function (where innerHTML is used). But this would mean a major slowdown for innerHTML, as you would need to travers recursively through all nodes.

Changed 13 years ago by vmx

Attachment: uniquebug.patch added

Patch for 1.1.3a

Changed 13 years ago by vmx

Attachment: uniquebug.htm added

Example for the bugfix

comment:5 Changed 13 years ago by vmx

I've thought about a way to add/remove the mergeNum property without an extra loop. This is my solution (it is only needed in onw specific merge() call). It adds a $merge=true propery to all elements that will be tested for uniqueness. If an element in unique() gets added, $merge is removed. This way unique() should work as expected. I'll attach a patch a against 1.1.3a and a small example where you can see the bug fix.

comment:6 Changed 13 years ago by jf.hovinne

I've applied the patch and tested with MSIE, FF (Win + Linux), Opera.
Works fine for me!

Test page.

Changed 13 years ago by vmx

Attachment: uniquebugv2.patch added

New version that passes the test suite

comment:7 Changed 13 years ago by vmx

The new version of the patch should pass the test suite. Warning: All selector will get slower (about 10%) with that patch.

The better solution to fix that bug will be removing the mergeNum property when you serialize with html(). So there will be only a slowdown on html() when it is used with Internet Explorer.

comment:8 Changed 13 years ago by vmx

To replace all mergeNum properties in Internet Explorer, you can replace the html() with this one.

html: function( val ) {
	if  (val == undefined ) {
		if ( this.length ) {
			if ( jQuery.browser.msie ) {
				var nodes = this[0].getElementsByTagName("*");
				for ( var i = 0; nodes[i]; i++)
					nodes[i].removeAttribute("mergeNum");
			}
			return this[0].innerHTML;
		}
	}
	else
		return this.empty().append( val );
},

I hope it can be optimized to a smaller file size, as it currently needs additional 64 bytes (packed).

Changed 13 years ago by brandon

Attachment: html.diff added

Patch for html

comment:9 Changed 13 years ago by brandon

Description: modified (diff)

Thanks vmx for the heavy lifting. I've attached a patch that fixes up html in a little more jQuery-ish way.

Let me know if this works and I'll commit it.

comment:10 Changed 13 years ago by jf.hovinne

I think the real problem actually resides in MSIE + designMode + expandos.
A test page is available, showing that extra DIVs get added while e.g. using execCommand. Messing up the DOM with these extra DIVs is unacceptable, since we try to produce clean strict XHTML. We could try to handle the issue by removing the extra nodes, but I guess other unexpected problems will appear.

Removing mergeNum in html() is an improvement, but I think that the only solution for MSIE + designMode + jQuery to correctly work (if jQuery still uses first[i].mergeNum = num; in unique()) is to avoid using jQuery.find() on the iframe nodes.
Current WYMeditor trunk already uses this workaround, so we can make a release very soon.

Changed 13 years ago by vmx

Attachment: uniquebugv3.patch added

Faster version

comment:11 Changed 13 years ago by vmx

Brandons patch won't work, as it doesn't travers all node recursively.

I've attached a new version of my uniquebug patch. It is faster than the previous one. Internet Explorer adds custom properties as attrbutes to tags, but only if they aren't an array. This patch takes advantage of that fact and encapsulates mergeNum into an array.

comment:12 Changed 12 years ago by vmx

Patching html() seems to be the way to go. Although comment:8 should be a very fast way to get it work it is neither very jQuery-ish, nor small in file size. Here are 2 propositions for a bugfix. The first one is from John, the second one from me. My version is approximately 3 times faster (tested with Firefox, Internet Explorer, Opera).

html: function( val ) { 
	return val == undefined ? 
		( this.length ? $(this[0]).find("*").removeAttr("mergeNum").end()[0].innerHTML : null ) : 
		this.empty().append( val ); 
	},
html: function( val ) { 
	return val == undefined ? 
		( this.length ? $(this[0]).find("*").each(function(){this.removeAttribute("mergeNum")}).end()[0].innerHTML : null ) : 
		this.empty().append( val ); 
	},

comment:13 Changed 12 years ago by john

Milestone: 1.1.31.1.4
Version: 1.1.21.1.3

Two possible solutions:

A plugin that you call after you run your selectors to clean everything up:

jQuery.fn.cleanup = function(){
  return this.find("*").removeAttr("mergeNum").end();
};

or extend and overwrite jQuery.unique to force it to clean up:

var _unique = jQuery.unique;
jQuery.unique = function(a){
  var ret = jQuery.unique(a);
  jQuery(a).removeAttr("mergeNum");
  return ret;
};

Simply due to the speed hit on this, we're not planning on fixing this.

comment:14 Changed 12 years ago by john

Oops, that should be:

var _unique = jQuery.unique;
jQuery.unique = function(a){
  var ret = jQuery._unique(a);
  jQuery(a).removeAttr("mergeNum");
  return ret;
};

comment:15 Changed 12 years ago by john

Milestone: 1.1.41.2
Resolution: fixed
Status: newclosed
Version: 1.1.31.1.4

This has been resolved in jQuery 1.2.

Note: See TracTickets for help on using tickets.