#9041 closed bug (fixed)

Opened February 01, 2013 06:07AM UTC

Closed March 08, 2013 09:58PM UTC

Last modified October 15, 2014 10:29PM UTC

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:
Blocked by:		Blocking:

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

Attachments (0)

Change History (13)

Changed February 02, 2013 02:16AM UTC by marventus.wp comment:1

I was also able to recreate the issue in JSFiddle.

Changed February 02, 2013 02:38AM UTC by tj.vantoll comment:2

_comment0:	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 #8972 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. \ \ #8972 was related to floating and this one isn't although it appears the root cause is the likely the same. → 1360119455360378
status:	new → open
summary:	Unstable dragging / sorting with jQuery UI 1.9.1 and above → 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.

Changed February 06, 2013 02:55AM UTC by jeff.hagen comment:3

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

Changed February 06, 2013 04:50PM UTC by jeff.hagen comment:4

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.

Changed February 10, 2013 02:51PM UTC by marventus.wp comment:5

_comment0:	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! \ → 1360507928932310

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!

Changed February 11, 2013 02:31AM UTC by tj.vantoll comment:6

Replying to [comment:5 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.

Changed February 11, 2013 10:36PM UTC by marventus.wp comment:7

_comment0:	Hello again. \ \ '''tl;dr:''' Re-adding the old line I mentioned (4310) does not cause any serious trouble in #8572, #8573, or #8574[[BR]] \ \ '''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, revertingline 4310 to 1.9.0 leads to a more stable sorting experience overall.[[BR]] \ \ '''Refs:'''[[BR]] \ '''1.''' [http://jsfiddle.net/Marventus/ZRz5W/1/][[BR]] \ '''2.''' [http://jsfiddle.net/Marventus/ytxEG/][[BR]] \ '''3.''' [http://jsfiddle.net/Marventus/RMVpV/] → 1360622199396307
_comment1:	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[[BR]]. \ \ '''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, revertingline 4310 to 1.9.0 leads to a more stable sorting experience overall.[[BR]] \ \ '''Refs:'''[[BR]] \ '''1.''' [http://jsfiddle.net/Marventus/ZRz5W/1/][[BR]] \ '''2.''' [http://jsfiddle.net/Marventus/ytxEG/][[BR]] \ '''3.''' [http://jsfiddle.net/Marventus/RMVpV/] → 1360625685800153
_comment2:	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[[BR]]. \ \ '''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, revertingline 4310 to 1.9.0 leads to a more stable sorting experience overall.[[BR]] \ \ '''Tests:'''[[BR]] \ '''1.''' [http://jsfiddle.net/Marventus/ZRz5W/1/][[BR]] \ '''2.''' [http://jsfiddle.net/Marventus/ytxEG/][[BR]] \ '''3.''' [http://jsfiddle.net/Marventus/RMVpV/] → 1360637503487072

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/

Changed February 17, 2013 05:22AM UTC by zhizhangchen comment:8

I've sent a pull request to fix these problems:

https://github.com/jquery/jquery-ui/pull/916

Changed March 08, 2013 09:58PM UTC by John Chen comment:9

resolution:	→ fixed
status:	open → closed

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

Changeset: 07ce771a13504b851bb9f74c8ce8e960d207384a

Changed March 08, 2013 10:29PM UTC by mikesherov comment:10

milestone:	none → 1.10.2

Changed October 15, 2014 09:40PM UTC by begizi comment:11

I believe this bugfix introduced a new bug that causes the "out" event not to fire with connected lists.

Changed October 15, 2014 09:41PM UTC by begizi comment:12

I believe this bugfix introduced a new bug that causes the "out" event not to fire when using connected lists.

Changed October 15, 2014 10:29PM UTC by tj.vantoll comment:13

Replying to [comment:12 begizi]: