#9693 open bug ()

Opened December 05, 2013 07:38PM UTC

Last modified August 18, 2014 11:52PM UTC

Droppable: Large number of droppable elements causes crash

Reported by: whschultz Owned by:
Priority: minor Milestone: none
Component: ui.droppable Version: 1.10.3
Keywords: Cc:
Blocked by: Blocking:
Description

When a user clicks in the header on a draggable panel that contains a large number of items in Chrome (verified in 32.0.1700.41 beta-m Aura on Windows 7), the page stops responding, and after about ten seconds, the Chrome task crashes.

This has been narrowed down a bit in this jsfiddle:

http://jsfiddle.net/vuX59/6/

This does not crash in Firefox and appears not to crash in earlier versions of Chrome (eg 31, the current release), though the call is quite slow in all browsers.

It appears the issue (at a somewhat high level) comes down to a call to

(t.currentItem || t.element).find(":data(ui-droppable)")

which takes a while to run. I don't know enough about the internals of the jQuery code to know if this is a valid fix, but in my own code, changing it to this made the call much faster without (obviously) breaking anything:

(t.currentItem || t.element).find(".ui-droppable").filter(":data(ui-droppable)")

Obviously, since Chrome is crashing, I've reported this to the Chromium project as a defect, but I'm reporting it here, too, since it means there may be room for improvement in jQuery itself.

Attachments (0)
Change History (10)

Changed December 05, 2013 08:42PM UTC by tj.vantoll comment:1

resolution: → patcheswelcome
status: new → closed

Hi whschultz,

Thanks for taking the time to contribute. We need to use 

:data(ui-droppable)
as it finds actual droppable widgets rather than elements that happen to have the 
ui-droppable
class name. That being said, 
:data
is definitely more processing intensive and is probably leading to the issues you're seeing.

Since the only "problem" in the jQuery UI code is that it does this potentially expensive query, and since this is definitely a bit of an edge case, I'm going to close this as patcheswelcome for now. If someone can come up with a reasonable means of improving the efficiency of this query while maintaining the functionality we'd be happy to review a pull request.

If anyone else on the team disagrees please comment.

Changed December 05, 2013 09:32PM UTC by whschultz comment:2

I get that you need to find actual droppable widgets, but searching first by class name before filtering by 

:data(ui-droppable)
should give the same results (while being significantly faster on large sets), so long as users haven't removed the 
ui-droppable
class name, right?

Changed December 06, 2013 02:31AM UTC by tj.vantoll comment:3

Replying to [comment:2 whschultz]:

I get that you need to find actual droppable widgets, but searching first by class name before filtering by 
:data(ui-droppable)
should give the same results (while being significantly faster on large sets), so long as users haven't removed the 
ui-droppable
class name, right?

That would be faster, but we can't rely on the class name.

Changed December 06, 2013 12:46PM UTC by scottgonzalez comment:4

resolution: patcheswelcome
status: closed → reopened

I'm ok with relying on the class name. Classes are part of our API; they're documented, tested, and any change is treated just like a change to a JS API.

Changed December 08, 2013 01:19AM UTC by mikesherov comment:5

owner: → mikesherov
status: reopened → assigned

Changed December 08, 2013 01:19AM UTC by mikesherov comment:6

milestone: none → 1.10.4

Changed January 15, 2014 10:28AM UTC by scottgonzalez comment:7

milestone: 1.10.4 → none
owner: mikesherov
status: assigned → open

Actually, as long as we have the addClasses option, we can't rely on this.

Changed August 16, 2014 02:30AM UTC by mikesherov comment:8

Although, couldn't we see if addClasses was used, and if not, first filter by class? Also, how would this jive with the classes option we're thinking of adding?

Changed August 17, 2014 02:09PM UTC by scottgonzalez comment:9

We can do that for now. In the future, we can always rely on the class since addClasses is being removed in the rewrite and the classes option doesn't allow removing the structural classes.

Changed August 18, 2014 11:52PM UTC by mikesherov comment:10

component: ui.draggable → ui.droppable
summary: Large number of elements in draggable panel crashes Chrome → Droppable: Large number of droppable elements causes crash