Skip to content

Commit ef92091

Browse files
crisbetommalerba
authored andcommitted
fix(drag-drop): not picking up indirect descendant items (#17226)
Fixes that the drop list doesn't pick up drop items that aren't direct descendants. This was put in on purpose before to try and enforce a simpler DOM structure, but it could be limiting in some cases (e.g. if users want to put an `ngSwitch` with an `ng-container` around it). These changes also add some extra logic to ensure that items aren't picked up by more than one ancestor list. Fixes #17047.
1 parent 86a8fee commit ef92091

File tree

2 files changed

+112
-10
lines changed

2 files changed

+112
-10
lines changed

src/cdk/drag-drop/directives/drag.spec.ts

Lines changed: 104 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3358,6 +3358,57 @@ describe('CdkDrag', () => {
33583358
cleanup();
33593359
}));
33603360

3361+
it('should pick up descendants inside of containers', fakeAsync(() => {
3362+
const fixture = createComponent(DraggableInDropZoneWithContainer);
3363+
fixture.detectChanges();
3364+
const dragItems = fixture.componentInstance.dragItems;
3365+
const firstItem = dragItems.first;
3366+
const thirdItemRect = dragItems.toArray()[2].element.nativeElement.getBoundingClientRect();
3367+
3368+
dragElementViaMouse(fixture, firstItem.element.nativeElement,
3369+
thirdItemRect.left + 1, thirdItemRect.top + 1);
3370+
flush();
3371+
fixture.detectChanges();
3372+
3373+
expect(fixture.componentInstance.droppedSpy).toHaveBeenCalledTimes(1);
3374+
3375+
const event = fixture.componentInstance.droppedSpy.calls.mostRecent().args[0];
3376+
3377+
// Assert the event like this, rather than `toHaveBeenCalledWith`, because Jasmine will
3378+
// go into an infinite loop trying to stringify the event, if the test fails.
3379+
expect(event).toEqual({
3380+
previousIndex: 0,
3381+
currentIndex: 2,
3382+
item: firstItem,
3383+
container: fixture.componentInstance.dropInstance,
3384+
previousContainer: fixture.componentInstance.dropInstance,
3385+
isPointerOverContainer: true,
3386+
distance: {x: jasmine.any(Number), y: jasmine.any(Number)}
3387+
});
3388+
}));
3389+
3390+
it('should not pick up items from descendant drop lists', fakeAsync(() => {
3391+
const fixture = createComponent(NestedDropZones);
3392+
fixture.detectChanges();
3393+
const {dragItems, innerList, outerList} = fixture.componentInstance;
3394+
const innerClasses = innerList.nativeElement.classList;
3395+
const outerClasses = outerList.nativeElement.classList;
3396+
const draggingClass = 'cdk-drop-list-dragging';
3397+
3398+
expect(innerClasses).not.toContain(draggingClass,
3399+
'Expected inner list to start off as not dragging.');
3400+
expect(outerClasses).not.toContain(draggingClass,
3401+
'Expected outer list to start off as not dragging.');
3402+
3403+
startDraggingViaMouse(fixture, dragItems.first.element.nativeElement);
3404+
fixture.detectChanges();
3405+
3406+
expect(innerClasses).toContain(draggingClass,
3407+
'Expected inner list to be dragging.');
3408+
expect(outerClasses).not.toContain(draggingClass,
3409+
'Expected outer list to not be dragging.');
3410+
}));
3411+
33613412
});
33623413

