From 693bf0edd6fc8fee25a000ff93b38b08e3fba0cc Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 27 Dec 2018 12:06:57 +0200 Subject: [PATCH] fix(drag-drop): unable to move item into connected container by passing through another container Fixes not being able to move an item from one container into another by passing it through an intermediate container that isn't connected to the final one. The issue comes from the fact that the way things are set up at the moment, the container from which the item started the sequence knows which containers it can go into, however all that knowledge is reset once the item enters into a different container. These changes rework the logic to have the individual containers know whether the item can enter into them and have the source container "ask" each of its siblings whether the item can enter. Fixes #14645. --- src/cdk/drag-drop/directives/drag.spec.ts | 66 ++++++++ src/cdk/drag-drop/drag-ref.ts | 5 +- src/cdk/drag-drop/drop-list-ref.ts | 198 ++++++++++------------ 3 files changed, 162 insertions(+), 107 deletions(-) diff --git a/src/cdk/drag-drop/directives/drag.spec.ts b/src/cdk/drag-drop/directives/drag.spec.ts index 64db4880263d..eb9b2f257610 100644 --- a/src/cdk/drag-drop/directives/drag.spec.ts +++ b/src/cdk/drag-drop/directives/drag.spec.ts @@ -2678,6 +2678,63 @@ describe('CdkDrag', () => { 'Expected new container not to have the receiving class after entering.'); })); + it('should be able to move the item over an intermediate container before ' + + 'dropping it into the final one', fakeAsync(() => { + const fixture = createComponent(ConnectedDropZones); + fixture.detectChanges(); + + const dropInstances = fixture.componentInstance.dropInstances.toArray(); + dropInstances[0].connectedTo = [dropInstances[1], dropInstances[2]]; + dropInstances[1].connectedTo = []; + dropInstances[2].connectedTo = []; + fixture.detectChanges(); + + const groups = fixture.componentInstance.groupedDragItems; + const dropZones = dropInstances.map(d => d.element.nativeElement); + const item = groups[0][1]; + const intermediateRect = dropZones[1].getBoundingClientRect(); + const finalRect = dropZones[2].getBoundingClientRect(); + + startDraggingViaMouse(fixture, item.element.nativeElement); + + const placeholder = dropZones[0].querySelector('.cdk-drag-placeholder')!; + + expect(placeholder).toBeTruthy(); + expect(dropZones[0].contains(placeholder)) + .toBe(true, 'Expected placeholder to be inside the first container.'); + + dispatchMouseEvent(document, 'mousemove', + intermediateRect.left + 1, intermediateRect.top + 1); + fixture.detectChanges(); + + expect(dropZones[1].contains(placeholder)) + .toBe(true, 'Expected placeholder to be inside second container.'); + + dispatchMouseEvent(document, 'mousemove', finalRect.left + 1, finalRect.top + 1); + fixture.detectChanges(); + + expect(dropZones[2].contains(placeholder)) + .toBe(true, 'Expected placeholder to be inside third container.'); + + dispatchMouseEvent(document, 'mouseup'); + fixture.detectChanges(); + flush(); + fixture.detectChanges(); + + const event = fixture.componentInstance.droppedSpy.calls.mostRecent().args[0]; + + expect(event).toBeTruthy(); + expect(event).toEqual(jasmine.objectContaining({ + previousIndex: 1, + currentIndex: 0, + item: groups[0][1], + container: dropInstances[2], + previousContainer: dropInstances[0], + isPointerOverContainer: false + })); + + })); + }); }); @@ -2967,6 +3024,14 @@ class DraggableInDropZoneWithCustomPlaceholder { (cdkDropListDropped)="droppedSpy($event)">
{{item}}
+ +
+
{{item}}
+
` }) class ConnectedDropZones implements AfterViewInit { @@ -2976,6 +3041,7 @@ class ConnectedDropZones implements AfterViewInit { groupedDragItems: CdkDrag[][] = []; todo = ['Zero', 'One', 'Two', 'Three']; done = ['Four', 'Five', 'Six']; + extra = []; droppedSpy = jasmine.createSpy('dropped spy'); ngAfterViewInit() { diff --git a/src/cdk/drag-drop/drag-ref.ts b/src/cdk/drag-drop/drag-ref.ts index dcf014274673..5af39da6ff43 100644 --- a/src/cdk/drag-drop/drag-ref.ts +++ b/src/cdk/drag-drop/drag-ref.ts @@ -691,7 +691,8 @@ export class DragRef { */ private _updateActiveDropContainer({x, y}: Point) { // Drop container that draggable has been moved into. - let newContainer = this.dropContainer!._getSiblingContainerFromPosition(this, x, y); + let newContainer = this.dropContainer!._getSiblingContainerFromPosition(this, x, y) || + this._initialContainer._getSiblingContainerFromPosition(this, x, y); // If we couldn't find a new container to move the item into, and the item has left it's // initial container, check whether the it's over the initial container. This handles the @@ -702,7 +703,7 @@ export class DragRef { newContainer = this._initialContainer; } - if (newContainer) { + if (newContainer && newContainer !== this.dropContainer) { this._ngZone.run(() => { // Notify the old container that the item has left. this.exited.next({item: this, container: this.dropContainer!}); diff --git a/src/cdk/drag-drop/drop-list-ref.ts b/src/cdk/drag-drop/drop-list-ref.ts index a99e89c9d850..cdafa2c78bdc 100644 --- a/src/cdk/drag-drop/drop-list-ref.ts +++ b/src/cdk/drag-drop/drop-list-ref.ts @@ -23,24 +23,11 @@ let _uniqueIdCounter = 0; */ const DROP_PROXIMITY_THRESHOLD = 0.05; -/** - * Object used to cache the position of a drag list, its items. and siblings. - * @docs-private - */ -interface PositionCache { - /** Cached positions of the items in the list. */ - items: ItemPositionCacheEntry[]; - /** Cached positions of the connected lists. */ - siblings: ListPositionCacheEntry[]; - /** Dimensions of the list itself. */ - self: ClientRect; -} - /** * Entry in the position cache for draggable items. * @docs-private */ -interface ItemPositionCacheEntry { +interface CachedItemPosition { /** Instance of the drag item. */ drag: DragRef; /** Dimensions of the item. */ @@ -49,17 +36,6 @@ interface ItemPositionCacheEntry { offset: number; } -/** - * Entry in the position cache for drop lists. - * @docs-private - */ -interface ListPositionCacheEntry { - /** Instance of the drop list. */ - drop: DropListRef; - /** Dimensions of the list. */ - clientRect: ClientRect; -} - /** * Internal compile-time-only representation of a `DropListRef`. * Used to avoid circular import issues between the `DropListRef` and the `DragRef`. @@ -131,8 +107,11 @@ export class DropListRef { /** Whether an item in the list is being dragged. */ private _isDragging = false; - /** Cache of the dimensions of all the items and the sibling containers. */ - private _positionCache: PositionCache = {items: [], siblings: [], self: {} as ClientRect}; + /** Cache of the dimensions of all the items inside the container. */ + private _itemPositions: CachedItemPosition[] = []; + + /** Cached `ClientRect` of the drop list. */ + private _clientRect: ClientRect; /** * Draggable items that are currently active inside the container. Includes the items @@ -147,19 +126,17 @@ export class DropListRef { */ private _previousSwap = {drag: null as DragRef | null, delta: 0}; - /** - * Draggable items in the container. - * TODO(crisbeto): support arrays. - */ + /** Draggable items in the container. */ private _draggables: DragRef[]; + /** Drop lists that are connected to the current one. */ private _siblings: DropListRef[] = []; /** Direction in which the list is oriented. */ private _orientation: 'horizontal' | 'vertical' = 'vertical'; - /** Amount of connected siblings that currently have a dragged item. */ - private _activeSiblings = 0; + /** Connected siblings that currently have a dragged item. */ + private _activeSiblings = new Set(); constructor( public element: ElementRef, @@ -177,6 +154,7 @@ export class DropListRef { this.exited.complete(); this.dropped.complete(); this.sorted.complete(); + this._activeSiblings.clear(); this._dragDropRegistry.removeDropContainer(this); } @@ -190,8 +168,9 @@ export class DropListRef { this.beforeStarted.next(); this._isDragging = true; this._activeDraggables = this._draggables.slice(); - this._cachePositions(); - this._positionCache.siblings.forEach(sibling => sibling.drop._toggleIsReceiving(true)); + this._cacheOwnPosition(); + this._cacheItemPositions(); + this._siblings.forEach(sibling => sibling._startReceiving(this)); } /** @@ -233,7 +212,7 @@ export class DropListRef { // Note that the positions were already cached when we called `start` above, // but we need to refresh them since the amount of items has changed. - this._cachePositions(); + this._cacheItemPositions(); } /** @@ -307,7 +286,7 @@ export class DropListRef { // The rest of the logic still stands no matter what orientation we're in, however // we need to invert the array when determining the index. const items = this._orientation === 'horizontal' && this._dir && this._dir.value === 'rtl' ? - this._positionCache.items.slice().reverse() : this._positionCache.items; + this._itemPositions.slice().reverse() : this._itemPositions; return findIndex(items, currentItem => currentItem.drag === item); } @@ -317,7 +296,7 @@ export class DropListRef { * is currently being dragged inside a connected drop list. */ isReceiving(): boolean { - return this._activeSiblings > 0; + return this._activeSiblings.size > 0; } /** @@ -334,7 +313,7 @@ export class DropListRef { return; } - const siblings = this._positionCache.items; + const siblings = this._itemPositions; const newIndex = this._getItemIndexFromPointerPosition(item, pointerX, pointerY, pointerDelta); if (newIndex === -1 && siblings.length > 0) { @@ -401,54 +380,43 @@ export class DropListRef { }); } + /** Caches the position of the drop list. */ + private _cacheOwnPosition() { + this._clientRect = this.element.nativeElement.getBoundingClientRect(); + } + /** Refreshes the position cache of the items and sibling containers. */ - private _cachePositions() { + private _cacheItemPositions() { const isHorizontal = this._orientation === 'horizontal'; - this._positionCache.self = this.element.nativeElement.getBoundingClientRect(); - this._positionCache.items = this._activeDraggables - .map(drag => { - const elementToMeasure = this._dragDropRegistry.isDragging(drag) ? - // If the element is being dragged, we have to measure the - // placeholder, because the element is hidden. - drag.getPlaceholderElement() : - drag.getRootElement(); - const clientRect = elementToMeasure.getBoundingClientRect(); - - return { - drag, - offset: 0, - // We need to clone the `clientRect` here, because all the values on it are readonly - // and we need to be able to update them. Also we can't use a spread here, because - // the values on a `ClientRect` aren't own properties. See: - // https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect#Notes - clientRect: { - top: clientRect.top, - right: clientRect.right, - bottom: clientRect.bottom, - left: clientRect.left, - width: clientRect.width, - height: clientRect.height - } - }; - }) - .sort((a, b) => { - return isHorizontal ? a.clientRect.left - b.clientRect.left : - a.clientRect.top - b.clientRect.top; - }); - - this._positionCache.siblings = this._siblings.map(drop => ({ - drop, - clientRect: drop.element.nativeElement.getBoundingClientRect() - })); - } - - /** - * Toggles whether the list can receive the item that is currently being dragged. - * Usually called by a sibling that initiated the dragging. - */ - _toggleIsReceiving(isDragging: boolean) { - this._activeSiblings = Math.max(0, this._activeSiblings + (isDragging ? 1 : -1)); + this._itemPositions = this._activeDraggables.map(drag => { + const elementToMeasure = this._dragDropRegistry.isDragging(drag) ? + // If the element is being dragged, we have to measure the + // placeholder, because the element is hidden. + drag.getPlaceholderElement() : + drag.getRootElement(); + const clientRect = elementToMeasure.getBoundingClientRect(); + + return { + drag, + offset: 0, + // We need to clone the `clientRect` here, because all the values on it are readonly + // and we need to be able to update them. Also we can't use a spread here, because + // the values on a `ClientRect` aren't own properties. See: + // https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect#Notes + clientRect: { + top: clientRect.top, + right: clientRect.right, + bottom: clientRect.bottom, + left: clientRect.left, + width: clientRect.width, + height: clientRect.height + } + }; + }).sort((a, b) => { + return isHorizontal ? a.clientRect.left - b.clientRect.left : + a.clientRect.top - b.clientRect.top; + }); } /** Resets the container to its initial state. */ @@ -457,10 +425,9 @@ export class DropListRef { // TODO(crisbeto): may have to wait for the animations to finish. this._activeDraggables.forEach(item => item.getRootElement().style.transform = ''); - this._positionCache.siblings.forEach(sibling => sibling.drop._toggleIsReceiving(false)); + this._siblings.forEach(sibling => sibling._stopReceiving(this)); this._activeDraggables = []; - this._positionCache.items = []; - this._positionCache.siblings = []; + this._itemPositions = []; this._previousSwap.drag = null; this._previousSwap.delta = 0; } @@ -472,7 +439,7 @@ export class DropListRef { * @param delta Direction in which the user is moving. */ private _getSiblingOffsetPx(currentIndex: number, - siblings: ItemPositionCacheEntry[], + siblings: CachedItemPosition[], delta: 1 | -1) { const isHorizontal = this._orientation === 'horizontal'; @@ -504,7 +471,7 @@ export class DropListRef { * @param pointerY Coordinates along the Y axis. */ private _isPointerNearDropContainer(pointerX: number, pointerY: number): boolean { - const {top, right, bottom, left, width, height} = this._positionCache.self; + const {top, right, bottom, left, width, height} = this._clientRect; const xThreshold = width * DROP_PROXIMITY_THRESHOLD; const yThreshold = height * DROP_PROXIMITY_THRESHOLD; @@ -541,10 +508,9 @@ export class DropListRef { */ private _getItemIndexFromPointerPosition(item: DragRef, pointerX: number, pointerY: number, delta?: {x: number, y: number}) { - const isHorizontal = this._orientation === 'horizontal'; - return findIndex(this._positionCache.items, ({drag, clientRect}, _, array) => { + return findIndex(this._itemPositions, ({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. @@ -575,7 +541,7 @@ export class DropListRef { * @param y Pointer position along the Y axis. */ _isOverContainer(x: number, y: number): boolean { - return isInsideClientRect(this._positionCache.self, x, y); + return isInsideClientRect(this._clientRect, x, y); } /** @@ -585,14 +551,19 @@ export class DropListRef { * @param x Position of the item along the X axis. * @param y Position of the item along the Y axis. */ - _getSiblingContainerFromPosition(item: DragRef, x: number, y: number): DropListRef | null { - const results = this._positionCache.siblings.filter(sibling => { - return isInsideClientRect(sibling.clientRect, x, y); - }); + _getSiblingContainerFromPosition(item: DragRef, x: number, y: number): DropListRef | undefined { + return this._siblings.find(sibling => sibling._canReceive(item, x, y)); + } - // No drop containers are intersecting with the pointer. - if (!results.length) { - return null; + /** + * Checks whether the drop list can receive the passed-in item. + * @param item Item that is being dragged into the list. + * @param x Position of the item along the X axis. + * @param y Position of the item along the Y axis. + */ + _canReceive(item: DragRef, x: number, y: number): boolean { + if (!this.enterPredicate(item, this) || !isInsideClientRect(this._clientRect, x, y)) { + return false; } const elementFromPoint = this._document.elementFromPoint(x, y); @@ -600,23 +571,40 @@ export class DropListRef { // If there's no element at the pointer position, then // the client rect is probably scrolled out of the view. if (!elementFromPoint) { - return null; + return false; } + const element = this.element.nativeElement; + // The `ClientRect`, that we're using to find the container over which the user is // hovering, doesn't give us any information on whether the element has been scrolled // out of the view or whether it's overlapping with other containers. This means that // we could end up transferring the item into a container that's invisible or is positioned // below another one. We use the result from `elementFromPoint` to get the top-most element // at the pointer position and to find whether it's one of the intersecting drop containers. - const result = results.find(sibling => { - const element = sibling.drop.element.nativeElement; - return element === elementFromPoint || element.contains(elementFromPoint); - }); + return elementFromPoint === element || element.contains(elementFromPoint); + } + + /** + * Called by one of the connected drop lists when a dragging sequence has started. + * @param sibling Sibling in which dragging has started. + */ + _startReceiving(sibling: DropListRef) { + const activeSiblings = this._activeSiblings; - return result && result.drop.enterPredicate(item, result.drop) ? result.drop : null; + if (!activeSiblings.has(sibling)) { + activeSiblings.add(sibling); + this._cacheOwnPosition(); + } } + /** + * Called by a connected drop list when dragging has stopped. + * @param sibling Sibling whose dragging has stopped. + */ + _stopReceiving(sibling: DropListRef) { + this._activeSiblings.delete(sibling); + } }