Ticket #9041 (closed bug: fixed)

Opened 18 months ago

Last modified 17 months ago

Sortable: Unstable dragging / sorting with jQuery UI 1.9.1 and above

Reported by: marventus.wp Owned by:
Priority: minor Milestone: 1.10.2
Component: ui.sortable Version: 1.9.1
Keywords: Cc:
Blocking: Blocked by:

Description

There seems to be a bug involving the sorting of elements in jQuery UI > 1.9.0 (affected versions: 1.9.1, 1.9.2, 1.10.0). Dragging and sorting are very jerky and unstable in source container are incredibly hard to resort after original sorting in target container.
I created a small demo tester to try to narrow down the problem:  http://elblawg.com/jqueryui-test/.

That's how I was able to determine the issue starts occurring with 1.9.1 and above. The offending line is 4310 of the full 1.10.0 version (ui-sortable section). Here is the diff I created:

--- C:/xampp/htdocs/blogs/elblawg/jqueryui-test/js/jquery-ui-1.10.0.js	vie feb  1 01:55:45 2013
+++ C:/xampp/htdocs/blogs/elblawg/jqueryui-test/js/jquery-ui-1.10.0_2.js	vie feb  1 01:55:31 2013
@@ -4307,7 +4307,7 @@
 		if(this.containers.length === 1) {
 			this.containers[innermostIndex]._trigger("over", event, this._uiHash(this));
 			this.containers[innermostIndex].containerCache.over = 1;
-		} else {
+		} else if(this.currentContainer != this.containers[innermostIndex]) {
 
 			//When entering a new container, we will find the item with the least distance and append our item near it
 			dist = 10000;

LMK if you need further assistance.

Marv

Change History

comment:1 Changed 18 months ago by marventus.wp

I was also able to recreate the issue in  JSFiddle.

comment:2 Changed 18 months ago by tj.vantoll

  • Status changed from new to open
  • Summary changed from Unstable dragging / sorting with jQuery UI 1.9.1 and above to Sortable: Unstable dragging / sorting with jQuery UI 1.9.1 and above

I can see the issue as well, here's a fiddle you can play with the version numbers in  http://jsfiddle.net/tj_vantoll/FeTr2/.

This broke with this commit that landed in 1.9.1:  https://github.com/jquery/jquery-ui/commit/bae06d2b1ef6bbc946dce9fae91f68cc41abccda#ui/jquery.ui.sortable.js. That same commit is also blamed for #8792 with had a pull that attempted to re-add the same line you're trying to add, but unfortunately re-adding that line causes other regressions.

#8792 was related to floating and this one isn't although it appears the root cause is the likely the same.

Last edited 18 months ago by tj.vantoll (previous) (diff)

comment:3 Changed 18 months ago by jeff.hagen

I also have the same issue. I've recreated the issue here:  http://jsfiddle.net/2rt9Z/ I had to downgrade to 1.9.0.

comment:4 Changed 18 months ago by jeff.hagen

This is also the cause of another issue. As of version 1.9.1, the over event fires on every pixel movement. I've recreated the issue at  http://jsfiddle.net/PZB5x/. Watch the console output.

comment:5 follow-up: ↓ 6 Changed 18 months ago by marventus.wp

Hello again. Ok, so it wasn't just me then, ;-P

@tj.vantoll: I understand what you mean. Could you provide more information on the other regressions you noticed? I tested the fix and it didn't seem to generate any issues, but, of course, it is impossible to contemplate every possible scenario. Thanks!

Last edited 18 months ago by marventus.wp (previous) (diff)

comment:6 in reply to: ↑ 5 Changed 18 months ago by tj.vantoll

Replying to marventus.wp:

Hello again. Ok, so it wasn't just me then, ;-P

@tj.vantoll: I understand what you mean. Could you provide more information on the other regressions you noticed? I tested the fix and it didn't seem to generate any issues, but, of course, it is impossible to contemplate every possible scenario. Thanks!

The commit that caused this issue was a fix for #8572, #8573, and #8574. What we really need are tests that show that re-adding that line fixes this issue, and doesn't cause any regressions with #8572 / #8573 / #8574.

comment:7 Changed 18 months ago by marventus.wp

Hello again.

tl;dr: Re-adding the old line I mentioned (4310) does not seem to cause any serious trouble in #8572, #8573, or #8574
.

Detailed: I ran some tests on JsFiddle based on the tickets you provided and the demos created by @zhizhangchen and the proposed fix does not seem to revert sorting action to any of the situations described in any of them. I did notice that, in respect to #8572, the placeholder takes a while before "coming down" to a new position, but sorting works well regardless of that. IMHO, restoring line 4310 the way it was in 1.9.0 leads to a more stable sorting experience overall.

Tests:
1.  http://jsfiddle.net/Marventus/ZRz5W/1/
2.  http://jsfiddle.net/Marventus/ytxEG/
3.  http://jsfiddle.net/Marventus/RMVpV/

Last edited 18 months ago by marventus.wp (previous) (diff)

comment:8 Changed 18 months ago by zhizhangchen

I've sent a pull request to fix these problems:  https://github.com/jquery/jquery-ui/pull/916

comment:9 Changed 17 months ago by John Chen

  • Status changed from open to closed
  • Resolution set to fixed

Sortable: Skip triggering over event if it's alreay over the continer. Fixes #9041: the over event fires on every pixel movement

Changeset: 07ce771a13504b851bb9f74c8ce8e960d207384a

comment:10 Changed 17 months ago by mikesherov

  • Milestone changed from none to 1.10.2
Note: See TracTickets for help on using tickets.