Skip to main content

Bug Tracker

Side navigation

#1143 closed bug (fixed)

Opened April 25, 2007 09:08AM UTC

Closed September 15, 2007 01:28PM UTC

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

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)
  • html.diff (0.4 KB) - added by brandon May 30, 2007 11:48PM UTC.

    Patch for html

  • uniquebug.htm (0.7 KB) - added by vmx May 24, 2007 04:17PM UTC.

    Example for the bugfix

  • uniquebug.patch (1.4 KB) - added by vmx May 24, 2007 04:17PM UTC.

    Patch for 1.1.3a

  • uniquebugv2.patch (1.2 KB) - added by vmx May 25, 2007 10:50AM UTC.

    New version that passes the test suite

  • uniquebugv3.patch (0.8 KB) - added by vmx June 07, 2007 12:10PM UTC.

    Faster version

Change History (15)

Changed April 25, 2007 05:18PM UTC by brandon comment:1

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

Changed May 10, 2007 09:40AM UTC by jf.hovinne comment:2

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.

Changed May 23, 2007 09:02PM UTC by vmx comment:3

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");

Changed May 23, 2007 09:16PM UTC by vmx comment:4

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 May 24, 2007 04:18PM UTC by vmx comment:5

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.

Changed May 24, 2007 07:56PM UTC by jf.hovinne comment:6

I've applied the patch and tested with MSIE, FF (Win + Linux), Opera.

Works fine for me!

Test page.

Changed May 25, 2007 10:54AM UTC by vmx comment:7

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.

Changed May 25, 2007 12:13PM UTC by vmx comment:8

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 May 30, 2007 11:52PM UTC by brandon comment:9

description: 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 [http://dev.wymeditor.org/wymeditor/tags/0.3-a1/advanced.html 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.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 [http://dev.wymeditor.org/wymeditor/tags/0.3-a1/advanced.html 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.

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.

Changed May 31, 2007 08:30AM UTC by jf.hovinne comment:10

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 June 07, 2007 12:15PM UTC by vmx comment:11

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.

Changed June 23, 2007 12:41PM UTC by vmx comment:12

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 ); 
	},

Changed August 10, 2007 06:58PM UTC by john comment:13

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.

Changed August 10, 2007 06:59PM UTC by john comment:14

Oops, that should be:

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

Changed September 15, 2007 01:28PM UTC by john comment:15

milestone: 1.1.41.2
resolution: → fixed
status: newclosed
version: 1.1.31.1.4

This has been resolved in jQuery 1.2.