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

crisbeto · crisbeto · commit 46bc0a9708ff · 2019-02-11T07:49:27.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
@@ -1530,6 +1530,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/directives/drag.ts b/src/cdk/drag-drop/directives/drag.ts
@@ -170,7 +170,7 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
     @Inject(DOCUMENT) private _document: any,
     private _ngZone: NgZone,
     private _viewContainerRef: ViewContainerRef,
-    viewportRuler: ViewportRuler,
+    _viewportRuler: ViewportRuler,
     dragDropRegistry: DragDropRegistry<DragRef, DropListRef>,
     @Inject(CDK_DRAG_CONFIG) config: DragRefConfig,
     @Optional() private _dir: Directionality,
@@ -188,8 +188,7 @@ export class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDestroy {
     if (dragDrop) {
       this._dragRef = dragDrop.createDrag(element, config);
     } else {
-      this._dragRef = new DragRef(element, config, _document, _ngZone, viewportRuler,
-          dragDropRegistry);
+      this._dragRef = new DragRef(element, config, _document, _ngZone, dragDropRegistry);
     }
 
     this._dragRef.data = this;
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,8 +39,7 @@ 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);
   }
 
   /**
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';
@@ -269,7 +268,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);
@@ -661,7 +659,12 @@ export class DragRef<T = any> {
     this._initialContainer = this._dropContainer!;
     this._pointerMoveSubscription = this._dragDropRegistry.pointerMove.subscribe(this._pointerMove);
     this._pointerUpSubscription = this._dragDropRegistry.pointerUp.subscribe(this._pointerUp);
-    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) {
       this._boundaryRect = this._boundaryElement.getBoundingClientRect();
diff --git a/tools/public_api_guard/cdk/drag-drop.d.ts b/tools/public_api_guard/cdk/drag-drop.d.ts
@@ -27,8 +27,8 @@ export declare class CdkDrag<T = any> implements AfterViewInit, OnChanges, OnDes
     started: EventEmitter<CdkDragStart>;
     constructor(
     element: ElementRef<HTMLElement>,
-    dropContainer: CdkDropList, _document: any, _ngZone: NgZone, _viewContainerRef: ViewContainerRef, viewportRuler: ViewportRuler, dragDropRegistry: DragDropRegistry<DragRef, DropListRef>, config: DragRefConfig, _dir: Directionality,
-    dragDrop?: DragDrop, _changeDetectorRef?: ChangeDetectorRef | undefined);
+    dropContainer: CdkDropList, _document: any, _ngZone: NgZone, _viewContainerRef: ViewContainerRef, _viewportRuler: ViewportRuler, dragDropRegistry: DragDropRegistry<DragRef, DropListRef>, config: DragRefConfig, _dir: Directionality,
+     dragDrop?: DragDrop, _changeDetectorRef?: ChangeDetectorRef | undefined);
     getPlaceholderElement(): HTMLElement;
     getRootElement(): HTMLElement;
     ngAfterViewInit(): void;
@@ -242,7 +242,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>);
     _withDropContainer(container: DropListRef): void;
     disableHandle(handle: HTMLElement): void;
     dispose(): void;
