Skip to content

Commit

Permalink
fix(drag-drop): not working correctly inside transplanted views
Browse files Browse the repository at this point in the history
Currently the CDK drag&drop is set up so that it keeps track of the items inside each drop list via `ContentChildren`, however this doesn't work properly inside of a transplanted view (e.g. in the header of a `mat-table`). There are two main problems inside transplanted views:

1. On the first change detection the `ContentChildren` query may be empty, even though the items exist. Usually they're fine on the second run, but that may be too late, because the user could've started dragging already.
2. Even if we somehow ensured that the `ContentChildren` is up-to-date when dragging starts, Angular won't update the order of the transplanted view items in the query once they're shuffled around.

To work around these limitations and to make it possible to support more advanced usages like in `mat-table`, these changes switch to keeping track of the items using DI. Whenever an item is created or destroyed, it registers/deregisters itself with the closest drop list. Since the insertion order can be different from the DOM order, we use `compareDocumentPosition` to sort the items right before dragging starts.

Fixes angular#18482.
  • Loading branch information
crisbeto committed Feb 13, 2020
1 parent fd1593d commit 60cd68e
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 39 deletions.
43 changes: 42 additions & 1 deletion src/cdk/drag-drop/directives/drag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1358,6 +1358,47 @@ describe('CdkDrag', () => {
expect(fixture.componentInstance.dropInstance.data).toBe(fixture.componentInstance.items);
});

it('should register an item with the drop container', () => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
const list = fixture.componentInstance.dropInstance;

spyOn(list, 'addItem').and.callThrough();

fixture.componentInstance.items.push({value: 'Extra', margin: 0, height: ITEM_HEIGHT});
fixture.detectChanges();

expect(list.addItem).toHaveBeenCalledTimes(1);
});

it('should remove an item from the drop container', () => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
const list = fixture.componentInstance.dropInstance;

spyOn(list, 'removeItem').and.callThrough();

fixture.componentInstance.items.pop();
fixture.detectChanges();

expect(list.removeItem).toHaveBeenCalledTimes(1);
});

it('should return the items sorted by their position in the DOM', () => {
const fixture = createComponent(DraggableInDropZone);
const items = fixture.componentInstance.items;
fixture.detectChanges();

// Insert a couple of items in the start and the middle so the list gets shifted around.
items.unshift({value: 'Extra 0', margin: 0, height: ITEM_HEIGHT});
items.splice(3, 0, {value: 'Extra 1', margin: 0, height: ITEM_HEIGHT});
fixture.detectChanges();

expect(fixture.componentInstance.dropInstance.getSortedItems().map(item => {
return item.element.nativeElement.textContent!.trim();
})).toEqual(['Extra 0', 'Zero', 'One', 'Extra 1', 'Two', 'Three']);
});

it('should sync the drop list inputs with the drop list ref', () => {
const fixture = createComponent(DraggableInDropZone);
fixture.detectChanges();
Expand Down Expand Up @@ -5167,7 +5208,7 @@ class ConnectedDropZones implements AfterViewInit {
this.groupedDragItems.push([]);
}

this.groupedDragItems[index].push(...dropZone._draggables.toArray());
this.groupedDragItems[index].push(...dropZone.getSortedItems());
});
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/cdk/drag-drop/directives/drag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
// assigning the drop container both from here and the list.
if (dropContainer) {
this._dragRef._withDropContainer(dropContainer._dropListRef);
dropContainer.addItem(this);
}

this._syncInputs(this._dragRef);
Expand Down Expand Up @@ -303,6 +304,10 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
}

