mirror of
https://github.com/angular/angular.git
synced 2026-09-14 13:54:52 +08:00
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.
This commit is contained in:
Kristiyan Kostadinov
committed by
mmalerba
mmalerba
parent
0958cbb2b1
commit
1f538416b5
@@ -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]
|
||||
});
|
||||
}));
|
||||
|
||||
});
|
||||
|
||||
@@ -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() {
|
||||
this._destroyPreview();
|
||||
this._placeholder.parentNode!.insertBefore(this.element.nativeElement, this._placeholder);
|
||||
this._destroyPlaceholder();
|
||||
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._destroyPlaceholder();
|
||||
|
||||
// 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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user