From 0a2b4082805f83c1b69f7c75df59ebcf84c30446 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Thu, 26 Jul 2018 22:17:58 +0200 Subject: [PATCH] fix(drag-drop): throwing off NgFor differ 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. --- src/cdk-experimental/drag-drop/drag.spec.ts | 98 +++++++++++++++------ src/cdk-experimental/drag-drop/drag.ts | 32 +++++-- 2 files changed, 94 insertions(+), 36 deletions(-) diff --git a/src/cdk-experimental/drag-drop/drag.spec.ts b/src/cdk-experimental/drag-drop/drag.spec.ts index d8c70c0ebb38..9423ac7f12b4 100644 --- a/src/cdk-experimental/drag-drop/drag.spec.ts +++ b/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,8 +814,6 @@ 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(); @@ -789,6 +821,9 @@ describe('CdkDrag', () => { 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] + }); })); }); diff --git a/src/cdk-experimental/drag-drop/drag.ts b/src/cdk-experimental/drag-drop/drag.ts index 533b249c102e..b4a5d47591d3 100644 --- a/src/cdk-experimental/drag-drop/drag.ts +++ b/src/cdk-experimental/drag-drop/drag.ts @@ -71,6 +71,12 @@ export class CdkDrag 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 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 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 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 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; }