Ticket #5454 (closed bug: patcheswelcome)

Opened 5 years ago

Last modified 20 months ago

Button Initialization Slow

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

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

Change History

comment:1 Changed 5 years ago by marcrohloff

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

comment:2 Changed 4 years ago by AzaToth

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)

comment:3 Changed 4 years ago by bungle

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.

comment:4 follow-up: ↓ 5 Changed 4 years ago by BStramke

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)

comment:5 in reply to: ↑ 4 Changed 4 years ago by scott.gonzalez

Replying to 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.

comment:6 Changed 3 years ago by egret230

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.

comment:7 Changed 3 years ago by egret230

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

comment:8 Changed 3 years ago by egret230

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

comment:9 Changed 3 years ago by ingeborgsjon

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.

Last edited 3 years ago by ingeborgsjon (previous) (diff)

comment:10 Changed 3 years ago by mvandiest

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.

comment:11 Changed 3 years ago by scott.gonzalez

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

comment:12 Changed 3 years ago by scott.gonzalez

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

comment:13 Changed 3 years ago by joern.zaefferer

  • Status changed from new to open
  • Milestone changed from TBD to 1.9

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

comment:14 Changed 2 years ago by scott.gonzalez

  • Milestone changed from 1.9.0 to 1.11.0

comment:15 Changed 2 years ago by fontzter

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/

comment:16 Changed 20 months ago by djQuery

 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.

Last edited 20 months ago by djQuery (previous) (diff)

comment:17 Changed 20 months ago by scott.gonzalez

  • Status changed from open to closed
  • Resolution set to patcheswelcome
Note: See TracTickets for help on using tickets.