fix(cdk/drag-drop): incorrectly sorting element inside dialog with blocked scrolling (#14806)

crisbeto · web-flow · commit 3e1de9dd298e · 2022-02-23T18:24:56.000+01:00
We use the `ViewportRuler` to determine how much a page has been scrolled in order to compensate for it in some of the `ClientRect`-related calculations. This breaks down when the drop list is inside a dialog with blocked scrolling, because scroll blocking works by offsetting the `html` node which in turn throws off the `ViewportRuler`. These changes switches to reading the values off the `window` which won't be thrown off. Fixes #14280.
diff --git a/src/cdk/drag-drop/directives/drag.spec.ts b/src/cdk/drag-drop/directives/drag.spec.ts
@@ -4903,6 +4903,25 @@ describe('CdkDrag', () => {
         .withContext('Expected placeholder to preserve transform when dragging stops.')
         .toBe(true);
     }));
+
+    it('should sort correctly if the <html> node has been offset', fakeAsync(() => {
+      const documentElement = document.documentElement!;
+      const fixture = createComponent(DraggableInDropZone);
+      fixture.detectChanges();
+
+      documentElement.style.position = 'absolute';
+      documentElement.style.top = '-100px';
+
+      assertDownwardSorting(
+        fixture,
+        fixture.componentInstance.dragItems.map(item => {
+          return item.element.nativeElement;
+        }),
+      );
+
+      documentElement.style.position = '';
+      documentElement.style.top = '';
+    }));
   });
 
   describe('in a connected drop container', () => {
diff --git a/src/cdk/drag-drop/drag-ref.ts b/src/cdk/drag-drop/drag-ref.ts
@@ -363,7 +363,7 @@ export class DragRef<T = any> {
     private _dragDropRegistry: DragDropRegistry<DragRef, DropListRef>,
   ) {
     this.withRootElement(element).withParent(_config.parentDragRef || null);
-    this._parentPositions = new ParentPositionTracker(_document, _viewportRuler);
+    this._parentPositions = new ParentPositionTracker(_document);
     _dragDropRegistry.registerDragItem(this);
   }
 
@@ -1461,10 +1461,10 @@ export class DragRef<T = any> {
 
   /** Gets the scroll position of the viewport. */
   private _getViewportScrollPosition() {
-    const cachedPosition = this._parentPositions.positions.get(this._document);
-    return cachedPosition
-      ? cachedPosition.scrollPosition
-      : this._viewportRuler.getViewportScrollPosition();
+    return (
+      this._parentPositions.positions.get(this._document)?.scrollPosition ||
+      this._parentPositions.getViewportScrollPosition()
+    );
   }
 
   /**
diff --git a/src/cdk/drag-drop/drop-list-ref.ts b/src/cdk/drag-drop/drop-list-ref.ts
@@ -230,7 +230,7 @@ export class DropListRef<T = any> {
     this._document = _document;
     this.withScrollableParents([this.element]);
     _dragDropRegistry.registerDropContainer(this);
-    this._parentPositions = new ParentPositionTracker(_document, _viewportRuler);
+    this._parentPositions = new ParentPositionTracker(_document);
   }
 
   /** Removes the drop list functionality from the DOM element. */
diff --git a/src/cdk/drag-drop/parent-position-tracker.ts b/src/cdk/drag-drop/parent-position-tracker.ts
@@ -6,7 +6,6 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ViewportRuler} from '@angular/cdk/scrolling';
 import {_getEventTarget} from '@angular/cdk/platform';
 import {getMutableClientRect, adjustClientRect} from './client-rect';
 
@@ -27,7 +26,7 @@ export class ParentPositionTracker {
     }
   >();
 
-  constructor(private _document: Document, private _viewportRuler: ViewportRuler) {}
+  constructor(private _document: Document) {}
 
   /** Clears the cached positions. */
   clear() {
@@ -38,7 +37,7 @@ export class ParentPositionTracker {
   cache(elements: readonly HTMLElement[]) {
     this.clear();
     this.positions.set(this._document, {
-      scrollPosition: this._viewportRuler.getViewportScrollPosition(),
+      scrollPosition: this.getViewportScrollPosition(),
     });
 
     elements.forEach(element => {
@@ -63,7 +62,7 @@ export class ParentPositionTracker {
     let newLeft: number;
 
     if (target === this._document) {
-      const viewportScrollPosition = this._viewportRuler!.getViewportScrollPosition();
+      const viewportScrollPosition = this.getViewportScrollPosition();
       newTop = viewportScrollPosition.top;
       newLeft = viewportScrollPosition.left;
     } else {
@@ -87,4 +86,14 @@ export class ParentPositionTracker {
 
     return {top: topDifference, left: leftDifference};
   }
+
+  /**
+   * Gets the scroll position of the viewport. Note that we use the scrollX and scrollY directly,
+   * instead of going through the `ViewportRuler`, because the first value the ruler looks at is
+   * the top/left offset of the `document.documentElement` which works for most cases, but breaks
+   * if the element is offset by something like the `BlockScrollStrategy`.
+   */
+  getViewportScrollPosition() {
+    return {top: window.scrollY, left: window.scrollX};
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -230,7 +230,7 @@ export class DropListRef<T = any> {`
`230`	`230`	`this._document = _document;`
`231`	`231`	`this.withScrollableParents([this.element]);`
`232`	`232`	`_dragDropRegistry.registerDropContainer(this);`
`233`		`- this._parentPositions = new ParentPositionTracker(_document, _viewportRuler);`
	`233`	`+ this._parentPositions = new ParentPositionTracker(_document);`
`234`	`234`	`}`
`235`	`235`
`236`	`236`	`/** Removes the drop list functionality from the DOM element. */`