fix(drag-drop): incorrectly laying out items with different height or margins (#13849)

Currently the `CdkDropList` makes some assumptions about the items not having a margin between each other. It also won't lay out and animate items with varying heights, in some cases. The following changes rework the directive to allow it to handle both varying heights and different margins.

Fixes #13483.

Author: crisbeto

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

@@ -1068,6 +1068,92 @@ describe('CdkDrag', () => {
         flush();
       }));
 
+    it('should lay out the elements correctly, when swapping down with a taller element',
+      fakeAsync(() => {
+        const fixture = createComponent(DraggableInDropZone);
+        fixture.detectChanges();
+
+        const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+        const {top, left} = items[0].getBoundingClientRect();
+
+        fixture.componentInstance.items[0].height = ITEM_HEIGHT * 2;
+        fixture.detectChanges();
+
+        startDraggingViaMouse(fixture, items[0], left, top);
+
+        const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+        const target = items[1];
+        const targetRect = target.getBoundingClientRect();
+
+        // Add a few pixels to the top offset so we get some overlap.
+        dispatchMouseEvent(document, 'mousemove', targetRect.left, targetRect.top + 5);
+        fixture.detectChanges();
+
+        expect(placeholder.style.transform).toBe(`translate3d(0px, ${ITEM_HEIGHT}px, 0px)`);
+        expect(target.style.transform).toBe(`translate3d(0px, ${-ITEM_HEIGHT * 2}px, 0px)`);
+
+        dispatchMouseEvent(document, 'mouseup');
+        fixture.detectChanges();
+        flush();
+      }));
+
+    it('should lay out the elements correctly, when swapping up with a taller element',
+      fakeAsync(() => {
+        const fixture = createComponent(DraggableInDropZone);
+        fixture.detectChanges();
+
+        const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+        const {top, left} = items[1].getBoundingClientRect();
+
+        fixture.componentInstance.items[1].height = ITEM_HEIGHT * 2;
+        fixture.detectChanges();
+
+        startDraggingViaMouse(fixture, items[1], left, top);
+
+        const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+        const target = items[0];
+        const targetRect = target.getBoundingClientRect();
+
+        // Add a few pixels to the top offset so we get some overlap.
+        dispatchMouseEvent(document, 'mousemove', targetRect.left, targetRect.bottom - 5);
+        fixture.detectChanges();
+
+        expect(placeholder.style.transform).toBe(`translate3d(0px, ${-ITEM_HEIGHT}px, 0px)`);
+        expect(target.style.transform).toBe(`translate3d(0px, ${ITEM_HEIGHT * 2}px, 0px)`);
+
+        dispatchMouseEvent(document, 'mouseup');
+        fixture.detectChanges();
+        flush();
+      }));
+
+    it('should lay out elements correctly, when swapping an item with margin', fakeAsync(() => {
+      const fixture = createComponent(DraggableInDropZone);
+      fixture.detectChanges();
+
+      const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+      const {top, left} = items[0].getBoundingClientRect();
+
+      fixture.componentInstance.items[0].margin = 12;
+      fixture.detectChanges();
+
+      startDraggingViaMouse(fixture, items[0], left, top);
+
+      const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+      const target = items[1];
+      const targetRect = target.getBoundingClientRect();
+
+      // Add a few pixels to the top offset so we get some overlap.
+      dispatchMouseEvent(document, 'mousemove', targetRect.left, targetRect.top + 5);
+      fixture.detectChanges();
+
+      expect(placeholder.style.transform).toBe(`translate3d(0px, ${ITEM_HEIGHT + 12}px, 0px)`);
+      expect(target.style.transform).toBe(`translate3d(0px, ${-ITEM_HEIGHT - 12}px, 0px)`);
+
+      dispatchMouseEvent(document, 'mouseup');
+      fixture.detectChanges();
+      flush();
+    }));
+
     it('should lay out the elements correctly, if an element skips multiple positions when ' +
       'sorting horizontally', fakeAsync(() => {
         const fixture = createComponent(DraggableInHorizontalDropZone);
@@ -1094,6 +1180,90 @@ describe('CdkDrag', () => {
         flush();
       }));
 
+    it('should lay out the elements correctly, when swapping to the right with a wider element',
+      fakeAsync(() => {
+        const fixture = createComponent(DraggableInHorizontalDropZone);
+        fixture.detectChanges();
+
+        const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+
+        fixture.componentInstance.items[0].width = ITEM_WIDTH * 2;
+        fixture.detectChanges();
+
+        const {top, left} = items[0].getBoundingClientRect();
+        startDraggingViaMouse(fixture, items[0], left, top);
+
+        const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+        const target = items[1];
+        const targetRect = target.getBoundingClientRect();
+
+        dispatchMouseEvent(document, 'mousemove', targetRect.right - 5, targetRect.top);
+        fixture.detectChanges();
+
+        expect(placeholder.style.transform).toBe(`translate3d(${ITEM_WIDTH}px, 0px, 0px)`);
+        expect(target.style.transform).toBe(`translate3d(${-ITEM_WIDTH * 2}px, 0px, 0px)`);
+
+        dispatchMouseEvent(document, 'mouseup');
+        fixture.detectChanges();
+        flush();
+      }));
+
+    it('should lay out the elements correctly, when swapping left with a wider element',
+      fakeAsync(() => {
+        const fixture = createComponent(DraggableInHorizontalDropZone);
+        fixture.detectChanges();
+
+        const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+        const {top, left} = items[1].getBoundingClientRect();
+
+        fixture.componentInstance.items[1].width = ITEM_WIDTH * 2;
+        fixture.detectChanges();
+
+        startDraggingViaMouse(fixture, items[1], left, top);
+
+        const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+        const target = items[0];
+        const targetRect = target.getBoundingClientRect();
+
+        dispatchMouseEvent(document, 'mousemove', targetRect.right - 5, targetRect.top);
+        fixture.detectChanges();
+
+        expect(placeholder.style.transform).toBe(`translate3d(${-ITEM_WIDTH}px, 0px, 0px)`);
+        expect(target.style.transform).toBe(`translate3d(${ITEM_WIDTH * 2}px, 0px, 0px)`);
+
+        dispatchMouseEvent(document, 'mouseup');
+        fixture.detectChanges();
+        flush();
+      }));
+
+    it('should lay out elements correctly, when horizontally swapping an item with margin',
+      fakeAsync(() => {
+        const fixture = createComponent(DraggableInHorizontalDropZone);
+        fixture.detectChanges();
+
+        const items = fixture.componentInstance.dragItems.map(i => i.element.nativeElement);
+        const {top, left} = items[0].getBoundingClientRect();
+
+        fixture.componentInstance.items[0].margin = 12;
+        fixture.detectChanges();
+
+        startDraggingViaMouse(fixture, items[0], left, top);
+
+        const placeholder = document.querySelector('.cdk-drag-placeholder')! as HTMLElement;
+        const target = items[1];
+        const targetRect = target.getBoundingClientRect();
+
+        dispatchMouseEvent(document, 'mousemove', targetRect.right - 5, targetRect.top);
+        fixture.detectChanges();
+
+        expect(placeholder.style.transform).toBe(`translate3d(${ITEM_WIDTH + 12}px, 0px, 0px)`);
+        expect(target.style.transform).toBe(`translate3d(${-ITEM_WIDTH - 12}px, 0px, 0px)`);
+
+        dispatchMouseEvent(document, 'mouseup');
+        fixture.detectChanges();
+        flush();
+      }));
+
     it('should not swap position for tiny pointer movements', fakeAsync(() => {
       const fixture = createComponent(DraggableInDropZone);
       fixture.detectChanges();
@@ -1819,14 +1989,21 @@ class StandaloneDraggableWithMultipleHandles {
         *ngFor="let item of items"
         cdkDrag
         [cdkDragData]="item"
-        style="width: 100%; height: ${ITEM_HEIGHT}px; background: red;">{{item}}</div>
+        [style.height.px]="item.height"
+        [style.margin-bottom.px]="item.margin"
+        style="width: 100%; background: red;">{{item.value}}</div>
     </div>
   `
 })
 class DraggableInDropZone {
   @ViewChildren(CdkDrag) dragItems: QueryList<CdkDrag>;
   @ViewChild(CdkDropList) dropInstance: CdkDropList;
-  items = ['Zero', 'One', 'Two', 'Three'];
+  items = [
+    {value: 'Zero', height: ITEM_HEIGHT, margin: 0},
+    {value: 'One', height: ITEM_HEIGHT, margin: 0},
+    {value: 'Two', height: ITEM_HEIGHT, margin: 0},
+    {value: 'Three', height: ITEM_HEIGHT, margin: 0}
+  ];
   dropZoneId = 'items';
   droppedSpy = jasmine.createSpy('dropped spy').and.callFake((event: CdkDragDrop<string[]>) => {
     moveItemInArray(this.items, event.previousIndex, event.currentIndex);
@@ -1841,13 +2018,12 @@ class DraggableInDropZone {
   `
     .cdk-drop-list {
       display: block;
-      width: 300px;
+      width: 500px;
       background: pink;
       font-size: 0;
     }
 
     .cdk-drag {
-      width: ${ITEM_WIDTH}px;
       height: ${ITEM_HEIGHT}px;
       background: red;
       display: inline-block;
@@ -1859,14 +2035,23 @@ class DraggableInDropZone {
       cdkDropListOrientation="horizontal"
       [cdkDropListData]="items"
       (cdkDropListDropped)="droppedSpy($event)">
-      <div *ngFor="let item of items" cdkDrag>{{item}}</div>
+      <div
+        *ngFor="let item of items"
+        [style.width.px]="item.width"
+        [style.margin-right.px]="item.margin"
+        cdkDrag>{{item.value}}</div>
     </div>
   `
 })
 class DraggableInHorizontalDropZone {
   @ViewChildren(CdkDrag) dragItems: QueryList<CdkDrag>;
   @ViewChild(CdkDropList) dropInstance: CdkDropList;
-  items = ['Zero', 'One', 'Two', 'Three'];
+  items = [
+    {value: 'Zero', width: ITEM_WIDTH, margin: 0},
+    {value: 'One', width: ITEM_WIDTH, margin: 0},
+    {value: 'Two', width: ITEM_WIDTH, margin: 0},
+    {value: 'Three', width: ITEM_WIDTH, margin: 0}
+  ];
   droppedSpy = jasmine.createSpy('dropped spy').and.callFake((event: CdkDragDrop<string[]>) => {
     moveItemInArray(this.items, event.previousIndex, event.currentIndex);
   });

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

@@ -37,6 +37,43 @@ let _uniqueIdCounter = 0;
  */
 const DROP_PROXIMITY_THRESHOLD = 0.05;
 
+/**
+ * Object used to cache the position of a drag list, its items. and siblings.
+ * @docs-private
+ */
+interface PositionCache {
+  /** Cached positions of the items in the list. */
+  items: ItemPositionCacheEntry[];
+  /** Cached positions of the connected lists. */
+  siblings: ListPositionCacheEntry[];
+  /** Dimensions of the list itself. */
+  self: ClientRect;
+}
+
+/**
+ * Entry in the position cache for draggable items.
+ * @docs-private
+ */
+interface ItemPositionCacheEntry {
+  /** Instance of the drag item. */
+  drag: CdkDrag;
+  /** Dimensions of the item. */
+  clientRect: ClientRect;
+  /** Amount by which the item has been moved since dragging started. */
+  offset: number;
+}
+
+/**
+ * Entry in the position cache for drop lists.
+ * @docs-private
+ */
+interface ListPositionCacheEntry {
+  /** Instance of the drop list. */
+  drop: CdkDropList;
+  /** Dimensions of the list. */
+  clientRect: ClientRect;
+}
+
 /** Container that wraps a set of draggable items. */
 @Directive({
   selector: '[cdkDropList], cdk-drop-list',
@@ -118,11 +155,7 @@ export class CdkDropList<T = any> implements OnInit, OnDestroy {
   _dragging = false;
 
   /** Cache of the dimensions of all the items and the sibling containers. */
-  private _positionCache = {
-    items: [] as {drag: CdkDrag, clientRect: ClientRect, offset: number}[],
-    siblings: [] as {drop: CdkDropList, clientRect: ClientRect}[],
-    self: {} as ClientRect
-  };
+  private _positionCache: PositionCache = {items: [], siblings: [], self: {} as ClientRect};
 
   /**
    * Draggable items that are currently active inside the container. Includes the items
@@ -263,12 +296,10 @@ export class CdkDropList<T = any> implements OnInit, OnDestroy {
     this._previousSwap.delta = isHorizontal ? pointerDelta.x : pointerDelta.y;
 
     // How many pixels the item's placeholder should be offset.
-    const itemOffset = isHorizontal ? newPosition.left - currentPosition.left :
-                                      newPosition.top - currentPosition.top;
+    const itemOffset = this._getItemOffsetPx(currentPosition, newPosition, delta);
 
     // How many pixels all the other items should be offset.
-    const siblingOffset = isHorizontal ? currentPosition.width * delta :
-                                         currentPosition.height * delta;
+    const siblingOffset = this._getSiblingOffsetPx(currentIndex, siblings, delta);
 
     // Save the previous order of the items before moving the item to its new index.
     // We use this to check whether an item has been moved as a result of the sorting.
@@ -449,6 +480,59 @@ export class CdkDropList<T = any> implements OnInit, OnDestroy {
     return pointerY > top - yThreshold && pointerY < bottom + yThreshold &&
            pointerX > left - xThreshold && pointerX < right + xThreshold;
   }
+
+  /**
+   * Gets the offset in pixels by which the item that is being dragged should be moved.
+   * @param currentPosition Current position of the item.
+   * @param newPosition Position of the item where the current item should be moved.
+   * @param delta Direction in which the user is moving.
+   */
+  private _getItemOffsetPx(currentPosition: ClientRect, newPosition: ClientRect, delta: 1 | -1) {
+    const isHorizontal = this.orientation === 'horizontal';
+    let itemOffset = isHorizontal ? newPosition.left - currentPosition.left :
+                                    newPosition.top - currentPosition.top;
+
+    // Account for differences in the item width/height.
+    if (delta === -1) {
+      itemOffset += isHorizontal ? newPosition.width - currentPosition.width :
+                                   newPosition.height - currentPosition.height;
+    }
+
+    return itemOffset;
+  }
+
+  /**
+   * Gets the offset in pixels by which the items that aren't being dragged should be moved.
+   * @param currentIndex Index of the item currently being dragged.
+   * @param siblings All of the items in the list.
+   * @param delta Direction in which the user is moving.
+   */
+  private _getSiblingOffsetPx(currentIndex: number,
+                              siblings: ItemPositionCacheEntry[],
+                              delta: 1 | -1) {
+
+    const isHorizontal = this.orientation === 'horizontal';
+    const currentPosition = siblings[currentIndex].clientRect;
+    const immediateSibling = siblings[currentIndex + delta * -1];
+    let siblingOffset = currentPosition[isHorizontal ? 'width' : 'height'] * delta;
+
+    if (immediateSibling) {
+      const start = isHorizontal ? 'left' : 'top';
+      const end = isHorizontal ? 'right' : 'bottom';
+
+      // Get the spacing between the start of the current item and the end of the one immediately
+      // after it in the direction in which the user is dragging, or vice versa. We add it to the
+      // offset in order to push the element to where it will be when it's inline and is influenced
+      // by the `margin` of its siblings.
+      if (delta === -1) {
+        siblingOffset -= immediateSibling.clientRect[start] - currentPosition[end];
+      } else {
+        siblingOffset += currentPosition[start] - immediateSibling.clientRect[end];
+      }
+    }
+
+    return siblingOffset;
+  }
 }