fix(drag-drop): boundary not accounting for parent scrolling

In #18597 some logic was added to update the boundary dimensions when the page is scrolled, however that logic didn't account for the case where a parent element is being scrolled. We were already handling parent elements for the drop list so these changes move the logic into a separate class so it can be reused and fix the issue for the drag item.

Fixes #19086.

Author: crisbeto

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

@@ -96,7 +96,7 @@ describe('CdkDrag', () => {
           const fixture = createComponent(StandaloneDraggable);
           fixture.detectChanges();
 
-          const cleanup = makePageScrollable();
+          const cleanup = makeScrollable();
           const dragElement = fixture.componentInstance.dragElement.nativeElement;
 
           scrollTo(0, 500);
@@ -126,7 +126,7 @@ describe('CdkDrag', () => {
           fixture.detectChanges();
 
           const dragElement = fixture.componentInstance.dragElement.nativeElement;
-          const cleanup = makePageScrollable();
+          const cleanup = makeScrollable();
 
           scrollTo(0, 500);
           expect(dragElement.style.transform).toBeFalsy();
@@ -256,7 +256,7 @@ describe('CdkDrag', () => {
           fixture.detectChanges();
 
           const dragElement = fixture.componentInstance.dragElement.nativeElement;
-          const cleanup = makePageScrollable();
+          const cleanup = makeScrollable();
 
           scrollTo(0, 500);
           expect(dragElement.style.transform).toBeFalsy();
@@ -285,7 +285,7 @@ describe('CdkDrag', () => {
           fixture.detectChanges();
 
           const dragElement = fixture.componentInstance.dragElement.nativeElement;
-          const cleanup = makePageScrollable();
+          const cleanup = makeScrollable();
 
           scrollTo(0, 500);
           expect(dragElement.style.transform).toBeFalsy();
@@ -2034,7 +2034,7 @@ describe('CdkDrag', () => {
 
       const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;
       const list = fixture.componentInstance.dropInstance.element.nativeElement;
-      const cleanup = makePageScrollable();
+      const cleanup = makeScrollable();
       scrollTo(0, 10);
       let listRect = list.getBoundingClientRect(); // Note that we need to measure after scrolling.
 
@@ -2060,6 +2060,40 @@ describe('CdkDrag', () => {
       cleanup();
     }));
 
+    it('should update the boundary if a parent is scrolled while dragging', fakeAsync(() => {
+      const fixture = createComponent(DraggableInScrollableParentContainer);
+      fixture.componentInstance.boundarySelector = '.cdk-drop-list';
+      fixture.detectChanges();
+
+      const container: HTMLElement = fixture.nativeElement.querySelector('.container');
+      const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;
+      const list = fixture.componentInstance.dropInstance.element.nativeElement;
+      const cleanup = makeScrollable('vertical', container);
+      container.scrollTop = 10;
+      let listRect = list.getBoundingClientRect(); // Note that we need to measure after scrolling.
+
+      startDraggingViaMouse(fixture, item);
+      startDraggingViaMouse(fixture, item, listRect.right, listRect.bottom);
+      flush();
+      dispatchMouseEvent(document, 'mousemove', listRect.right, listRect.bottom);
+      fixture.detectChanges();
+
+      const preview = document.querySelector('.cdk-drag-preview')! as HTMLElement;
+      let previewRect = preview.getBoundingClientRect();
+      expect(Math.floor(previewRect.bottom)).toBe(Math.floor(listRect.bottom));
+
+      container.scrollTop = 0;
+      dispatchFakeEvent(container, 'scroll');
+      fixture.detectChanges();
+      listRect = list.getBoundingClientRect(); // We need to update these since we've scrolled.
+      dispatchMouseEvent(document, 'mousemove', listRect.right, listRect.bottom);
+      fixture.detectChanges();
+      previewRect = preview.getBoundingClientRect();
+
+      expect(Math.floor(previewRect.bottom)).toBe(Math.floor(listRect.bottom));
+      cleanup();
+    }));
+
     it('should clear the id from the preview', fakeAsync(() => {
       const fixture = createComponent(DraggableInDropZone);
       fixture.detectChanges();
@@ -2375,7 +2409,7 @@ describe('CdkDrag', () => {
       fakeAsync(() => {
         const fixture = createComponent(DraggableInDropZone);
         fixture.detectChanges();
-        const cleanup = makePageScrollable();
+        const cleanup = makeScrollable();
 
         scrollTo(0, 500);
         assertDownwardSorting(fixture, fixture.componentInstance.dragItems.map(item => {
@@ -2396,7 +2430,7 @@ describe('CdkDrag', () => {
       fakeAsync(() => {
         const fixture = createComponent(DraggableInDropZone);
         fixture.detectChanges();
-        const cleanup = makePageScrollable();
+        const cleanup = makeScrollable();
 
         scrollTo(0, 500);
         assertUpwardSorting(fixture, fixture.componentInstance.dragItems.map(item => {
@@ -2893,7 +2927,7 @@ describe('CdkDrag', () => {
     it('should keep the preview next to the trigger if the page was scrolled', fakeAsync(() => {
       const fixture = createComponent(DraggableInDropZoneWithCustomPreview);
       fixture.detectChanges();
-      const cleanup = makePageScrollable();
+      const cleanup = makeScrollable();
       const item = fixture.componentInstance.dragItems.toArray()[1].element.nativeElement;
 
       startDraggingViaMouse(fixture, item, 50, 50);
@@ -3468,7 +3502,7 @@ describe('CdkDrag', () => {
       const fixture = createComponent(DraggableInDropZone);
       fixture.detectChanges();
 
-      const cleanup = makePageScrollable();
+      const cleanup = makeScrollable();
       const item = fixture.componentInstance.dragItems.first.element.nativeElement;
       const viewportRuler = TestBed.inject(ViewportRuler);
       const viewportSize = viewportRuler.getViewportSize();
@@ -3489,7 +3523,7 @@ describe('CdkDrag', () => {
       const fixture = createComponent(DraggableInDropZone);
       fixture.detectChanges();
 
-      const cleanup = makePageScrollable();
+      const cleanup = makeScrollable();
       const item = fixture.componentInstance.dragItems.first.element.nativeElement;
       const viewportRuler = TestBed.inject(ViewportRuler);
       const viewportSize = viewportRuler.getViewportSize();
@@ -3512,7 +3546,7 @@ describe('CdkDrag', () => {
       const fixture = createComponent(DraggableInDropZone);
       fixture.detectChanges();
 
-      const cleanup = makePageScrollable('horizontal');
+      const cleanup = makeScrollable('horizontal');
       const item = fixture.componentInstance.dragItems.first.element.nativeElement;
       const viewportRuler = TestBed.inject(ViewportRuler);
       const viewportSize = viewportRuler.getViewportSize();
@@ -3533,7 +3567,7 @@ describe('CdkDrag', () => {
       const fixture = createComponent(DraggableInDropZone);
       fixture.detectChanges();
 
-      const cleanup = makePageScrollable('horizontal');
+      const cleanup = makeScrollable('horizontal');
       const item = fixture.componentInstance.dragItems.first.element.nativeElement;
       const viewportRuler = TestBed.inject(ViewportRuler);
       const viewportSize = viewportRuler.getViewportSize();
@@ -3570,7 +3604,7 @@ describe('CdkDrag', () => {
         list.style.margin = '0';
 
         const listRect = list.getBoundingClientRect();
-        const cleanup = makePageScrollable();
+        const cleanup = makeScrollable();
 
         scrollTo(0, viewportRuler.getViewportSize().height * 5);
         list.scrollTop = 50;
@@ -3608,7 +3642,7 @@ describe('CdkDrag', () => {
         list.style.margin = '0';
 
         const listRect = list.getBoundingClientRect();
-        const cleanup = makePageScrollable();
+        const cleanup = makeScrollable();
 
         scrollTo(0, viewportRuler.getViewportSize().height * 5);
         list.scrollTop = 0;
@@ -4684,7 +4718,7 @@ describe('CdkDrag', () => {
       fixture.detectChanges();
 
       // Make the page scrollable and scroll the items out of view.
-      const cleanup = makePageScrollable();
+      const cleanup = makeScrollable();
       scrollTo(0, 4000);
       dispatchFakeEvent(document, 'scroll');
       fixture.detectChanges();
@@ -5890,11 +5924,13 @@ function getElementSibligsByPosition(element: Element, direction: 'top' | 'left'
  * Adds a large element to the page in order to make it scrollable.
  * @returns Function that should be used to clean up after the test is done.
  */
-function makePageScrollable(direction: 'vertical' | 'horizontal' = 'vertical') {
+function makeScrollable(
+  direction: 'vertical' | 'horizontal' = 'vertical',
+  element = document.body) {
   const veryTallElement = document.createElement('div');
   veryTallElement.style.width = direction === 'vertical' ? '100%' : '4000px';
   veryTallElement.style.height = direction === 'vertical' ? '2000px' : '5px';
-  document.body.appendChild(veryTallElement);
+  element.appendChild(veryTallElement);
 
   return () => {
     scrollTo(0, 0);

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

@@ -12,12 +12,12 @@ import {Direction} from '@angular/cdk/bidi';
 import {normalizePassiveListenerOptions} from '@angular/cdk/platform';
 import {coerceBooleanProperty, coerceElement} from '@angular/cdk/coercion';
 import {Subscription, Subject, Observable} from 'rxjs';
-import {startWith} from 'rxjs/operators';
 import {DropListRefInternal as DropListRef} from './drop-list-ref';
 import {DragDropRegistry} from './drag-drop-registry';
 import {extendStyles, toggleNativeDragInteractions} from './drag-styling';
 import {getTransformTransitionDurationInMs} from './transition-duration';
 import {getMutableClientRect, adjustClientRect} from './client-rect';
+import {ParentPositionTracker} from './parent-position-tracker';
 
 /** Object that can be used to configure the behavior of DragRef. */
 export interface DragRefConfig {
@@ -136,8 +136,8 @@ export class DragRef<T = any> {
   /** Index at which the item started in its initial container. */
   private _initialIndex: number;
 
-  /** Cached scroll position on the page when the element was picked up. */
-  private _scrollPosition: {top: number, left: number};
+  /** Cached positions of scrollable parent elements. */
+  private _parentPositions: ParentPositionTracker;
 
   /** Emits when the item is being moved. */
   private _moveEvents = new Subject<{
@@ -305,6 +305,7 @@ export class DragRef<T = any> {
     private _dragDropRegistry: DragDropRegistry<DragRef, DropListRef>) {
 
     this.withRootElement(element);
+    this._parentPositions = new ParentPositionTracker(_document, _viewportRuler);
     _dragDropRegistry.registerDragItem(this);
   }
 
@@ -422,6 +423,7 @@ export class DragRef<T = any> {
     this._disabledHandles.clear();
     this._dropContainer = undefined;
     this._resizeSubscription.unsubscribe();
+    this._parentPositions.clear();
     this._boundaryElement = this._rootElement = this._placeholderTemplate =
         this._previewTemplate = this._anchor = null!;
   }
@@ -702,7 +704,9 @@ export class DragRef<T = any> {
 
     this._toggleNativeDragInteractions();
 
-    if (this._dropContainer) {
+    const dropContainer = this._dropContainer;
+
+    if (dropContainer) {
       const element = this._rootElement;
       const parent = element.parentNode!;
       const preview = this._preview = this._createPreviewElement();
@@ -718,12 +722,16 @@ export class DragRef<T = any> {
       element.style.display = 'none';
       this._document.body.appendChild(parent.replaceChild(placeholder, element));
       getPreviewInsertionPoint(this._document).appendChild(preview);
-      this._dropContainer.start();
-      this._initialContainer = this._dropContainer;
-      this._initialIndex = this._dropContainer.getItemIndex(this);
+      dropContainer.start();
+      this._initialContainer = dropContainer;
+      this._initialIndex = dropContainer.getItemIndex(this);
     } else {
       this._initialContainer = this._initialIndex = undefined!;
     }
+
+    // Important to run after we've called `start` on the parent container
+    // so that it has had time to resolve its scrollable parents.
+    this._parentPositions.cache(dropContainer ? dropContainer.getScrollableParents() : []);
   }
 
   /**
@@ -775,8 +783,8 @@ export class DragRef<T = any> {
     this._removeSubscriptions();
     this._pointerMoveSubscription = this._dragDropRegistry.pointerMove.subscribe(this._pointerMove);
     this._pointerUpSubscription = this._dragDropRegistry.pointerUp.subscribe(this._pointerUp);
-    this._scrollSubscription = this._dragDropRegistry.scroll.pipe(startWith(null)).subscribe(() => {
-      this._updateOnScroll();
+    this._scrollSubscription = this._dragDropRegistry.scroll.subscribe(scrollEvent => {
+      this._updateOnScroll(scrollEvent);
     });
 
     if (this._boundaryElement) {
@@ -1012,8 +1020,9 @@ export class DragRef<T = any> {
     const handleElement = referenceElement === this._rootElement ? null : referenceElement;
     const referenceRect = handleElement ? handleElement.getBoundingClientRect() : elementRect;
     const point = isTouchEvent(event) ? event.targetTouches[0] : event;
-    const x = point.pageX - referenceRect.left - this._scrollPosition.left;
-    const y = point.pageY - referenceRect.top - this._scrollPosition.top;
+    const scrollPosition = this._getViewportScrollPosition();
+    const x = point.pageX - referenceRect.left - scrollPosition.left;
+    const y = point.pageY - referenceRect.top - scrollPosition.top;
 
     return {
       x: referenceRect.left - elementRect.left + x,
@@ -1025,10 +1034,11 @@ export class DragRef<T = any> {
   private _getPointerPositionOnPage(event: MouseEvent | TouchEvent): Point {
     // `touches` will be empty for start/end events so we have to fall back to `changedTouches`.
     const point = isTouchEvent(event) ? (event.touches[0] || event.changedTouches[0]) : event;
+    const scrollPosition = this._getViewportScrollPosition();
 
     return {
-      x: point.pageX - this._scrollPosition.left,
-      y: point.pageY - this._scrollPosition.top
+      x: point.pageX - scrollPosition.left,
+      y: point.pageY - scrollPosition.top
     };
   }
 
@@ -1146,6 +1156,7 @@ export class DragRef<T = any> {
   /** Cleans up any cached element dimensions that we don't need after dragging has stopped. */
   private _cleanupCachedDimensions() {
     this._boundaryRect = this._previewRect = undefined;
+    this._parentPositions.clear();
   }
 
   /**
@@ -1221,19 +1232,21 @@ export class DragRef<T = any> {
   }
 
   /** Updates the internal state of the draggable element when scrolling has occurred. */
-  private _updateOnScroll() {
-    const oldScrollPosition = this._scrollPosition;
-    const currentScrollPosition = this._viewportRuler.getViewportScrollPosition();
+  private _updateOnScroll(event: Event) {
+    const scrollDifference = this._parentPositions.handleScroll(event);
 
     // ClientRect dimensions are based on the page's scroll position so
     // we have to update the cached boundary ClientRect if the user has scrolled.
-    if (oldScrollPosition && this._boundaryRect) {
-      const topDifference = oldScrollPosition.top - currentScrollPosition.top;
-      const leftDifference = oldScrollPosition.left - currentScrollPosition.left;
-      adjustClientRect(this._boundaryRect, topDifference, leftDifference);
+    if (this._boundaryRect && scrollDifference) {
+      adjustClientRect(this._boundaryRect, scrollDifference.top, scrollDifference.left);
     }
+  }
 
-    this._scrollPosition = currentScrollPosition;
+  /** Gets the scroll position of the viewport. */
+  private _getViewportScrollPosition() {
+    const cachedPosition = this._parentPositions.positions.get(this._document);
+    return cachedPosition ? cachedPosition.scrollPosition :
+        this._viewportRuler.getViewportScrollPosition();
   }
 }