Opened 6 years ago

Last modified 5 years ago

#9693 open bug

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.

Change History (10)

comment:1 Changed 6 years ago by tj.vantoll

Resolution: patcheswelcome
Status: newclosed

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.

comment:2 Changed 6 years ago by 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?

comment:3 in reply to:  2 Changed 6 years ago by tj.vantoll

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

comment:4 Changed 6 years ago by Scott González

Resolution: patcheswelcome
Status: closedreopened

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.

comment:5 Changed 6 years ago by mikesherov

Owner: set to mikesherov
Status: reopenedassigned

comment:6 Changed 6 years ago by mikesherov

Milestone: none1.10.4

comment:7 Changed 6 years ago by Scott González

Milestone: 1.10.4none
Owner: mikesherov deleted
Status: assignedopen

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

comment:8 Changed 5 years ago by mikesherov

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?

comment:9 Changed 5 years ago by Scott González

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.

comment:10 Changed 5 years ago by mikesherov

Component: ui.draggableui.droppable
Summary: Large number of elements in draggable panel crashes ChromeDroppable: Large number of droppable elements causes crash
Note: See TracTickets for help on using tickets.