Skip to content

fix(cdk/drag-drop): account for enterPredicate when setting receiving class #21346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 63 additions & 29 deletions src/cdk/drag-drop/directives/drag.spec.ts
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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was removed, because it's conflicting with what we want to do.

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
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
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