Skip to content

fix(drag-drop): drop-list wrong enter position #19116

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.

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 4 commits into from
Apr 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
134 changes: 131 additions & 3 deletions src/cdk/drag-drop/directives/drag.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3916,9 +3916,11 @@ describe('CdkDrag', () => {
const groups = fixture.componentInstance.groupedDragItems.slice();
const element = groups[0][1].element.nativeElement;
const dropInstances = fixture.componentInstance.dropInstances.toArray();
const targetRect = groups[1][2].element.nativeElement.getBoundingClientRect();
const targetRect = groups[1][1].element.nativeElement.getBoundingClientRect();

dragElementViaMouse(fixture, element, targetRect.left + 1, targetRect.top + 1);
// Use coordinates of [1] item corresponding to [2] item
// after dragged item is removed from first container
dragElementViaMouse(fixture, element, targetRect.left + 1, targetRect.top);
flush();
fixture.detectChanges();

Expand All @@ -3927,7 +3929,7 @@ describe('CdkDrag', () => {
expect(event).toBeTruthy();
expect(event).toEqual({
previousIndex: 1,
currentIndex: 3,
currentIndex: 2, // dragged item should replace the [2] item (see comment above)
item: groups[0][1],
container: dropInstances[1],
previousContainer: dropInstances[0],
Expand Down Expand Up @@ -4106,6 +4108,132 @@ describe('CdkDrag', () => {
expect(fixture.componentInstance.droppedSpy).not.toHaveBeenCalled();
}));

it('should update drop zone after element has entered', fakeAsync(() => {
const fixture = createComponent(ConnectedDropZones);

// Make sure there's only one item in the first list.
fixture.componentInstance.todo = ['things'];
fixture.detectChanges();

const dropInstances = fixture.componentInstance.dropInstances.toArray();
const groups = fixture.componentInstance.groupedDragItems;
const dropZones = dropInstances.map(d => d.element.nativeElement);
const item = groups[0][0];
const initialTargetZoneRect = dropZones[1].getBoundingClientRect();
const targetElement = groups[1][groups[1].length - 1].element.nativeElement;
let targetRect = targetElement.getBoundingClientRect();

startDraggingViaMouse(fixture, item.element.nativeElement);

const placeholder = dropZones[0].querySelector('.cdk-drag-placeholder')!;

expect(placeholder).toBeTruthy();

dispatchMouseEvent(document, 'mousemove', targetRect.left + 1, targetRect.top + 1);
fixture.detectChanges();

expect(targetElement.previousSibling === placeholder)
.toBe(true, 'Expected placeholder to be inside second container before last item.');

// Update target rect
targetRect = targetElement.getBoundingClientRect();
expect(initialTargetZoneRect.bottom <= targetRect.top)
.toBe(true, 'Expected target rect to be outside of initial target zone rect');

// Swap with target
dispatchMouseEvent(document, 'mousemove', targetRect.left + 1, targetRect.bottom - 1);
fixture.detectChanges();

// Drop and verify item drop positon and coontainer
dispatchMouseEvent(document, 'mouseup', targetRect.left + 1, targetRect.bottom - 1);
flush();
fixture.detectChanges();

const event = fixture.componentInstance.droppedSpy.calls.mostRecent().args[0];

expect(event).toBeTruthy();
expect(event).toEqual({
previousIndex: 0,
currentIndex: 3,
item: item,
container: dropInstances[1],
previousContainer: dropInstances[0],
isPointerOverContainer: true,
distance: {x: jasmine.any(Number), y: jasmine.any(Number)}
});
}));

it('should enter as first child if entering from top', fakeAsync(() => {
const fixture = createComponent(ConnectedDropZones);

// Make sure there's only one item in the first list.
fixture.componentInstance.todo = ['things'];
fixture.detectChanges();

const groups = fixture.componentInstance.groupedDragItems;
const dropZones = fixture.componentInstance.dropInstances.map(d => d.element.nativeElement);
const item = groups[0][0];

// Add some initial padding as the target drop zone
dropZones[1].style.paddingTop = '10px';

const targetRect = dropZones[1].getBoundingClientRect();

startDraggingViaMouse(fixture, item.element.nativeElement);

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, targetRect.top);
fixture.detectChanges();

expect(dropZones[1].firstElementChild === placeholder)
.toBe(true, 'Expected placeholder to be first child inside second container.');

dispatchMouseEvent(document, 'mouseup');
}));

it('should enter as last child if entering from top in reversed container', fakeAsync(() => {
const fixture = createComponent(ConnectedDropZones);

// Make sure there's only one item in the first list.
fixture.componentInstance.todo = ['things'];
fixture.detectChanges();

const groups = fixture.componentInstance.groupedDragItems;
const dropZones = fixture.componentInstance.dropInstances.map(d => d.element.nativeElement);
const item = groups[0][0];

// Add some initial padding as the target drop zone
const targetDropZoneStyle = dropZones[1].style;
targetDropZoneStyle.paddingTop = '10px';
targetDropZoneStyle.display = 'flex';
targetDropZoneStyle.flexDirection = 'column-reverse';

const targetRect = dropZones[1].getBoundingClientRect();

startDraggingViaMouse(fixture, item.element.nativeElement);

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, targetRect.top);
fixture.detectChanges();

expect(dropZones[1].lastChild === placeholder)
.toBe(true, 'Expected placeholder to be last child inside second container.');

dispatchMouseEvent(document, 'mouseup');
}));

it('should assign a default id on each drop zone', fakeAsync(() => {
const fixture = createComponent(ConnectedDropZones);
fixture.detectChanges();
Expand Down
43 changes: 38 additions & 5 deletions src/cdk/drag-drop/drop-list-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,16 +309,24 @@ export class DropListRef<T = any> {
element.parentElement!.insertBefore(placeholder, element);
activeDraggables.splice(newIndex, 0, item);
} else {
coerceElement(this.element).appendChild(placeholder);
activeDraggables.push(item);
const element = coerceElement(this.element);
if (this._shouldEnterAsFirstChild(pointerX, pointerY)) {
element.insertBefore(placeholder, activeDraggables[0].getRootElement());
activeDraggables.unshift(item);
} else {
element.appendChild(placeholder);
activeDraggables.push(item);
}
}

// The transform needs to be cleared so it doesn't throw off the measurements.
placeholder.style.transform = '';

// Note that the positions were already cached when we called `start` above,
// but we need to refresh them since the amount of items has changed.
// but we need to refresh them since the amount of items has changed and also parent rects.
this._cacheItemPositions();
this._cacheParentPositions();

this.entered.next({item, container: this, currentIndex: this.getItemIndex(item)});
}

Expand Down Expand Up @@ -695,6 +703,31 @@ export class DropListRef<T = any> {
return itemOffset;
}

/**
* Checks if pointer is entering in the first position
* @param pointerX Position of the user's pointer along the X axis.
* @param pointerY Position of the user's pointer along the Y axis.
*/
private _shouldEnterAsFirstChild(pointerX: number, pointerY: number) {
if (!this._activeDraggables.length) {
return false;
}

const itemPositions = this._itemPositions;
const isHorizontal = this._orientation === 'horizontal';

// `itemPositions` are sorted by position while `activeDraggables` are sorted by child index
// check if container is using some sort of "reverse" ordering (eg: flex-direction: row-reverse)
const reversed = itemPositions[0].drag !== this._activeDraggables[0];
if (reversed) {
const lastItemRect = itemPositions[itemPositions.length - 1].clientRect;
return isHorizontal ? pointerX >= lastItemRect.right : pointerY >= lastItemRect.bottom;
} else {
const firstItemRect = itemPositions[0].clientRect;
return isHorizontal ? pointerX <= firstItemRect.left : pointerY <= firstItemRect.top;
}
}

/**
* Gets the index of an item in the drop container, based on the position of the user's pointer.
* @param item Item that is being sorted.
Expand Down Expand Up @@ -726,8 +759,8 @@ export class DropListRef<T = any> {
return isHorizontal ?
// Round these down since most browsers report client rects with
// sub-pixel precision, whereas the pointer coordinates are rounded to pixels.
pointerX >= Math.floor(clientRect.left) && pointerX <= Math.floor(clientRect.right) :
pointerY >= Math.floor(clientRect.top) && pointerY <= Math.floor(clientRect.bottom);
pointerX >= Math.floor(clientRect.left) && pointerX < Math.floor(clientRect.right) :
pointerY >= Math.floor(clientRect.top) && pointerY < Math.floor(clientRect.bottom);
});
}

Expand Down
26 changes: 21 additions & 5 deletions src/dev-app/drag-drop/drag-drop-demo.html
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,31 @@ <h2>Done</h2>
</div>
</div>

<div>
<div cdkDropListGroup>
<div class="demo-list demo-list-horizontal">
<h2>Horizontal list</h2>
<h2>Ages</h2>
<div
cdkDropList
cdkDropListOrientation="horizontal"
(cdkDropListDropped)="drop($event)"
[cdkDropListLockAxis]="axisLock"
[cdkDropListData]="ages">
<div *ngFor="let item of ages" cdkDrag>
{{item}}
<mat-icon cdkDragHandle svgIcon="dnd-move"></mat-icon>
</div>
</div>
</div>

<div class="demo-list demo-list-horizontal" style="text-align: right">
<h2>Preferred Ages</h2>
<div
cdkDropList
cdkDropListOrientation="horizontal"
(cdkDropListDropped)="drop($event)"
[cdkDropListLockAxis]="axisLock"
[cdkDropListData]="horizontalData">
<div *ngFor="let item of horizontalData" cdkDrag>
[cdkDropListData]="preferredAges">
<div *ngFor="let item of preferredAges" cdkDrag>
{{item}}
<mat-icon cdkDragHandle svgIcon="dnd-move"></mat-icon>
</div>
Expand All @@ -55,7 +70,8 @@ <h2>Free dragging</h2>
<h2>Data</h2>
<pre>{{todo.join(', ')}}</pre>
<pre>{{done.join(', ')}}</pre>
<pre>{{horizontalData.join(', ')}}</pre>
<pre>{{ages.join(', ')}}</pre>
<pre>{{preferredAges.join(', ')}}</pre>
</div>

<div>
Expand Down
8 changes: 5 additions & 3 deletions src/dev-app/drag-drop/drag-drop-demo.scss
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
display: block;

.demo-list-horizontal & {
display: flex;
padding: 0 24px;
display: inline-flex;
flex-direction: row;
}
}
Expand All @@ -49,8 +50,9 @@
.demo-list-horizontal & {
border: none;
border-right: solid 1px #ccc;
flex-grow: 1;
flex-basis: 0;
flex: 1 1;
white-space: nowrap;
background-color: #fff;

[dir='rtl'] & {
border-right: none;
Expand Down
9 changes: 6 additions & 3 deletions src/dev-app/drag-drop/drag-drop-demo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,15 @@ export class DragAndDropDemo {
'Check reddit'
];

horizontalData = [
ages = [
'Stone age',
'Bronze age',
'Iron age',
'Middle ages',
'Early modern period',
'Long nineteenth century'
];
preferredAges = [
'Modern period',
'Renaissance'
];

constructor(iconRegistry: MatIconRegistry, sanitizer: DomSanitizer) {
Expand Down