selection is changed very slowly by keyboard and mouse

dynamic richlistbox

(In reply to comment #0) > Created an attachment (id=254250) [details] Now this looks like a nice optimization. Thanks. A few details though: > + <field name="children">new Array()</field> This field should still be readonly="true", since we'll only want the array's content to be changed, not the array itself. > + for (var child = this.firstChild; child; child = child.nextSibling) { > + if (child instanceof > + Components.interfaces.nsIDOMXULSelectControlItemElement) > + this.children.push(child); > + } _refreshSelection can be called repeatedly (especially when using it for an RDF datasource). Won't this cause duplicates every time it's called again? Should probably go into the constructor. > + if (item instanceof > + Components.interfaces.nsIDOMXULSelectControlItemElement) { Please also make sure that the item actually belongs to this (rich)listbox (by checking the identity of either item.parentNode or item.control) - although quite unlikely, someone might have fun using a listbox inside a richlistitem. On DOMNodeRemoved it should be sufficient to make sure that index >= 0. > + if (item.previousSibling) { This will break when you insert a new item at the very start of the list where previousSibling is null (you might get away with dropping the condition and else branch altogether, since with a non-existend previousSibling you should get index==-1 which will result in the item being prepended). This will also break when the richlistbox mixes richlistitems with other elements - i.e. you'll have to iterate over the previous siblings until you find a richlistitem one. Before you do that, you'll want to make sure that the item to be inserted actually belongs to this richlistbox in the first place though.

(In reply to comment #3) > > + <field name="children">new Array()</field> > > This field should still be readonly="true", since we'll only want the array's > content to be changed, not the array itself. <field>s are just arbitrary JS properties, they can't be marked readonly (only <properties> can). Unfortunately I think we have code that makes this mistake in the tree... :(

Comment on attachment 254250 [details] [diff] [review] patch comment 3 lists some things that I noted too

We currently have issues with XBL that needs to keep track of child elements; there's a debate going on in the radiogroup version of this bug as to the best way to resolve this. In particular the use of mutation events is a problem.

(In reply to comment #6) > We currently have issues with XBL that needs to keep track of child elements; > there's a debate going on in the radiogroup version of this bug as to the best > way to resolve this. In particular the use of mutation events is a problem. > Neil, do you have in view bug 367009, right?

We should probably just wait for bug 367400

(In reply to comment #8) > We should probably just wait for bug 367400 > If mutation events will slow down the page entererly where richlistbox is used then it looks reasonable.

Comment on attachment 254250 [details] [diff] [review] patch Cancelling review as per comment #8.

I'm not going to keep this in nearest future, reassign to default assignee.