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

* fix(drag-drop): drop-list wrong enter position
Parent rect should be cached again when the new item has been added.
If item is entering from start (left/top) it should be inserted in the first position.
When looking for item index from pointer position exclude right/bottom pixel: that pixel is external.

* Corrected a bug when entering with no active draggables.

* Corrected enter behavior on reversed container
Added unit tests to cover fixed scenarios

* Fixed view-engine broken test

Author: albyrock87

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

@@ -4013,9 +4013,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();
 
@@ -4024,7 +4026,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],
@@ -4203,6 +4205,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();

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

@@ -302,16 +302,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)});
   }
 
@@ -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.
@@ -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);
     });
   }
 

src/dev-app/drag-drop/drag-drop-demo.html

@@ -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>
@@ -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>

src/dev-app/drag-drop/drag-drop-demo.scss

@@ -24,7 +24,8 @@
   display: block;
 
   .demo-list-horizontal & {
-    display: flex;
+    padding: 0 24px;
+    display: inline-flex;
     flex-direction: row;
   }
 }
@@ -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;

src/dev-app/drag-drop/drag-drop-demo.ts

@@ -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) {