ngOnDestroy() {
if (this.dropContainer) {
this.dropContainer.removeItem(this);
}

this._destroyed.next();
this._destroyed.complete();
this._dragRef.dispose();
Expand Down
60 changes: 45 additions & 15 deletions src/cdk/drag-drop/directives/drop-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@

import {BooleanInput, coerceArray, coerceBooleanProperty} from '@angular/cdk/coercion';
import {
ContentChildren,
ElementRef,
EventEmitter,
Input,
OnDestroy,
Output,
QueryList,
Optional,
Directive,
ChangeDetectorRef,
Expand Down Expand Up @@ -71,9 +69,6 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
/** Reference to the underlying drop list instance. */
_dropListRef: DropListRef<CdkDropList<T>>;

/** Draggable items in the container. */
@ContentChildren(CdkDrag, {descendants: true}) _draggables: QueryList<CdkDrag>;

/**
* Other draggable containers that this container is connected to and into which the
* container's items can be transferred. Can either be references to other drop containers,
Expand Down Expand Up @@ -147,6 +142,15 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
@Output('cdkDropListSorted')
sorted: EventEmitter<CdkDragSortEvent<T>> = new EventEmitter<CdkDragSortEvent<T>>();

/**
* Keeps track of the items that are registered with this container. Historically we used to
* do this with a `ContentChildren` query, however queries don't handle transplanted views very
* well which means that we can't handle cases like dragging the headers of a `mat-table`
* correctly. What we do instead is to have the items register themselves with the container
* and then we sort them based on their position in the DOM.
*/
private _unsortedItems = new Set<CdkDrag>();

constructor(
/** Element that the drop list is attached to. */
public element: ElementRef<HTMLElement>, dragDrop: DragDrop,
Expand Down Expand Up @@ -187,18 +191,37 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
.map(scrollable => scrollable.getElementRef().nativeElement);
this._dropListRef.withScrollableParents(scrollableParents);
}
}

this._draggables.changes
.pipe(startWith(this._draggables), takeUntil(this._destroyed))
.subscribe((items: QueryList<CdkDrag>) => {
this._dropListRef.withItems(items.reduce((filteredItems, drag) => {
if (drag.dropContainer === this) {
filteredItems.push(drag._dragRef);
}
/** Registers an items with the drop list. */
addItem(item: CdkDrag): void {
this._unsortedItems.add(item);

return filteredItems;
}, [] as DragRef[]));
});
if (this._dropListRef.isDragging()) {
this._syncItemsWithRef();
}
}

/** Removes an item from the drop list. */
removeItem(item: CdkDrag): void {
this._unsortedItems.delete(item);

if (this._dropListRef.isDragging()) {
this._syncItemsWithRef();
}
}

/** Gets the registered items in the list, sorted by their position in the DOM. */
getSortedItems(): CdkDrag[] {
return Array.from(this._unsortedItems).sort((a: CdkDrag, b: CdkDrag) => {
const documentPosition =
a._dragRef.getVisibleElement().compareDocumentPosition(b._dragRef.getVisibleElement());

// `compareDocumentPosition` return a bitmask so we have to use a bitwise operator. See:
// https://developer.mozilla.org/en-US/docs/Web/API/Node/compareDocumentPosition
// tslint:disable-next-line:no-bitwise
return documentPosition & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1;
});
}

ngOnDestroy() {
Expand All @@ -212,6 +235,7 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
this._group._items.delete(this);
}

this._unsortedItems.clear();
this._dropListRef.dispose();
this._destroyed.next();
this._destroyed.complete();
Expand Down Expand Up @@ -310,6 +334,7 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
/** Handles events from the underlying DropListRef. */
private _handleEvents(ref: DropListRef<CdkDropList>) {
ref.beforeStarted.subscribe(() => {
this._syncItemsWithRef();
this._changeDetectorRef.markForCheck();
});

Expand Down Expand Up @@ -371,6 +396,11 @@ export class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
}
}

/** Syncs up the registered drag items with underlying drop list ref. */
private _syncItemsWithRef() {
this._dropListRef.withItems(this.getSortedItems().map(item => item._dragRef));
}