33633414
describe('in a connected drop container', () => {
@@ -4561,6 +4612,32 @@ class DraggableInScrollableVerticalDropZone extends DraggableInDropZone {
45614612
}
45624613
}
45634614

4615+
@Component({
4616+
// Note that we need the blank `ngSwitch` below to hit the code path that we're testing.
4617+
template: `
4618+
<div
4619+
cdkDropList
4620+
class="drop-list"
4621+
style="width: 100px; background: pink;"
4622+
[id]="dropZoneId"
4623+
[cdkDropListData]="items"
4624+
(cdkDropListSorted)="sortedSpy($event)"
4625+
(cdkDropListDropped)="droppedSpy($event)">
4626+
<ng-container [ngSwitch]="true">
4627+
<div
4628+
*ngFor="let item of items"
4629+
cdkDrag
4630+
[cdkDragData]="item"
4631+
[cdkDragBoundary]="boundarySelector"
4632+
[style.height.px]="item.height"
4633+
[style.margin-bottom.px]="item.margin"
4634+
style="width: 100%; background: red;">{{item.value}}</div>
4635+
</ng-container>
4636+
</div>
4637+
`
4638+
})
4639+
class DraggableInDropZoneWithContainer extends DraggableInDropZone {}
4640+
45644641
// Use inline blocks here to avoid flexbox issues and not to have to flip floats in rtl.
45654642
const HORIZONTAL_FIXTURE_STYLES = `
45664643
.cdk-drop-list {
@@ -5095,16 +5172,14 @@ class WrappedDropContainerComponent {
50955172
class="container"
50965173
(cdkDragStarted)="containerDragStartedSpy($event)"
50975174
(cdkDragMoved)="containerDragMovedSpy($event)"
5098-
(cdkDragReleased)="containerDragReleasedSpy($event)"
5099-
>
5175+
(cdkDragReleased)="containerDragReleasedSpy($event)">
51005176
<div
51015177
cdkDrag
51025178
class="item"
51035179
#item
51045180
(cdkDragStarted)="itemDragStartedSpy($event)"
51055181
(cdkDragMoved)="itemDragMovedSpy($event)"
5106-
(cdkDragReleased)="itemDragReleasedSpy($event)"
5107-
>
5182+
(cdkDragReleased)="itemDragReleasedSpy($event)">
51085183
</div>
51095184
</div>`
51105185
})
@@ -5120,6 +5195,31 @@ class NestedDragsComponent {
51205195
itemDragReleasedSpy = jasmine.createSpy('item drag released spy');
51215196
}
51225197

5198+
@Component({
5199+
styles: [`
5200+
.drop-list {
5201+
width: 100px;
5202+
background: pink;
5203+
}
5204+
`],
5205+
template: `
5206+
<div cdkDropList class="drop-list" #outerList>
5207+
<div cdkDropList class="drop-list" #innerList>
5208+
<div
5209+
*ngFor="let item of items"
5210+
cdkDrag
5211+
style="width: 100%; background: red; height: ${ITEM_HEIGHT}px;">{{item}}</div>
5212+
</div>
5213+
</div>
5214+
`
5215+
})
5216+
class NestedDropZones {
5217+
@ViewChildren(CdkDrag) dragItems: QueryList<CdkDrag>;
5218+
@ViewChild('outerList', {static: false}) outerList: ElementRef<HTMLElement>;
5219+
@ViewChild('innerList', {static: false}) innerList: ElementRef<HTMLElement>;
5220+
items = ['Zero', 'One', 'Two', 'Three'];
5221+
}
5222+
51235223
/**
51245224
* Drags an element to a position on the page using the mouse.
51255225
* @param fixture Fixture on which to run change detection.

src/cdk/drag-drop/directives/drop-list.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,11 +69,7 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
6969
_dropListRef: DropListRef<CdkDropList<T>>;
7070

7171
/** Draggable items in the container. */
72-
@ContentChildren(CdkDrag, {
73-
// Explicitly set to false since some of the logic below makes assumptions about it.
74-
// The `.withItems` call below should be updated if we ever need to switch this to `true`.
75-
descendants: false
76-
}) _draggables: QueryList<CdkDrag>;
72+
@ContentChildren(CdkDrag, {descendants: true}) _draggables: QueryList<CdkDrag>;
7773

7874
/**
7975
* Other draggable containers that this container is connected to and into which the
@@ -172,7 +168,13 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
172168
this._draggables.changes
173169
.pipe(startWith(this._draggables), takeUntil(this._destroyed))
174170
.subscribe((items: QueryList<CdkDrag>) => {
175-
this._dropListRef.withItems(items.map(drag => drag._dragRef));
171+
this._dropListRef.withItems(items.reduce((filteredItems, drag) => {
172+
if (drag.dropContainer === this) {
173+
filteredItems.push(drag._dragRef);
174+
}
175+
176+
return filteredItems;
177+
}, [] as DragRef[]));
176178
});
177179
}
178180

0 commit comments

Comments
 (0)