fix(drag-drop): error if next sibling is removed after drag sequence has started

When a drag sequence is started, we save a reference to the next sibling of the drag element so that we can use it at the end of the sequence to restore the element's original DOM position. This breaks down and throws an error if the next sibling gets removed while the user is dragging. These changes make the approach a bit more robust by inserting a comment node instead.

Fixes #18205.

Author: crisbeto

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

@@ -1850,7 +1850,8 @@ describe('CdkDrag', () => {
           },
           removeEventListener: function() {
             document.addEventListener.apply(document, arguments);
-          }
+          },
+          createComment: (text: string) => document.createComment(text)
         };
         const fixture = createComponent(DraggableInDropZone, [{
           provide: DOCUMENT,
@@ -4403,6 +4404,31 @@ describe('CdkDrag', () => {
         });
       }));
 
+      it('should not throw if its next sibling is removed while dragging', fakeAsync(() => {
+        const fixture = createComponent(ConnectedDropZonesWithSingleItems);
+        fixture.detectChanges();
+
+        const items = fixture.componentInstance.dragItems.toArray();
+        const item = items[0];
+        const nextSibling = items[1].element.nativeElement;
+        const extraSibling = document.createElement('div');
+        const targetRect = nextSibling.getBoundingClientRect();
+
+        // Manually insert an element after the node to simulate an external package.
+        nextSibling.parentNode!.insertBefore(extraSibling, nextSibling);
+
+        dragElementViaMouse(fixture, item.element.nativeElement,
+          targetRect.left + 1, targetRect.top + 1);
+
+        // Remove the extra node after the element was dropped, but before the animation is over.
+        extraSibling.parentNode!.removeChild(extraSibling);
+
+        expect(() => {
+          flush();
+          fixture.detectChanges();
+        }).not.toThrow();
+      }));
+
   });
 
   describe('with nested drags', () => {

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

@@ -87,10 +87,10 @@ export class DragRef<T = any> {
   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.
+   * Anchor node used to save the place in the DOM where the element was
+   * picked up so that it can be restored at the end of the drag sequence.
    */
-  private _nextSibling: Node | null;
+  private _anchor: Comment;
 
   /**
    * CSS `transform` applied to the element when it isn't being dragged. We need a
@@ -374,9 +374,10 @@ export class DragRef<T = any> {
     if (this.isDragging()) {
       // Since we move out the element to the end of the body while it's being
       // dragged, we have to make sure that it's removed if it gets destroyed.
-      removeElement(this._rootElement);
+      removeNode(this._rootElement);
     }
 
+    removeNode(this._anchor);
     this._destroyPreview();
     this._destroyPlaceholder();
     this._dragDropRegistry.removeDragItem(this);
@@ -394,7 +395,7 @@ export class DragRef<T = any> {
     this._dropContainer = undefined;
     this._resizeSubscription.unsubscribe();
     this._boundaryElement = this._rootElement = this._placeholderTemplate =
-        this._previewTemplate = this._nextSibling = null!;
+        this._previewTemplate = this._anchor = null!;
   }
 
   /** Checks whether the element is currently being dragged. */
@@ -481,7 +482,7 @@ export class DragRef<T = any> {
   /** Destroys the preview element and its ViewRef. */
   private _destroyPreview() {
     if (this._preview) {
-      removeElement(this._preview);
+      removeNode(this._preview);
     }
 
     if (this._previewRef) {
@@ -494,7 +495,7 @@ export class DragRef<T = any> {
   /** Destroys the placeholder element and its ViewRef. */
   private _destroyPlaceholder() {
     if (this._placeholder) {
-      removeElement(this._placeholder);
+      removeNode(this._placeholder);
     }
 
     if (this._placeholderRef) {
@@ -672,19 +673,19 @@ export class DragRef<T = any> {
 
     if (this._dropContainer) {
       const element = this._rootElement;
-
-      // Grab the `nextSibling` before the preview and placeholder
-      // have been created so we don't get the preview by accident.
-      this._nextSibling = element.nextSibling;
-
+      const parent = element.parentNode!;
       const preview = this._preview = this._createPreviewElement();
       const placeholder = this._placeholder = this._createPlaceholderElement();
+      const anchor = this._anchor = this._anchor || this._document.createComment('');
+
+      // Insert an anchor node so that we can restore the element's position in the DOM.
+      parent.insertBefore(anchor, element);
 
       // We move the element out at the end of the body and we make it hidden, because keeping it in
       // 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._document.body.appendChild(element.parentNode!.replaceChild(placeholder, element));
+      this._document.body.appendChild(parent.replaceChild(placeholder, element));
       getPreviewInsertionPoint(this._document).appendChild(preview);
       this._dropContainer.start();
     }
@@ -767,12 +768,7 @@ export class DragRef<T = any> {
     // 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._rootElement.style.display = '';
-
-    if (this._nextSibling) {
-      this._nextSibling.parentNode!.insertBefore(this._rootElement, this._nextSibling);
-    } else {
-      coerceElement(this._initialContainer.element).appendChild(this._rootElement);
-    }
+    this._anchor.parentNode!.replaceChild(this._rootElement, this._anchor);
 
     this._destroyPreview();
     this._destroyPlaceholder();
@@ -1236,12 +1232,12 @@ function clamp(value: number, min: number, max: number) {
 }
 
 /**
- * Helper to remove an element from the DOM and to do all the necessary null checks.
- * @param element Element to be removed.
+ * Helper to remove a node from the DOM and to do all the necessary null checks.
+ * @param node Node to be removed.
  */
-function removeElement(element: HTMLElement | null) {
-  if (element && element.parentNode) {
-    element.parentNode.removeChild(element);
+function removeNode(node: Node | null) {
+  if (node && node.parentNode) {
+    node.parentNode.removeChild(node);
   }
 }