From dfead914e67e2e7157d424919dd193bbf3373cdc Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 27 Aug 2018 17:26:35 +0200 Subject: [PATCH] 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 #12694. --- src/cdk/drag-drop/drag.spec.ts | 80 +++++++++++++++++++++++++++++ src/cdk/drag-drop/drag.ts | 45 +++++++++++++++- src/cdk/drag-drop/drop-container.ts | 2 +- src/cdk/drag-drop/drop.ts | 41 ++++++++++++--- 4 files changed, 159 insertions(+), 9 deletions(-) diff --git a/src/cdk/drag-drop/drag.spec.ts b/src/cdk/drag-drop/drag.spec.ts index 34240908f218..46662d837d97 100644 --- a/src/cdk/drag-drop/drag.spec.ts +++ b/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(); diff --git a/src/cdk/drag-drop/drag.ts b/src/cdk/drag-drop/drag.ts index c1b667cd47b9..5ec0d7f7891b 100644 --- a/src/cdk/drag-drop/drag.ts +++ b/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 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; @@ -255,7 +267,10 @@ export class CdkDrag 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 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 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 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. */ diff --git a/src/cdk/drag-drop/drop-container.ts b/src/cdk/drag-drop/drop-container.ts index b489a8bbb2e8..61461b60cf72 100644 --- a/src/cdk/drag-drop/drop-container.ts +++ b/src/cdk/drag-drop/drop-container.ts @@ -52,7 +52,7 @@ export interface CdkDropContainer { * @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; _getSiblingContainerFromPosition(item: CdkDrag, x: number, y: number): CdkDropContainer | null; } diff --git a/src/cdk/drag-drop/drop.ts b/src/cdk/drag-drop/drop.ts index a83e6dfbae1c..fe2c6a66e366 100644 --- a/src/cdk/drag-drop/drop.ts +++ b/src/cdk/drag-drop/drop.ts @@ -129,6 +129,12 @@ export class CdkDrop 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,15 +226,17 @@ export class CdkDrop 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; @@ -236,10 +244,14 @@ export class CdkDrop implements OnInit, OnDestroy { 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 implements OnInit, OnDestroy { this._activeDraggables = []; this._positionCache.items = []; this._positionCache.siblings = []; + this._previousSwap.drag = null; + this._previousSwap.delta = 0; } /** @@ -367,8 +381,13 @@ export class CdkDrop 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 @@ -376,9 +395,19 @@ export class CdkDrop implements OnInit, OnDestroy { 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); });