static ngAcceptInputType_disabled: BooleanInput;
static ngAcceptInputType_sortingDisabled: BooleanInput;
static ngAcceptInputType_autoScrollDisabled: BooleanInput;
Expand Down
26 changes: 22 additions & 4 deletions src/cdk/drag-drop/drag-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,9 @@ export class DragRef<T = any> {
/** Drop container in which the DragRef resided when dragging began. */
private _initialContainer: DropListRef;

/** Index at which the item started in its initial container. */
private _initialIndex: number;

/** Cached scroll position on the page when the element was picked up. */
private _scrollPosition: {top: number, left: number};

Expand Down Expand Up @@ -309,6 +312,14 @@ export class DragRef<T = any> {
return this._rootElement;
}

/**
* Gets the currently-visible element that represents the drag item.
* While dragging this is the placeholder, otherwise it's the root element.
*/
getVisibleElement(): HTMLElement {
return this.isDragging() ? this.getPlaceholderElement() : this.getRootElement();
}

/** Registers the handles that can be used to drag the element. */
withHandles(handles: (HTMLElement | ElementRef<HTMLElement>)[]): this {
this._handles = handles.map(handle => coerceElement(handle));
Expand Down Expand Up @@ -697,6 +708,10 @@ export class DragRef<T = any> {
this._document.body.appendChild(parent.replaceChild(placeholder, element));
getPreviewInsertionPoint(this._document).appendChild(preview);
this._dropContainer.start();
this._initialContainer = this._dropContainer;
this._initialIndex = this._dropContainer.getItemIndex(this);
} else {
this._initialContainer = this._initialIndex = undefined!;
}
}

Expand Down Expand Up @@ -743,7 +758,6 @@ export class DragRef<T = any> {
}

this._hasStartedDragging = this._hasMoved = false;
this._initialContainer = this._dropContainer!;

// Avoid multiple subscriptions and memory leaks when multi touch
// (isDragging check above isn't enough because of possible temporal and/or dimensional delays)
Expand Down Expand Up @@ -796,13 +810,14 @@ export class DragRef<T = any> {
this.dropped.next({
item: this,
currentIndex,
previousIndex: this._initialContainer.getItemIndex(this),
previousIndex: this._initialIndex,
container: container,
previousContainer: this._initialContainer,
isPointerOverContainer,
distance
});
container.drop(this, currentIndex, this._initialContainer, isPointerOverContainer, distance);
container.drop(this, currentIndex, this._initialContainer, isPointerOverContainer, distance,
this._initialIndex);
this._dropContainer = this._initialContainer;
});
}
Expand Down Expand Up @@ -831,7 +846,10 @@ export class DragRef<T = any> {
this._dropContainer!.exit(this);
// Notify the new container that the item has entered.
this._dropContainer = newContainer!;
this._dropContainer.enter(this, x, y);
this._dropContainer.enter(this, x, y,
// If we're re-entering the initial container,
// put the into its starting index to begin with.
newContainer === this._initialContainer ? this._initialIndex : undefined);
this.entered.next({
item: this,
container: newContainer!,
Expand Down
42 changes: 27 additions & 15 deletions src/cdk/drag-drop/drop-list-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,18 +256,26 @@ export class DropListRef<T = any> {
* @param item Item that was moved into the container.
* @param pointerX Position of the item along the X axis.
* @param pointerY Position of the item along the Y axis.
* @param index Index at which the item entered. If omitted, the container will try to figure it
* out automatically.
*/
enter(item: DragRef, pointerX: number, pointerY: number): void {
enter(item: DragRef, pointerX: number, pointerY: number, index?: number): void {
this.start();

// If sorting is disabled, we want the item to return to its starting
// position if the user is returning it to its initial container.
let newIndex = this.sortingDisabled ? this._draggables.indexOf(item) : -1;
let newIndex: number;

if (newIndex === -1) {
// We use the coordinates of where the item entered the drop
// zone to figure out at which index it should be inserted.
newIndex = this._getItemIndexFromPointerPosition(item, pointerX, pointerY);
if (index == null) {
newIndex = this.sortingDisabled ? this._draggables.indexOf(item) : -1;

if (newIndex === -1) {
// We use the coordinates of where the item entered the drop
// zone to figure out at which index it should be inserted.
newIndex = this._getItemIndexFromPointerPosition(item, pointerX, pointerY);
}
} else {
newIndex = index;
}

const activeDraggables = this._activeDraggables;
Expand Down Expand Up @@ -325,14 +333,22 @@ export class DropListRef<T = any> {
* @param isPointerOverContainer Whether the user's pointer was over the
* container when the item was dropped.
* @param distance Distance the user has dragged since the start of the dragging sequence.
* @param previousIndex Index of the item when dragging started.
*
* @breaking-change 11.0.0 `previousIndex` parameter to become required.
*/
drop(item: DragRef, currentIndex: number, previousContainer: DropListRef,
isPointerOverContainer: boolean, distance: Point): void {
isPointerOverContainer: boolean, distance: Point, previousIndex?: number): void {
this._reset();
this.dropped.next({
item,

// @breaking-change 11.0.0 Remove this fallback logic once `previousIndex` is a required param.
if (previousIndex == null) {
previousIndex = previousContainer.getItemIndex(item);
}

this.dropped.next({item,
currentIndex,
previousIndex: previousContainer.getItemIndex(item),
previousIndex,
container: this,
previousContainer,
isPointerOverContainer,
Expand Down Expand Up @@ -591,11 +607,7 @@ export class DropListRef<T = any> {
const isHorizontal = this._orientation === 'horizontal';

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 elementToMeasure = drag.getVisibleElement();
return {drag, offset: 0, clientRect: getMutableClientRect(elementToMeasure)};
}).sort((a, b) => {
return isHorizontal ? a.clientRect.left - b.clientRect.left :
Expand Down
11 changes: 7 additions & 4 deletions tools/public_api_guard/cdk/drag-drop.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ export interface CdkDragStart<T = any> {
}

export declare class CdkDropList<T = any> implements AfterContentInit, OnDestroy {
_draggables: QueryList<CdkDrag>;
_dropListRef: DropListRef<CdkDropList<T>>;
autoScrollDisabled: boolean;
connectedTo: (CdkDropList | string)[] | CdkDropList | string;
Expand All @@ -163,17 +162,20 @@ export declare class CdkDropList<T = any> implements AfterContentInit, OnDestroy
constructor(
element: ElementRef<HTMLElement>, dragDrop: DragDrop, _changeDetectorRef: ChangeDetectorRef, _dir?: Directionality | undefined, _group?: CdkDropListGroup<CdkDropList<any>> | undefined,
_scrollDispatcher?: ScrollDispatcher | undefined, config?: DragDropConfig);
addItem(item: CdkDrag): void;
drop(item: CdkDrag, currentIndex: number, previousContainer: CdkDropList, isPointerOverContainer: boolean): void;
enter(item: CdkDrag, pointerX: number, pointerY: number): void;
exit(item: CdkDrag): void;
getItemIndex(item: CdkDrag): number;
getSortedItems(): CdkDrag[];
ngAfterContentInit(): void;
ngOnDestroy(): void;
removeItem(item: CdkDrag): void;
start(): void;
static ngAcceptInputType_autoScrollDisabled: BooleanInput;
static ngAcceptInputType_disabled: BooleanInput;
static ngAcceptInputType_sortingDisabled: BooleanInput;
static ɵdir: i0.ɵɵDirectiveDefWithMeta<CdkDropList<any>, "[cdkDropList], cdk-drop-list", ["cdkDropList"], { "connectedTo": "cdkDropListConnectedTo"; "data": "cdkDropListData"; "orientation": "cdkDropListOrientation"; "id": "id"; "lockAxis": "cdkDropListLockAxis"; "disabled": "cdkDropListDisabled"; "sortingDisabled": "cdkDropListSortingDisabled"; "enterPredicate": "cdkDropListEnterPredicate"; "autoScrollDisabled": "cdkDropListAutoScrollDisabled"; }, { "dropped": "cdkDropListDropped"; "entered": "cdkDropListEntered"; "exited": "cdkDropListExited"; "sorted": "cdkDropListSorted"; }, ["_draggables"]>;
static ɵdir: i0.ɵɵDirectiveDefWithMeta<CdkDropList<any>, "[cdkDropList], cdk-drop-list", ["cdkDropList"], { "connectedTo": "cdkDropListConnectedTo"; "data": "cdkDropListData"; "orientation": "cdkDropListOrientation"; "id": "id"; "lockAxis": "cdkDropListLockAxis"; "disabled": "cdkDropListDisabled"; "sortingDisabled": "cdkDropListSortingDisabled"; "enterPredicate": "cdkDropListEnterPredicate"; "autoScrollDisabled": "cdkDropListAutoScrollDisabled"; }, { "dropped": "cdkDropListDropped"; "entered": "cdkDropListEntered"; "exited": "cdkDropListExited"; "sorted": "cdkDropListSorted"; }, never>;
static ɵfac: i0.ɵɵFactoryDef<CdkDropList<any>>;
}

Expand Down Expand Up @@ -298,6 +300,7 @@ export declare class DragRef<T = any> {
getFreeDragPosition(): Readonly<Point>;
getPlaceholderElement(): HTMLElement;
getRootElement(): HTMLElement;
getVisibleElement(): HTMLElement;
isDragging(): boolean;
reset(): void;
setFreeDragPosition(value: Point): this;
Expand Down Expand Up @@ -368,8 +371,8 @@ export declare class DropListRef<T = any> {
_stopScrolling(): void;
connectedTo(connectedTo: DropListRef[]): this;
dispose(): void;
drop(item: DragRef, currentIndex: number, previousContainer: DropListRef, isPointerOverContainer: boolean, distance: Point): void;
enter(item: DragRef, pointerX: number, pointerY: number): void;
drop(item: DragRef, currentIndex: number, previousContainer: DropListRef, isPointerOverContainer: boolean, distance: Point, previousIndex?: number): void;
enter(item: DragRef, pointerX: number, pointerY: number, index?: number): void;
exit(item: DragRef): void;
getItemIndex(item: DragRef): number;
isDragging(): boolean;
Expand Down

0 comments on commit 60cd68e

Please sign in to comment.