Opened 9 years ago
Last modified 8 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:
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 9 years ago by
Resolution: | → patcheswelcome |
---|---|
Status: | new → closed |
comment:2 follow-up: 3 Changed 9 years ago by
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 Changed 9 years ago by
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 theui-droppable
class name, right?
That would be faster, but we can't rely on the class name.
comment:4 Changed 9 years ago by
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.
comment:5 Changed 9 years ago by
Owner: | set to mikesherov |
---|---|
Status: | reopened → assigned |
comment:6 Changed 9 years ago by
Milestone: | none → 1.10.4 |
---|
comment:7 Changed 9 years ago by
Milestone: | 1.10.4 → none |
---|---|
Owner: | mikesherov deleted |
Status: | assigned → open |
Actually, as long as we have the addClasses
option, we can't rely on this.
comment:8 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
Component: | ui.draggable → ui.droppable |
---|---|
Summary: | Large number of elements in draggable panel crashes Chrome → Droppable: Large number of droppable elements causes crash |
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 theui-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.