fix(drag-drop): incorrectly sorting element inside dialog with blocked scrolling

crisbeto · crisbeto · commit cf3856c96abc · 2019-08-06T16:36:15.000+03: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
@@ -2028,6 +2028,22 @@ describe('CdkDrag', () => {
       }));
     }));
 
+    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 = '';
+    }));
+
     it('should move the placeholder as an item is being sorted down on a scrolled page',
       fakeAsync(() => {
         const fixture = createComponent(DraggableInDropZone);
diff --git a/src/cdk/drag-drop/drag-drop.ts b/src/cdk/drag-drop/drag-drop.ts
@@ -27,7 +27,8 @@ export class DragDrop {
   constructor(
     @Inject(DOCUMENT) private _document: any,
     private _ngZone: NgZone,
-    private _viewportRuler: ViewportRuler,
+    // @breaking-change 8.0.0 `_viewportRuler` parameter isn't being used. To be removed.
+    _viewportRuler: ViewportRuler,
     private _dragDropRegistry: DragDropRegistry<DragRef, DropListRef>) {}
 
   /**
@@ -38,16 +39,14 @@ export class DragDrop {
   createDrag<T = any>(element: ElementRef<HTMLElement> | HTMLElement,
                 config: DragRefConfig = DEFAULT_CONFIG): DragRef<T> {
 
-    return new DragRef<T>(element, config, this._document, this._ngZone, this._viewportRuler,
-        this._dragDropRegistry);
+    return new DragRef<T>(element, config, this._document, this._ngZone, this._dragDropRegistry);
   }
 
   /**
    * Turns an element into a drop list.
    * @param element Element to which to attach the drop list functionality.
    */
   createDropList<T = any>(element: ElementRef<HTMLElement> | HTMLElement): DropListRef<T> {
-    return new DropListRef<T>(element, this._dragDropRegistry, this._document, this._ngZone,
-        this._viewportRuler);
+    return new DropListRef<T>(element, this._dragDropRegistry, this._document, this._ngZone);
   }
 }
diff --git a/src/cdk/drag-drop/drag-ref.ts b/src/cdk/drag-drop/drag-ref.ts
@@ -7,7 +7,6 @@
  */
 
 import {EmbeddedViewRef, ElementRef, NgZone, ViewContainerRef, TemplateRef} from '@angular/core';
-import {ViewportRuler} from '@angular/cdk/scrolling';
 import {Direction} from '@angular/cdk/bidi';
 import {normalizePassiveListenerOptions} from '@angular/cdk/platform';
 import {coerceBooleanProperty, coerceElement} from '@angular/cdk/coercion';
@@ -278,7 +277,6 @@ export class DragRef<T = any> {
     private _config: DragRefConfig,
     private _document: Document,
     private _ngZone: NgZone,
-    private _viewportRuler: ViewportRuler,
     private _dragDropRegistry: DragDropRegistry<DragRef, DropListRef>) {
 
     this.withRootElement(element);
@@ -726,7 +724,11 @@ export class DragRef<T = any> {
     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._scrollPosition = this._viewportRuler.getViewportScrollPosition();
+      // 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`.
+      this._scrollPosition = {top: window.scrollY, left: window.scrollX};
     });
 
     if (this._boundaryElement) {
diff --git a/tools/public_api_guard/cdk/drag-drop.d.ts b/tools/public_api_guard/cdk/drag-drop.d.ts
@@ -273,7 +273,7 @@ export declare class DragRef<T = any> {
     started: Subject<{
         source: DragRef<any>;
     }>;
-    constructor(element: ElementRef<HTMLElement> | HTMLElement, _config: DragRefConfig, _document: Document, _ngZone: NgZone, _viewportRuler: ViewportRuler, _dragDropRegistry: DragDropRegistry<DragRef, DropListRef>);
+    constructor(element: ElementRef<HTMLElement> | HTMLElement, _config: DragRefConfig, _document: Document, _ngZone: NgZone, _dragDropRegistry: DragDropRegistry<DragRef, DropListRef>);
     _sortFromLastPointerPosition(): void;
     _withDropContainer(container: DropListRef): void;
     disableHandle(handle: HTMLElement): void;
