fix(drag-drop): sorting too often if pointer is inside element after position swap

Currently we check whether the user's pointer overlaps an item for each pixel that they've moved. This works fine when the items have a similar height, however it breaks down if there's a position swap between a very tall item and a smaller one. These changes rework the logic to take into account the direction that the user is moving in, before doing a position swap.

Fixes angular#12694.

Author: crisbeto

src/cdk/drag-drop/drag.spec.ts

@@ -843,6 +843,86 @@ describe('CdkDrag', () => {
         flush();
       }));
 
+    it('should not swap position for tiny pointer movements', fakeAsync(() => {
+      const fixture = createComponent(DraggableInDropZone);
+      fixture.detectChanges();
+
+      const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+      const draggedItem = items[0];
+      const target = items[1];
+      const {top, left} = draggedItem.getBoundingClientRect();
+
+      // Bump the height so the pointer doesn't leave after swapping.
+      target.style.height = `${ITEM_HEIGHT * 3}px`;
+
+      dispatchMouseEvent(draggedItem, 'mousedown', left, top);
+      fixture.detectChanges();
+
+      const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+
+      expect(getElementSibligsByPosition(placeholder, 'top').map(e => e.textContent!.trim()))
+          .toEqual(['Zero', 'One', 'Two', 'Three']);
+
+      const targetRect = target.getBoundingClientRect();
+      const pointerTop = targetRect.top + 20;
+
+      // Move over the target so there's a 20px overlap.
+      dispatchMouseEvent(document, 'mousemove', targetRect.left, pointerTop);
+      fixture.detectChanges();
+      expect(getElementSibligsByPosition(placeholder, 'top').map(e => e.textContent!.trim()))
+          .toEqual(['One', 'Zero', 'Two', 'Three'], 'Expected position to swap.');
+
+      // Move down a further 1px.
+      dispatchMouseEvent(document, 'mousemove', targetRect.left, pointerTop + 1);
+      fixture.detectChanges();
+      expect(getElementSibligsByPosition(placeholder, 'top').map(e => e.textContent!.trim()))
+          .toEqual(['One', 'Zero', 'Two', 'Three'], 'Expected positions not to swap.');
+
+      dispatchMouseEvent(document, 'mouseup');
+      fixture.detectChanges();
+      flush();
+    }));
+
+    it('should swap position for pointer movements in the opposite direction', fakeAsync(() => {
+      const fixture = createComponent(DraggableInDropZone);
+      fixture.detectChanges();
+
+      const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+      const draggedItem = items[0];
+      const target = items[1];
+      const {top, left} = draggedItem.getBoundingClientRect();
+
+      // Bump the height so the pointer doesn't leave after swapping.
+      target.style.height = `${ITEM_HEIGHT * 3}px`;
+
+      dispatchMouseEvent(draggedItem, 'mousedown', left, top);
+      fixture.detectChanges();
+
+      const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+
+      expect(getElementSibligsByPosition(placeholder, 'top').map(e => e.textContent!.trim()))
+          .toEqual(['Zero', 'One', 'Two', 'Three']);
+
+      const targetRect = target.getBoundingClientRect();
+      const pointerTop = targetRect.top + 20;
+
+      // Move over the target so there's a 20px overlap.
+      dispatchMouseEvent(document, 'mousemove', targetRect.left, pointerTop);
+      fixture.detectChanges();
+      expect(getElementSibligsByPosition(placeholder, 'top').map(e => e.textContent!.trim()))
+          .toEqual(['One', 'Zero', 'Two', 'Three'], 'Expected position to swap.');
+
+      // Move up 10px.
+      dispatchMouseEvent(document, 'mousemove', targetRect.left, pointerTop - 10);
+      fixture.detectChanges();
+      expect(getElementSibligsByPosition(placeholder, 'top').map(e => e.textContent!.trim()))
+          .toEqual(['Zero', 'One', 'Two', 'Three'], 'Expected positions to swap again.');
+
+      dispatchMouseEvent(document, 'mouseup');
+      fixture.detectChanges();
+      flush();
+    }));
+
     it('should clean up the preview element if the item is destroyed mid-drag', fakeAsync(() => {
       const fixture = createComponent(DraggableInDropZone);
       fixture.detectChanges();

src/cdk/drag-drop/drag.ts

@@ -46,6 +46,12 @@ import {takeUntil} from 'rxjs/operators';
 // TODO(crisbeto): add an API for moving a draggable up/down the
 // list programmatically. Useful for keyboard controls.
 
+/**
+ * Amount the pixels the user should drag before we
+ * consider them to have changed the drag direction.
+ */
+const DELTA_CHANGE_THRESHOLD = 5;
+
 /** Element that can be moved inside a CdkDrop container. */
 @Directive({
   selector: '[cdkDrag]',
@@ -114,6 +120,12 @@ export class CdkDrag<T = any> implements OnDestroy {
    */
   private _moveEventSubscriptions = 0;
 
+  /** Keeps track of the direction in which the user is dragging along each axis. */
+  private _delta: {
+    /** Pointer position at which the last change in the delta occurred. */
+    positionAtLastChange: Point
+  } & Point;
+
   /** Elements that can be used to drag the draggable item. */
   @ContentChildren(CdkDragHandle) _handles: QueryList<CdkDragHandle>;
 
@@ -255,7 +267,9 @@ export class CdkDrag<T = any> implements OnDestroy {
     // extra `getBoundingClientRect` calls and just move the preview next to the cursor.
     this._pickupPositionInElement = this._previewTemplate ? {x: 0, y: 0} :
         this._getPointerPositionInElement(referenceElement, event);
-    this._pickupPositionOnPage = this._getPointerPositionOnPage(event);
+    const pointerPosition = this._pickupPositionOnPage = this._getPointerPositionOnPage(event);
+
+    this._delta = {x: 0, y: 0, positionAtLastChange: {x: pointerPosition.x, y: pointerPosition.y}};
 
     // Emit the event on the item before the one on the container.
     this.started.emit({source: this});
@@ -292,6 +306,7 @@ export class CdkDrag<T = any> implements OnDestroy {
     event.preventDefault();
 
     const pointerPosition = this._getConstrainedPointerPosition(event);
+    this._updateDelta(pointerPosition);
 
     if (this.dropContainer) {
       this._updateActiveDropContainer(pointerPosition);
@@ -392,7 +407,7 @@ export class CdkDrag<T = any> implements OnDestroy {
       });
     }
 
-    this.dropContainer._sortItem(this, x, y);
+    this.dropContainer._sortItem(this, x, y, this._delta);
     this._setTransform(this._preview,
                        x - this._pickupPositionInElement.x,
                        y - this._pickupPositionInElement.y);
@@ -590,6 +605,30 @@ export class CdkDrag<T = any> implements OnDestroy {
 
     this._placeholder = this._placeholderRef = null!;
   }
+
+  /** Updates the current drag delta, based on the user's current pointer position on the page. */
+  private _updateDelta(pointerPositionOnPage: Point) {
+    const {x, y} = pointerPositionOnPage;
+    const delta = this._delta;
+
+    // Amount of pixels the user has dragged since the last time the direction changed.
+    const changeX = Math.abs(x - delta.positionAtLastChange.x);
+    const changeY = Math.abs(y - delta.positionAtLastChange.y);
+
+    // Because we handle pointer events on a per-pixel basis, we don't want the delta
+    // to change for every pixel, otherwise anything that depends on it can look erratic.
+    // To make the delta more consistent, we track how much the user has moved since the last
+    // delta change and we only update it after it has reached a certain threshold.
+    if (changeX > DELTA_CHANGE_THRESHOLD) {
+      delta.x = x > delta.positionAtLastChange.x ? 1 : -1;
+      delta.positionAtLastChange.x = x;
+    }
+
+    if (changeY > DELTA_CHANGE_THRESHOLD) {
+      delta.y = y > delta.positionAtLastChange.y ? 1 : -1;
+      delta.positionAtLastChange.y = y;
+    }
+  }
 }
 
 /** Parses a CSS time value to milliseconds. */

src/cdk/drag-drop/drop-container.ts

@@ -52,7 +52,7 @@ export interface CdkDropContainer<T = any> {
    * @param item Item whose index should be determined.
    */
   getItemIndex(item: CdkDrag): number;
-  _sortItem(item: CdkDrag, pointerX: number, pointerY: number): void;
+  _sortItem(item: CdkDrag, pointerX: number, pointerY: number, delta: {x: number, y: number}): void;
   _draggables: QueryList<CdkDrag>;
   _getSiblingContainerFromPosition(item: CdkDrag, x: number, y: number): CdkDropContainer | null;
 }

src/cdk/drag-drop/drop.ts

@@ -129,6 +129,12 @@ export class CdkDrop<T = any> implements OnInit, OnDestroy {
    */
   private _activeDraggables: CdkDrag[];
 
+  /**
+   * Keeps track of the item that was last swapped with the dragged item, as
+   * well as what direction the pointer was moving in when the swap occured.
+   */
+  private _previousSwap = {drag: null as CdkDrag | null, delta: 0};
+
   /** Starts dragging an item. */
   start(): void {
     this._dragging = true;
@@ -220,26 +226,32 @@ export class CdkDrop<T = any> implements OnInit, OnDestroy {
    * @param item Item to be sorted.
    * @param pointerX Position of the item along the X axis.
    * @param pointerY Position of the item along the Y axis.
+   * @param pointerDeta Direction in which the pointer is moving along each axis.
    */
-  _sortItem(item: CdkDrag, pointerX: number, pointerY: number): void {
+  _sortItem(item: CdkDrag, pointerX: number, pointerY: number,
+            pointerDelta: {x: number, y: number}): void {
     // Don't sort the item if it's out of range.
     if (!this._isPointerNearDropContainer(pointerX, pointerY)) {
       return;
     }
 
     const siblings = this._positionCache.items;
-    const newIndex = this._getItemIndexFromPointerPosition(item, pointerX, pointerY);
+    const newIndex = this._getItemIndexFromPointerPosition(item, pointerX, pointerY, pointerDelta);
 
     if (newIndex === -1 && siblings.length > 0) {
       return;
     }
 
     const isHorizontal = this.orientation === 'horizontal';
     const currentIndex = siblings.findIndex(currentItem => currentItem.drag === item);
+    const siblingAtNewPosition = siblings[newIndex];
     const currentPosition = siblings[currentIndex].clientRect;
-    const newPosition = siblings[newIndex].clientRect;
+    const newPosition = siblingAtNewPosition.clientRect;
     const delta = currentIndex > newIndex ? 1 : -1;
 
+    this._previousSwap.drag = siblingAtNewPosition.drag;
+    this._previousSwap.delta = isHorizontal ? pointerDelta.x : pointerDelta.y;
+
     // How many pixels the item's placeholder should be offset.
     const itemOffset = isHorizontal ? newPosition.left - currentPosition.left :
                                       newPosition.top - currentPosition.top;
@@ -346,6 +358,8 @@ export class CdkDrop<T = any> implements OnInit, OnDestroy {
     this._activeDraggables = [];
     this._positionCache.items = [];
     this._positionCache.siblings = [];
+    this._previousSwap.drag = null;
+    this._previousSwap.delta = 0;
   }
 
   /**
@@ -367,18 +381,33 @@ export class CdkDrop<T = any> implements OnInit, OnDestroy {
    * @param item Item that is being sorted.
    * @param pointerX Position of the user's pointer along the X axis.
    * @param pointerY Position of the user's pointer along the Y axis.
+   * @param delta Direction in which the user is moving their pointer.
    */
-  private _getItemIndexFromPointerPosition(item: CdkDrag, pointerX: number, pointerY: number) {
+  private _getItemIndexFromPointerPosition(item: CdkDrag, pointerX: number, pointerY: number,
+                                           delta?: {x: number, y: number}) {
+
+    const isHorizontal = this.orientation === 'horizontal';
+
     return this._positionCache.items.findIndex(({drag, clientRect}, _, array) => {
       if (drag === item) {
         // If there's only one item left in the container, it must be
         // the dragged item itself so we use it as a reference.
         return array.length < 2;
       }
 
-      return this.orientation === 'horizontal' ?
+      if (delta) {
+        const direction = isHorizontal ? delta.x : delta.y;
+
+        // If the user is still hovering over the same item as last time, and they didn't change
+        // the direction in which they're dragging, we don't consider it a direction swap.
+        if (drag === this._previousSwap.drag && direction === this._previousSwap.delta) {
+          return false;
+        }
+      }
+
+      return isHorizontal ?
           // Round these down since most browsers report client rects with
-          // sub-pixel precision, whereas the mouse coordinates are rounded to pixels.
+          // sub-pixel precision, whereas the pointer coordinates are rounded to pixels.
           pointerX >= Math.floor(clientRect.left) && pointerX <= Math.floor(clientRect.right) :
           pointerY >= Math.floor(clientRect.top) && pointerY <= Math.floor(clientRect.bottom);
     });