Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 29, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 70 additions & 28 deletions src/cdk-experimental/drag-drop/drag.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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`',
Expand All @@ -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]
});
}));


Expand All @@ -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(() => {
Expand All @@ -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();

Expand Down Expand Up @@ -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]
});
}));

});
Expand Down
32 changes: 24 additions & 8 deletions src/cdk-experimental/drag-drop/drag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down