fix(drag-drop): throwing off NgFor differ (#12393)

Reworks the drag&drop not to move the element around in the DOM anymore once it's dropped. Moving the element while it's inside an `NgFor` can be dangerous, because Angular may decide to move the element itself, rather than recreate it, in which case the DOM order will be wrong.

Fixes #12388.

Author: crisbeto

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

@@ -340,6 +340,29 @@ describe('CdkDrag', () => {
           .toEqual(['One', 'Two', 'Zero', 'Three']);
     }));
 
+    it('should not move the original element from its initial DOM position', fakeAsync(() => {
+      const fixture = createComponent(DraggableInDropZone);
+      fixture.detectChanges();
+      const root = fixture.nativeElement as HTMLElement;
+      let dragElements = Array.from(root.querySelectorAll('.cdk-drag'));
+
+      expect(dragElements.map(el => el.textContent)).toEqual(['Zero', 'One', 'Two', 'Three']);
+
+      // Stub out the original call so the list doesn't get re-rendered.
+      // We're testing the DOM order explicitly.
+      fixture.componentInstance.droppedSpy.and.callFake(() => {});
+
+      const thirdItemRect = dragElements[2].getBoundingClientRect();
+
+      dragElementViaMouse(fixture, fixture.componentInstance.dragItems.first.element.nativeElement,
+          thirdItemRect.left + 1, thirdItemRect.top + 1);
+      flush();
+      fixture.detectChanges();
+
+      dragElements = Array.from(root.querySelectorAll('.cdk-drag'));
+      expect(dragElements.map(el => el.textContent)).toEqual(['Zero', 'One', 'Two', 'Three']);
+    }));
+
     it('should dispatch the `dropped` event in a horizontal drop zone', fakeAsync(() => {
       const fixture = createComponent(DraggableInHorizontalDropZone);
       fixture.detectChanges();
@@ -677,14 +700,14 @@ describe('CdkDrag', () => {
       const initialRect = item.element.nativeElement.getBoundingClientRect();
       const targetRect = groups[1][2].element.nativeElement.getBoundingClientRect();
 
-      expect(dropZones[0].contains(item.element.nativeElement))
-        .toBe(true, 'Expected placeholder to be inside the first container.');
       dispatchMouseEvent(item.element.nativeElement, 'mousedown');
       fixture.detectChanges();
 
       const placeholder = dropZones[0].querySelector('.cdk-drag-placeholder')!;
 
       expect(placeholder).toBeTruthy();
+      expect(dropZones[0].contains(placeholder))
+          .toBe(true, 'Expected placeholder to be inside the first container.');
 
       dispatchMouseEvent(document, 'mousemove', targetRect.left + 1, targetRect.top + 1);
       fixture.detectChanges();
@@ -708,18 +731,25 @@ describe('CdkDrag', () => {
       const fixture = createComponent(ConnectedDropZones);
       fixture.detectChanges();
 
-      const groups = fixture.componentInstance.groupedDragItems;
+      const groups = fixture.componentInstance.groupedDragItems.slice();
       const element = groups[0][1].element.nativeElement;
-      const dropZones = fixture.componentInstance.dropInstances.map(d => d.element.nativeElement);
+      const dropInstances = fixture.componentInstance.dropInstances.toArray();
       const targetRect = groups[1][2].element.nativeElement.getBoundingClientRect();
 
-      expect(dropZones[0].contains(element)).toBe(true);
-
       dragElementViaMouse(fixture, element, targetRect.left + 1, targetRect.top + 1);
       flush();
       fixture.detectChanges();
 
-      expect(dropZones[1].contains(element)).toBe(true);
+      const event = fixture.componentInstance.droppedSpy.calls.mostRecent().args[0];
+
+      expect(event).toBeTruthy();
+      expect(event).toEqual({
+        previousIndex: 1,
+        currentIndex: 3,
+        item: groups[0][1],
+        container: dropInstances[1],
+        previousContainer: dropInstances[0]
+      });
     }));
 
     it('should not be able to transfer an item into a container that is not in `connectedTo`',
@@ -730,18 +760,25 @@ describe('CdkDrag', () => {
         fixture.componentInstance.dropInstances.forEach(d => d.connectedTo = []);
         fixture.detectChanges();
 
-        const groups = fixture.componentInstance.groupedDragItems;
+        const groups = fixture.componentInstance.groupedDragItems.slice();
         const element = groups[0][1].element.nativeElement;
-        const dropZones = fixture.componentInstance.dropInstances.map(d => d.element.nativeElement);
+        const dropInstances = fixture.componentInstance.dropInstances.toArray();
         const targetRect = groups[1][2].element.nativeElement.getBoundingClientRect();
 
-        expect(dropZones[0].contains(element)).toBe(true);
-
         dragElementViaMouse(fixture, element, targetRect.left + 1, targetRect.top + 1);
         flush();
         fixture.detectChanges();
 
-        expect(dropZones[0].contains(element)).toBe(true);
+        const event = fixture.componentInstance.droppedSpy.calls.mostRecent().args[0];
+
+        expect(event).toBeTruthy();
+        expect(event).toEqual({
+          previousIndex: 1,
+          currentIndex: 1,
+          item: groups[0][1],
+          container: dropInstances[0],
+          previousContainer: dropInstances[0]
+        });
       }));
 
 
@@ -751,20 +788,17 @@ describe('CdkDrag', () => {
 
       const groups = fixture.componentInstance.groupedDragItems;
       const element = groups[0][1].element.nativeElement;
-      const dropZones = fixture.componentInstance.dropInstances.map(d => d.element.nativeElement);
-      const targetRect = dropZones[1].getBoundingClientRect();
-
-      expect(dropZones[0].contains(element)).toBe(true);
+      const dropZone = fixture.componentInstance.dropInstances.toArray()[1].element.nativeElement;
+      const targetRect = dropZone.getBoundingClientRect();
 
       // Drag the element into the drop zone and move it to the top.
       [1, -1].forEach(offset => {
         dragElementViaMouse(fixture, element, targetRect.left + offset, targetRect.top + offset);
         flush();
         fixture.detectChanges();
-        expect(dropZones[1].contains(element)).toBe(true);
       });
 
-      assertDownwardSorting(fixture, Array.from(dropZones[1].querySelectorAll('.cdk-drag')));
+      assertDownwardSorting(fixture, Array.from(dropZone.querySelectorAll('.cdk-drag')));
     }));
 
     it('should be able to return the last item inside its initial container', fakeAsync(() => {
@@ -780,15 +814,16 @@ describe('CdkDrag', () => {
       const initialRect = item.element.nativeElement.getBoundingClientRect();
       const targetRect = groups[1][0].element.nativeElement.getBoundingClientRect();
 
-      expect(dropZones[0].contains(item.element.nativeElement))
-          .toBe(true, 'Expected placeholder to be inside the first container.');
       dispatchMouseEvent(item.element.nativeElement, 'mousedown');
       fixture.detectChanges();
 
       const placeholder = dropZones[0].querySelector('.cdk-drag-placeholder')!;
 
       expect(placeholder).toBeTruthy();
 
+      expect(dropZones[0].contains(placeholder))
+          .toBe(true, 'Expected placeholder to be inside the first container.');
+
       dispatchMouseEvent(document, 'mousemove', targetRect.left + 1, targetRect.top + 1);
       fixture.detectChanges();
 
@@ -820,25 +855,32 @@ describe('CdkDrag', () => {
       const fixture = createComponent(ConnectedDropZones);
       fixture.detectChanges();
 
-      const dropZones = fixture.componentInstance.dropInstances.toArray();
+      const dropInstances = fixture.componentInstance.dropInstances.toArray();
 
-      dropZones[0].id = 'todo';
-      dropZones[1].id = 'done';
-      dropZones[0].connectedTo = ['done'];
-      dropZones[1].connectedTo = ['todo'];
+      dropInstances[0].id = 'todo';
+      dropInstances[1].id = 'done';
+      dropInstances[0].connectedTo = ['done'];
+      dropInstances[1].connectedTo = ['todo'];
       fixture.detectChanges();
 
       const groups = fixture.componentInstance.groupedDragItems;
       const element = groups[0][1].element.nativeElement;
       const targetRect = groups[1][2].element.nativeElement.getBoundingClientRect();
 
-      expect(dropZones[0].element.nativeElement.contains(element)).toBe(true);
-
       dragElementViaMouse(fixture, element, targetRect.left + 1, targetRect.top + 1);
       flush();
       fixture.detectChanges();
 
-      expect(dropZones[1].element.nativeElement.contains(element)).toBe(true);
+      const event = fixture.componentInstance.droppedSpy.calls.mostRecent().args[0];
+
+      expect(event).toBeTruthy();
+      expect(event).toEqual({
+        previousIndex: 1,
+        currentIndex: 3,
+        item: groups[0][1],
+        container: dropInstances[1],
+        previousContainer: dropInstances[0]
+      });
     }));
 
   });

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

@@ -71,6 +71,12 @@ export class CdkDrag<T = any> implements OnDestroy {
   /** Coordinates on the page at which the user picked up the element. */
   private _pickupPositionOnPage: Point;
 
+  /**
+   * Reference to the element that comes after the draggable in the DOM, at the time
+   * it was picked up. Used for restoring its initial position when it's dropped.
+   */
+  private _nextSibling: Node | null;
+
   /**
    * CSS `transform` applied to the element when it isn't being dragged. We need a
    * passive transform in order for the dragged element to retain its new position
@@ -158,6 +164,7 @@ export class CdkDrag<T = any> implements OnDestroy {
       this._removeElement(this.element.nativeElement);
     }
 
+    this._nextSibling = null;
     this._dragDropRegistry.remove(this);
     this._destroyed.next();
     this._destroyed.complete();
@@ -220,6 +227,7 @@ export class CdkDrag<T = any> implements OnDestroy {
       // place will throw off the consumer's `:last-child` selectors. We can't remove the element
       // from the DOM completely, because iOS will stop firing all subsequent events in the chain.
       element.style.display = 'none';
+      this._nextSibling = element.nextSibling;
       this._document.body.appendChild(element.parentNode!.replaceChild(placeholder, element));
       this._document.body.appendChild(preview);
       this.dropContainer.start();
@@ -271,15 +279,25 @@ export class CdkDrag<T = any> implements OnDestroy {
 
   /** Cleans up the DOM artifacts that were added to facilitate the element being dragged. */
   private _cleanupDragArtifacts() {
+    const currentIndex = this._getElementIndexInDom(this._placeholder);
+
+    // Restore the element's visibility and insert it at its old position in the DOM.
+    // It's important that we maintain the position, because moving the element around in the DOM
+    // can throw off `NgFor` which does smart diffing and re-creates elements only when necessary,
+    // while moving the existing elements in all other cases.
+    this.element.nativeElement.style.display = '';
+
+    if (this._nextSibling) {
+      this._nextSibling.parentNode!.insertBefore(this.element.nativeElement, this._nextSibling);
+    } else {
+      this._placeholder.parentNode!.appendChild(this.element.nativeElement);
+    }
+
     this._destroyPreview();
-    this._placeholder.parentNode!.insertBefore(this.element.nativeElement, this._placeholder);
     this._destroyPlaceholder();
-    this.element.nativeElement.style.display = '';
 
     // Re-enter the NgZone since we bound `document` events on the outside.
     this._ngZone.run(() => {
-      const currentIndex = this._getElementIndexInDom();
-
       this.ended.emit({source: this});
       this.dropped.emit({
         item: this,
@@ -368,14 +386,12 @@ export class CdkDrag<T = any> implements OnDestroy {
     return placeholder;
   }
 
-  /** Gets the index of the dragable element, based on its index in the DOM. */
-  private _getElementIndexInDom(): number {
+  /** Gets the index of an element, based on its index in the DOM. */
+  private _getElementIndexInDom(element: HTMLElement): number {
     // Note: we may be able to figure this in memory while sorting, but doing so won't be very
     // reliable when transferring between containers, because the new container doesn't have
     // the proper indices yet. Also this will work better for the case where the consumer
     // isn't using an `ngFor` to render the list.
-    const element = this.element.nativeElement;
-
     if (!element.parentElement) {
       return -1;
     }