Skip to content

Commit

Permalink
fix(cdk/drag-drop): account for enterPredicate when setting receiving…
Browse files Browse the repository at this point in the history 
… 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.

(cherry picked from commit 411b174)
  • Loading branch information
@crisbeto @mmalerba
crisbeto authored and mmalerba committed Dec 17, 2020
1 parent 2e13216 commit c535444
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 51 deletions.
92 changes: 63 additions & 29 deletions src/cdk/drag-drop/directives/drag.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
62 changes: 41 additions & 21 deletions src/cdk/drag-drop/drop-list-ref.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export class DropListRef<T = any> {
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
Expand All @@ -163,7 +163,7 @@ export class DropListRef<T = any> {
private _previousSwap = {drag: null as DragRef | null, delta: 0, overlaps: false};

/** Draggable items in the container. */
private _draggables: ReadonlyArray<DragRef>;
private _draggables: ReadonlyArray<DragRef> = [];

/** Drop lists that are connected to the current one. */
private _siblings: ReadonlyArray<DropListRef> = [];
Expand Down Expand Up @@ -240,19 +240,8 @@ export class DropListRef<T = any> {

/** 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();
}

/**
Expand All @@ -264,7 +253,7 @@ export class DropListRef<T = any> {
* 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.
Expand Down Expand Up @@ -323,6 +312,8 @@ export class DropListRef<T = any> {
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)});
}

Expand Down Expand Up @@ -463,7 +454,7 @@ export class DropListRef<T = any> {
_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;
}
Expand Down Expand Up @@ -600,6 +591,22 @@ export class DropListRef<T = any> {
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);
Expand Down Expand Up @@ -802,7 +809,7 @@ export class DropListRef<T = any> {
* @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);
}

/**
Expand All @@ -823,7 +830,8 @@ export class DropListRef<T = any> {
* @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;
}

Expand All @@ -850,10 +858,16 @@ export class DropListRef<T = any> {
* 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();
Expand Down Expand Up @@ -917,6 +931,12 @@ export class DropListRef<T = any> {

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));
}
}


Expand Down
2 changes: 1 addition & 1 deletion tools/public_api_guard/cdk/drag-drop.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ export declare class DropListRef<T = any> {
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;
Expand Down

0 comments on commit c535444

Please sign in to comment.