From 411b1747df587243f7d70f8e5a7cc75a753d1b7d Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 17 Dec 2020 22:57:56 +0100 Subject: [PATCH] fix(cdk/drag-drop): account for enterPredicate when setting receiving class (#21346) Currently we set a class when a container is able to receive the dragged item, however we add this class to all connected containers without accounting for other things that could prevent the item from entering, like `enterPredicate`. These changes add some logic to account for the predicate when setting the class. Fixes #21171. --- src/cdk/drag-drop/directives/drag.spec.ts | 92 ++++++++++++++++------- src/cdk/drag-drop/drop-list-ref.ts | 62 +++++++++------ tools/public_api_guard/cdk/drag-drop.d.ts | 2 +- 3 files changed, 105 insertions(+), 51 deletions(-) diff --git a/src/cdk/drag-drop/directives/drag.spec.ts b/src/cdk/drag-drop/directives/drag.spec.ts index 80eab4e8f871..ad79d4585b6c 100644 --- a/src/cdk/drag-drop/directives/drag.spec.ts +++ b/src/cdk/drag-drop/directives/drag.spec.ts @@ -4483,33 +4483,6 @@ describe('CdkDrag', () => { expect(spy).toHaveBeenCalledWith(dragItem, dropInstances[1]); })); - it('should not call the `enterPredicate` if the pointer is not over the container', - fakeAsync(() => { - const fixture = createComponent(ConnectedDropZones); - fixture.detectChanges(); - - const dropInstances = fixture.componentInstance.dropInstances.toArray(); - const spy = jasmine.createSpy('enterPredicate spy').and.returnValue(true); - const groups = fixture.componentInstance.groupedDragItems.slice(); - const dragElement = groups[0][1].element.nativeElement; - const targetRect = groups[1][2].element.nativeElement.getBoundingClientRect(); - - dropInstances[1].enterPredicate = spy; - fixture.detectChanges(); - - startDraggingViaMouse(fixture, dragElement); - - dispatchMouseEvent(document, 'mousemove', targetRect.left - 1, targetRect.top - 1); - fixture.detectChanges(); - - expect(spy).not.toHaveBeenCalled(); - - dispatchMouseEvent(document, 'mousemove', targetRect.left + 1, targetRect.top + 1); - fixture.detectChanges(); - - expect(spy).toHaveBeenCalledTimes(1); - })); - it('should be able to start dragging after an item has been transferred', fakeAsync(() => { const fixture = createComponent(ConnectedDropZones); fixture.detectChanges(); @@ -5009,14 +4982,75 @@ describe('CdkDrag', () => { expect(dropZones[0].classList) .toContain( 'cdk-drop-list-receiving', - 'Expected old container not to have the receiving class after exiting.'); + 'Expected old container to have the receiving class after exiting.'); expect(dropZones[1].classList) .not.toContain( 'cdk-drop-list-receiving', - 'Expected new container not to have the receiving class after entering.'); + 'Expected new container not to have the receiving class after exiting.'); })); + it('should not set the receiving class if the item does not match the enter predicate', + fakeAsync(() => { + const fixture = createComponent(ConnectedDropZones); + fixture.detectChanges(); + fixture.componentInstance.dropInstances.toArray()[1].enterPredicate = () => false; + + const dropZones = + fixture.componentInstance.dropInstances.map(d => d.element.nativeElement); + const item = fixture.componentInstance.groupedDragItems[0][1]; + + expect(dropZones.every(c => !c.classList.contains('cdk-drop-list-receiving'))) + .toBe(true, 'Expected neither of the containers to have the class.'); + + startDraggingViaMouse(fixture, item.element.nativeElement); + fixture.detectChanges(); + + expect(dropZones.every(c => !c.classList.contains('cdk-drop-list-receiving'))) + .toBe(true, 'Expected neither of the containers to have the class.'); + })); + + it('should set the receiving class on the source container, even if the enter predicate ' + + 'does not match', fakeAsync(() => { + const fixture = createComponent(ConnectedDropZones); + fixture.detectChanges(); + fixture.componentInstance.dropInstances.toArray()[0].enterPredicate = () => false; + + const groups = fixture.componentInstance.groupedDragItems; + const dropZones = + fixture.componentInstance.dropInstances.map(d => d.element.nativeElement); + const item = groups[0][1]; + const targetRect = groups[1][2].element.nativeElement.getBoundingClientRect(); + + expect(dropZones.every(c => !c.classList.contains('cdk-drop-list-receiving'))) + .toBe(true, 'Expected neither of the containers to have the class.'); + + startDraggingViaMouse(fixture, item.element.nativeElement); + + expect(dropZones[0].classList) + .not.toContain( + 'cdk-drop-list-receiving', + 'Expected source container not to have the receiving class.'); + + expect(dropZones[1].classList) + .toContain( + 'cdk-drop-list-receiving', + 'Expected target container to have the receiving class.'); + + dispatchMouseEvent(document, 'mousemove', targetRect.left + 1, targetRect.top + 1); + fixture.detectChanges(); + + expect(dropZones[0].classList) + .toContain( + 'cdk-drop-list-receiving', + 'Expected old container to have the receiving class after exiting.'); + + expect(dropZones[1].classList) + .not.toContain( + 'cdk-drop-list-receiving', + 'Expected new container not to have the receiving class after exiting.'); + })); + it('should be able to move the item over an intermediate container before ' + 'dropping it into the final one', fakeAsync(() => { const fixture = createComponent(ConnectedDropZones); diff --git a/src/cdk/drag-drop/drop-list-ref.ts b/src/cdk/drag-drop/drop-list-ref.ts index 7577ebb23dc8..48e276b4782b 100644 --- a/src/cdk/drag-drop/drop-list-ref.ts +++ b/src/cdk/drag-drop/drop-list-ref.ts @@ -146,7 +146,7 @@ export class DropListRef { private _parentPositions: ParentPositionTracker; /** Cached `ClientRect` of the drop list. */ - private _clientRect: ClientRect; + private _clientRect: ClientRect | undefined; /** * Draggable items that are currently active inside the container. Includes the items @@ -163,7 +163,7 @@ export class DropListRef { private _previousSwap = {drag: null as DragRef | null, delta: 0, overlaps: false}; /** Draggable items in the container. */ - private _draggables: ReadonlyArray; + private _draggables: ReadonlyArray = []; /** Drop lists that are connected to the current one. */ private _siblings: ReadonlyArray = []; @@ -240,19 +240,8 @@ export class DropListRef { /** Starts dragging an item. */ start(): void { - const styles = coerceElement(this.element).style as DragCSSStyleDeclaration; - this.beforeStarted.next(); - this._isDragging = true; - - // We need to disable scroll snapping while the user is dragging, because it breaks automatic - // scrolling. The browser seems to round the value based on the snapping points which means - // that we can't increment/decrement the scroll position. - this._initialScrollSnap = styles.msScrollSnapType || styles.scrollSnapType || ''; - styles.scrollSnapType = styles.msScrollSnapType = 'none'; - this._cacheItems(); - this._siblings.forEach(sibling => sibling._startReceiving(this)); - this._viewportScrollSubscription.unsubscribe(); - this._listenToScrollEvents(); + this._draggingStarted(); + this._notifyReceivingSiblings(); } /** @@ -264,7 +253,7 @@ export class DropListRef { * out automatically. */ enter(item: DragRef, pointerX: number, pointerY: number, index?: number): void { - this.start(); + this._draggingStarted(); // If sorting is disabled, we want the item to return to its starting // position if the user is returning it to its initial container. @@ -323,6 +312,8 @@ export class DropListRef { this._cacheItemPositions(); this._cacheParentPositions(); + // Notify siblings at the end so that the item has been inserted into the `activeDraggables`. + this._notifyReceivingSiblings(); this.entered.next({item, container: this, currentIndex: this.getItemIndex(item)}); } @@ -463,7 +454,7 @@ export class DropListRef { _sortItem(item: DragRef, pointerX: number, pointerY: number, pointerDelta: {x: number, y: number}): void { // Don't sort the item if sorting is disabled or it's out of range. - if (this.sortingDisabled || + if (this.sortingDisabled || !this._clientRect || !isPointerNearClientRect(this._clientRect, DROP_PROXIMITY_THRESHOLD, pointerX, pointerY)) { return; } @@ -600,6 +591,22 @@ export class DropListRef { this._stopScrollTimers.next(); } + /** Starts the dragging sequence within the list. */ + private _draggingStarted() { + const styles = coerceElement(this.element).style as DragCSSStyleDeclaration; + this.beforeStarted.next(); + this._isDragging = true; + + // We need to disable scroll snapping while the user is dragging, because it breaks automatic + // scrolling. The browser seems to round the value based on the snapping points which means + // that we can't increment/decrement the scroll position. + this._initialScrollSnap = styles.msScrollSnapType || styles.scrollSnapType || ''; + styles.scrollSnapType = styles.msScrollSnapType = 'none'; + this._cacheItems(); + this._viewportScrollSubscription.unsubscribe(); + this._listenToScrollEvents(); + } + /** Caches the positions of the configured scrollable parents. */ private _cacheParentPositions() { const element = coerceElement(this.element); @@ -802,7 +809,7 @@ export class DropListRef { * @param y Pointer position along the Y axis. */ _isOverContainer(x: number, y: number): boolean { - return isInsideClientRect(this._clientRect, x, y); + return this._clientRect != null && isInsideClientRect(this._clientRect, x, y); } /** @@ -823,7 +830,8 @@ export class DropListRef { * @param y Position of the item along the Y axis. */ _canReceive(item: DragRef, x: number, y: number): boolean { - if (!isInsideClientRect(this._clientRect, x, y) || !this.enterPredicate(item, this)) { + if (!this._clientRect || !isInsideClientRect(this._clientRect, x, y) || + !this.enterPredicate(item, this)) { return false; } @@ -850,10 +858,16 @@ export class DropListRef { * 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) { + _startReceiving(sibling: DropListRef, items: DragRef[]) { const activeSiblings = this._activeSiblings; - if (!activeSiblings.has(sibling)) { + if (!activeSiblings.has(sibling) && items.every(item => { + // Note that we have to add an exception to the `enterPredicate` for items that started off + // in this drop list. The drag ref has logic that allows an item to return to its initial + // container, if it has left the initial container and none of the connected containers + // allow it to enter. See `DragRef._updateActiveDropContainer` for more context. + return this.enterPredicate(item, this) || this._draggables.indexOf(item) > -1; + })) { activeSiblings.add(sibling); this._cacheParentPositions(); this._listenToScrollEvents(); @@ -917,6 +931,12 @@ export class DropListRef { return this._cachedShadowRoot; } + + /** Notifies any siblings that may potentially receive the item. */ + private _notifyReceivingSiblings() { + const draggedItems = this._activeDraggables.filter(item => item.isDragging()); + this._siblings.forEach(sibling => sibling._startReceiving(this, draggedItems)); + } } diff --git a/tools/public_api_guard/cdk/drag-drop.d.ts b/tools/public_api_guard/cdk/drag-drop.d.ts index 7422c8819f87..e955b38c1779 100644 --- a/tools/public_api_guard/cdk/drag-drop.d.ts +++ b/tools/public_api_guard/cdk/drag-drop.d.ts @@ -374,7 +374,7 @@ export declare class DropListRef { x: number; y: number; }): void; - _startReceiving(sibling: DropListRef): void; + _startReceiving(sibling: DropListRef, items: DragRef[]): void; _startScrollingIfNecessary(pointerX: number, pointerY: number): void; _stopReceiving(sibling: DropListRef): void; _stopScrolling(): void;