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

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 #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 POINTER_DIRECTION_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 _pointerDirectionDelta: {x: -1 | 0 | 1, y: -1 | 0 | 1};
+
+  /** Pointer position at which the last change in the delta occurred. */
+  private _pointerPositionAtLastDirectionChange: Point;
+
   /** Elements that can be used to drag the draggable item. */
   @ContentChildren(CdkDragHandle) _handles: QueryList<CdkDragHandle>;
 
@@ -255,7 +267,10 @@ 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._pointerDirectionDelta = {x: 0, y: 0};
+    this._pointerPositionAtLastDirectionChange = {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 +307,7 @@ export class CdkDrag<T = any> implements OnDestroy {
     event.preventDefault();
 
     const pointerPosition = this._getConstrainedPointerPosition(event);
+    this._updatePointerDirectionDelta(pointerPosition);
 
     if (this.dropContainer) {
       this._updateActiveDropContainer(pointerPosition);
@@ -392,7 +408,7 @@ export class CdkDrag<T = any> implements OnDestroy {
       });
     }
 
-    this.dropContainer._sortItem(this, x, y);
+    this.dropContainer._sortItem(this, x, y, this._pointerDirectionDelta);
     this._setTransform(this._preview,
                        x - this._pickupPositionInElement.x,
                        y - this._pickupPositionInElement.y);
@@ -590,6 +606,31 @@ 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 _updatePointerDirectionDelta(pointerPositionOnPage: Point) {
+    const {x, y} = pointerPositionOnPage;
+    const delta = this._pointerDirectionDelta;
+    const positionSinceLastChange = this._pointerPositionAtLastDirectionChange;
+
+    // Amount of pixels the user has dragged since the last time the direction changed.
+    const changeX = Math.abs(x - positionSinceLastChange.x);
+    const changeY = Math.abs(y - positionSinceLastChange.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 > POINTER_DIRECTION_CHANGE_THRESHOLD) {
+      delta.x = x > positionSinceLastChange.x ? 1 : -1;
+      positionSinceLastChange.x = x;
+    }
+
+    if (changeY > POINTER_DIRECTION_CHANGE_THRESHOLD) {
+      delta.y = y > positionSinceLastChange.y ? 1 : -1;
+      positionSinceLastChange.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);
     });