Skip to main content

Search and Top Navigation

#5454 closed bug (patcheswelcome)

Opened March 31, 2010 02:22AM UTC

Closed February 02, 2013 08:32PM UTC

Button Initialization Slow

Reported by: marcrohloff Owned by:
Priority: major Milestone: 1.11.0
Component: ui.button Version: 1.8
Keywords: Cc:
Blocked by: Blocking:
Description

Initializing a button (or buttonset) is very slow with a large DOM. I tracked down the issue to the following lines in the ''_determineButtonType'' function:

this.buttonElement = this.element.parents().last()
   .find( "[for=" + this.element.attr("id") + "]" );

This does an attribute based search on the whole document (parents().last() which is very slow and also seems unnecessary. IMO, This can be sped up in two ways (1) Only search under the immediate parent, (2) limit the search to labels. i.e.

this.buttonElement = this.element.parent()
   .find( "label[for=" + this.element.attr("id") + "]" );

Attachments (0)
Change History (17)

Changed April 01, 2010 12:51AM UTC by marcrohloff comment:1

Similarly, the ''radioGroup'' function also searches the whole DOM:

form = radio.form,
...
radios = $( form ).find( "[name='" + name + "']" );
} else {
radios = $( "[name='" + name + "']", ...

The following is much faster

container = $(radio).parent();
...
radios = container.find( "input[name='" + name + "']" );

Additioally, ''buttonset._create'' calls ''_init'' which results in ''_init'' running twice and

Changed May 23, 2010 12:25AM UTC by AzaToth comment:2

this wont work if you are for example in a table, and have the label in one row and the input in an other (though it might fail otherwise then as well, so it's a question what's to be allowed or not)

Changed June 02, 2010 01:23PM UTC by bungle comment:3

Great catch! I hope that this will be fixed in a upcoming release. This is especially bad on Internet Explorer. In my sample page, IE loads (and freezes) for about 6 seconds while Chrome does it in less than a second.

Changed June 10, 2010 01:49PM UTC by BStramke comment:4

For me it looks like the part described here looks like it should only be executed when the type is "checkbox" or "radio". Otherwise its using "this.buttonElement = this.element;"

(Sourcecode:)


if ( this.type === "checkbox" || this.type === "radio" ) {
   // we don't search against the document in case the element
   // is disconnected from the DOM
   this.buttonElement = this.element.parents().last()
	.find( "[for=" + this.element.attr("id") + "]" );
	this.element.addClass( "ui-helper-hidden-accessible" );

	var checked = this.element.is( ":checked" );
	if ( checked ) {
		this.buttonElement.addClass( "ui-state-active" );
	}
	this.buttonElement.attr( "aria-pressed", checked );
} else {
	this.buttonElement = this.element;
}

I found a Problem which might be very related to the one described, but at another location (line 6377), which, if you don't use a form tag, would parse the complete DOM...

_create: function() {
	this.element.closest( "form" )
		.unbind( "reset.button" )
		.bind( "reset.button", formResetHandler );

Try to add an empty <form/> Tag at the beginning of your huge DOM and report back if there was a major speedup. (this can only be a workaround)

Changed August 20, 2010 12:04PM UTC by scottgonzalez comment:5

Replying to [comment:4 BStramke]:

I found a Problem which might be very related to the one described, but at another location (line 6377), which, if you don't use a form tag, would parse the complete DOM...
> _create: function() {
> 	this.element.closest( "form" )
> 		.unbind( "reset.button" )
> 		.bind( "reset.button", formResetHandler );
> 

No, .closest('form') will return an empty set if you're not contained in a form. Using .closest() is extremely efficient here.

Changed May 12, 2011 03:37PM UTC by egret230 comment:6

This just took me days to figure out.

I've got a large DOM, with lots of buttons, and initializing them froze all browsers. Now I know why!

Here's the problem code:

if(this.type==="checkbox"||this.type==="radio"){
var b=this.element.parents().filter(":last"),c="label[for="+this.element.attr("id")+"]";this.buttonElement=b.find(c);

I changed:

var b=this.parent();

If the owners of jquery ui feel that it's important to accomodate a label for a checkbox where-ever it may be in the dom, then I'd suggest doing it in a multi-step process:

1. first look for it as an immediate sibling to the check box

2. then look in a parent table (if not found as a sibling)

3. then look in a parent form (if not found in table, or no table)

4. then go to the top of the dom as a last resort

Thank you for your consideration.

Changed May 18, 2011 04:18PM UTC by egret230 comment:7

Another option would be to add an option to the button widget.

example: labelLocation (default = null, unspecified = assumes label is a sibling element, where it "should" be)

you could then set labelLocation to:

  • tr (and it looks for the label in .closest("tr")
  • tbody (and it looks for the label in .closest("tbody")
  • table (and it looks for the label in .closest("table")
  • form (and it looks for the label in .closest("form")
  • body (and it looks for the label in .closest("body")

Or, per my previous suggestion, make the code smart and have it look for it in these places until it finds it.

But as things stand now, this is a CRITICAL FLAW for large DOMs with lots of buttons - the web page will crash having to traverse to the body and then back down to style every single button...

Changed May 22, 2011 01:46PM UTC by egret230 comment:8

By the way - I'm experiencing this bug in jQuery UI 1.8.12

Changed June 30, 2011 12:15PM UTC by ingeborgsjon comment:9

_comment0: If the function have to search the whole DOM I think this should be an optional parameter (or vice versa). As it is today vanilla jQuery UI becomes completely useless for many of us. However if you tweak the code a bit it can be used, however this means that you have to modify the code at any upgrade.1309436140173763

If the function have to search the whole DOM I think this should be an optional parameter (or vice versa). As it is today vanilla jQuery UI becomes completely useless for many of us. However if you tweak the code a bit it can be used, but this also means that you have to modify the code at any upgrade.

Changed January 23, 2012 12:37AM UTC by mvandiest comment:10

The usage of '.closest("form")' in my very large page is killing render times in IE. Every button instantiation walks the entire DOM to find the one form element thousands of elements above.

A default option of form: 'form' could be used to support this, but currently the use of .closest has a very specific behavior for the button and doesn't follow the selector pattern similar to the autocomplete 'appendTo' option where it is a simple selector.

Going to continue to try and find a way to supply a 'form' selector option and support the current .closest behavior by default.

Changed January 23, 2012 01:57AM UTC by scottgonzalez comment:11

@mvandiest There should never be a form option. This is extremely different than autocomplete's appendTo option. You cannot optionally change the behavior of an HTML form.

Changed January 23, 2012 01:59AM UTC by scottgonzalez comment:12

We can easily improve .closest( "form" ) with [0].form.

Changed February 27, 2012 12:03PM UTC by jzaefferer comment:13

milestone: TBD1.9
status: newopen

This look like we should address is ASAP. The rewrite will likely use the same approach, so no need to wait for that.

Changed October 11, 2012 02:43PM UTC by scottgonzalez comment:14

milestone: 1.9.01.11.0

Changed October 16, 2012 09:38PM UTC by fontzter comment:15

tried applying the following code to check prev and next elements first. It had negligible benefit.

ancestor = this.element.prev().add( this.element.next() );
labelSelector = "label[for='" + this.element.attr("id") + "']";

this.buttonElement = ancestor.filter( labelSelector );

if ( !this.buttonElement.length ) {
	ancestor = this.element.parents().last();
	this.buttonElement = ancestor.filter( labelSelector );
	if ( !this.buttonElement.length ) {
		ancestor = ancestor.length ? ancestor.siblings() : this.element.siblings();
		this.buttonElement = ancestor.filter( labelSelector );
		if ( !this.buttonElement.length ) {
			this.buttonElement = ancestor.find( labelSelector );
		}
	}
}

i created a fiddle to test this here: http://jsfiddle.net/fontzter/7jgvW/2/

Changed February 02, 2013 04:31PM UTC by djQuery comment:16

_comment0: http://jsperf.com/label-selector-test \ \ After running the above test I don't see that making any changes would improve the performance significantly. \ $('#container').find('label').filter('[for="rb-250-2"]'); Runs fastest on most modern browsers but slower in others. \ \ 1359831401406868

http://jsperf.com/label-selector-test/1

http://jsperf.com/label-selector-test/2

After running the above test I don't see that making any changes would improve the performance significantly.

$('#container').find('label').filter('[for="rb-250-2"]'); Runs fastest on most modern browsers but slower in others.

Changed February 02, 2013 08:32PM UTC by scottgonzalez comment:17

resolution: → patcheswelcome
status: openclosed