Opened 12 years ago

Closed 11 years ago

#6524 closed defect (fixed)

Dialog: show() is slow for dialogs with many nested elements

Reported by: guest Owned by: bill
Priority: high Milestone: 1.2
Component: Dijit Version: 1.1.0
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

Problem Description: The new feature of dijit.Dialog that focuses the first focusable element on the dialog performs poor in cases where the dialog contains a large set of nested HTML elements. In our case, we have a dialog that has two selectboxes with a couple (~>100 + ~>350) of <option> elements. Dialog.show() takes about 2.5 seconds on our machine (P4 2.6GhZ, FF 2.0.13) in this case. One could argue that we should use dynamic reloadable dropdown boxes instead but this is currently not possible due to other constraints. However, I see some room for improvements in the current dijit code which could solve our problem. I put a timestamp debug log statement into the show() method of the dijit.Dialog to measure possible performance improvements. Here are my results.

1.Without improvement: 2384.2 ms (FF2), 4143.8ms (IE6) (average of 5 tries)

  1. Changed dijit._DialogMixin._getFocusItems to call dijit._getTabNavigable directly instead of calling it implicitely twice due to the

calls to getFirstInTabbingOrder/getLastInTabbingOrder:

_getFocusItems: function(/*Node*/ dialogNode){
			// find focusable Items each time a dialog is opened
			
			var elems = dijit._getTabNavigable(dojo.byId(dialogNode));
			//var focusItem = dijit.getFirstInTabbingOrder(dialogNode);
			var focusItem = elems.lowest ? elems.lowest : elems.first;
			this._firstFocusItem = focusItem ? focusItem : dialogNode;
			//focusItem = dijit.getLastInTabbingOrder(dialogNode);
			focusItem = elems.last ? elems.last : elems.highest;
			this._lastFocusItem = focusItem ? focusItem : this._firstFocusItem;
			if(dojo.isMoz && this._firstFocusItem.tagName.toLowerCase() == "input" && dojo.attr(this._firstFocusItem, "type").toLowerCase() == "file"){
					//FF doesn't behave well when first element is input type=file, set first focusable to dialog container
					dojo.attr(dialogNode, "tabindex", "0");
					this._firstFocusItem = dialogNode;
			}
		}

Result: 1502.8 ms (FF2), 2559.6 ms (IE6) (average of 5 tries)

  1. additionally change dijit._getTabNavigable to explicitely exclude "SELECT" children from recursive walkTree() calls

simply changing:

if(isShown){ walkTree(child) }

to

if(isShown && child.nodeName.toUpperCase() != 'SELECT'){ walkTree(child) }

Result: 669 ms (FF2), 549.8 ms (IE6) (average of 5 tries)

So the question is: could this changes be incorporated into dijit or is did I oversee something which makes this impossible. Speaking for our project, we cannot live without this improvement and will stick to the custom hacked code until an officially improved release.

Regards, Philipp

PS: I am attaching the changed dijit classes hoping this makes your life a bit easier...

Attachments (1)

dijit_dialog_performance.txt (3.2 KB) - added by guest 12 years ago.
complete changes to classes

Download all attachments as: .zip

Change History (8)

Changed 12 years ago by guest

complete changes to classes

comment:1 Changed 12 years ago by guest

One additional remark: Using the "tab" key to navigate between fields in a dialog is very slow in V1.1 because of the _getFocusItems performance. You can see this even in the current Dijit Dialog testcase. The onKey handler executes _getFocusItems each time the tab key is pressed which really heats up the client CPU already in the small test Dialog. Doing this in our case would mean that the user would have to wait 2.5sec on FF2 or even 4.5 sec on IE6 after each tab- key until the next field is focused.

comment:2 Changed 12 years ago by bill

Summary: Dialog.show is slow for dialogs with many nested elementsDialog: show() is slow for dialogs with many nested elements

Hi Philipp, thanks for working on this. It sounds like your changes make sense, both for initially showing the Dialog and for tabbing between stuff, but can you file a CLA so we can merge them in? Dojo is really strict about that.

comment:3 in reply to:  2 Changed 12 years ago by guest

Replying to bill:

Hi Philipp, thanks for working on this. It sounds like your changes make sense, both for initially showing the Dialog and for tabbing between stuff, but can you file a CLA so we can merge them in? Dojo is really strict about that.

Hi Bill,

I am an IBM employee, do I still need to sign a CLA or am I automatically running on the one of IBM? Otherwise I will sign my own ;-)

regards, Philipp

comment:4 Changed 12 years ago by bill

Milestone: 1.2
Owner: set to bill

Ah good, they have a CCLA so that's enough.

comment:5 Changed 11 years ago by bill

Description: modified (diff)
Status: newassigned

comment:6 Changed 11 years ago by bill

OK Philipp, thanks for the patch, I'll check it in!

comment:7 Changed 11 years ago by bill

Resolution: fixed
Status: assignedclosed

(In [14581]) Fixes #6524: performance enhancements for finding focusable element when you open a dialog. !strict

Note: See TracTickets for help on using tickets.