Buggy Collision Detection UI-Sortable

Hello,

This is my first bug report to any project really. I've read your guidelines but please provide any guidance and feedback so that I can do better in the future. If you have any questions at the end of this then please let me know. I can submit a video if it's really needed, but the bug should be easy to reproduce.

BUG DESCRIPTION

JS fiddle: http://jsfiddle.net/AzuObs/cp8c6xyf/.

Imgur: http://imgur.com/fHghiK9

The ui-sortable-helper does not detect properly when their is a collision with the other sortable-handles in the list. In my fiddle, if you drag "ITEM 1" and enter "ITEM 2" from the right border or left border and with no downward drag on your mouse; either a pure horizontal mouse-motion or, a horizontal and upward mouse-motion, then the library will not trigger a CHANGE event.

Indeed, the library will only trigger a CHANGE event when entering the list through the top and bottom borders or using a downward motion if entered form the side borders.

Note that I am using the sort option { tolerance : "pointer" } and that this is unexpected behavior and definitely does not feel good as a user. When I have my mouse over the element I want to sort - it should sort and trigger a CHANGE event no matter which angle I came from.

EXPLANATION

The main code in the _mouseDrag event does this;

A) _intersectWithPointer(item)

return false OR 1 OR 2

false: no collision

1 OR 2: collision with mouse coming from direction "X". Depends if list is horizontal or vertical and the delta: mousePosition - mousePreviousPosition. Logic is saying something like if "1" then the mouse drag was downard and rightwards, and if "2" then mouse drag was upwards and leftwards

B) placeHolder [ "next" OR "prev" ] - is the previous OR next element the same as our helper?

"prev" or "next" depends on the return value of _intersectWithPointer

C) call triggerChangeEvent

The problem here is that this works fine as long as your mouse stays within the list that contains the sortable elements, but acts buggy if you leave it and reenter from an axis opposite to that of the list. These sort of checks based on mouse direction are kind of prone to hidden bugs because you can enter a UI element from 4 different directions, and yet in this case only a single horizontal OR vertical direction will yield the expected behavior.

The current design is really not needed for what we want to do. We are not sorting things based on the direction our mouse is going in; we are sorting them based on where they are on the screen compared to our mouse pointer. It's also adds to the complexity, and although I'm a beginner it took me a long time to understand where my error was coming from, and then fix. So, the design could perhaps be improved to limit this error from happening in the future.

OFFERED SOLUTION

JS Fiddle: http://jsfiddle.net/AzuObs/cgh7xvpv/9/

A) Have _intersectWithPointer only return TRUE or FALSE - this means that the mouse collides with AN item but it could just be itself at this stage

_intersectsWithPointer: function(item) {
  var isOverElementHeight = (this.options.axis === "x") || this._isOverAxis(this.positionAbs.top + this.offset.click.top, item.top, item.height);
  var isOverElementWidth = (this.options.axis === "y") || this._isOverAxis(this.positionAbs.left + this.offset.click.left, item.left, item.width);

  return isOverElementHeight && isOverElementWidth;
}

B) Delete the check: if placeholder[ "next" OR "prev" ] is the previous element our helper

C) Inside the final check, just before called this._trigger("change"...)

// create position of placeholder and currentItem
var placeHoldPos = {
  x: this.placeholder[0].offsetLeft + this.placeholder[0].offsetWidth,
  y: this.placeholder[0].offsetTop + this.placeholder[0].offsetHeight
};
			
var currentItemPos = {
  x: this.currentItem[0].offsetLeft + this.currentItem[0].offsetWidth,
  y: this.currentItem[0].offsetTop + this.currentItem[0].offsetHeight
};
	
// direction of the collision?
if (this.floating) {
  if (placeHoldPos.x < currentItemPos.x) {
    this.direction = "up";
  } else {
    this.direction = "down";
  }
} else {
  if (placeHoldPos.y < currentItemPos.y) {
    this.direction = "up";
  } else {
    this.direction = "down";
  }
}

D) Time/Space complexity are still the same. You could tinker around with the _intersectWithPointer to return values between 1-9 if you wanted to keep in the original style - but honestly it's not worth keeping that system IMO as it's complicated and subject to hidden bugs like this one.

I hope that you like my fix. If you would like me to explain then I can but I think that it is simple. Also, I would really appreciate the chance to contribute and make a pull to the project as I've never done anything like that before :-)