This defect was already fixed in PR #12624 but this one is going to be merge into 6.4.X.

Leo Rossi · jelbourn · commit 98fc4ddbb620 · 2018-10-04T12:46:29.000-07:00
* Fixes the position of flexible overlays with pushing enabled being thrown off once the user starts scrolling. * Fixes flexible overlays with pushing not handling locked positioning. With these changes locked overlays will only be pushed when they're opened, whereas non-locked overlays will stay in the viewport, even when the user scrolls down. * Fixes a potential issue due to a couple of variables being initialized together where one is set to zero and the other one is undefined. Fixes #11365.
diff --git a/src/cdk/overlay/position/flexible-connected-position-strategy.spec.ts b/src/cdk/overlay/position/flexible-connected-position-strategy.spec.ts
@@ -1134,6 +1134,133 @@ describe('FlexibleConnectedPositionStrategy', () => {
       expect(Math.floor(overlayRect.top)).toBe(15);
     });
 
+    it('should not mess with the left offset when pushing from the top', () => {
+        originElement.style.top = `${-OVERLAY_HEIGHT * 2}px`;
+        originElement.style.left = '200px';
+        positionStrategy.withPositions([{
+            originX: 'start',
+            originY: 'bottom',
+            overlayX: 'start',
+            overlayY: 'top'
+        }]);
+        attachOverlay({positionStrategy});
+        const overlayRect = overlayRef.overlayElement.getBoundingClientRect();
+        expect(Math.floor(overlayRect.left)).toBe(200);
+    });
+    it('should align to the trigger if the overlay is wider than the viewport, but the trigger ' +
+        'is still within the viewport', () => {
+        originElement.style.top = '200px';
+        originElement.style.left = '150px';
+        positionStrategy.withPositions([
+            {
+                originX: 'start',
+                originY: 'bottom',
+                overlayX: 'start',
+                overlayY: 'top'
+            },
+            {
+                originX: 'end',
+                originY: 'bottom',
+                overlayX: 'end',
+                overlayY: 'top'
+            }
+        ]);
+        attachOverlay({
+            width: viewport.getViewportRect().width + 100,
+            positionStrategy
+        });
+        const overlayRect = overlayRef.overlayElement.getBoundingClientRect();
+        const originRect = originElement.getBoundingClientRect();
+        expect(Math.floor(overlayRect.left)).toBe(Math.floor(originRect.left));
+    });
+    it('should push into the viewport if the overlay is wider than the viewport and the trigger' +
+        'out of the viewport', () => {
+        originElement.style.top = '200px';
+        originElement.style.left = `-${DEFAULT_WIDTH / 2}px`;
+        positionStrategy.withPositions([
+            {
+                originX: 'start',
+                originY: 'bottom',
+                overlayX: 'start',
+                overlayY: 'top'
+            },
+            {
+                originX: 'end',
+                originY: 'bottom',
+                overlayX: 'end',
+                overlayY: 'top'
+            }
+        ]);
+        attachOverlay({
+            width: viewport.getViewportRect().width + 100,
+            positionStrategy
+        });
+        const overlayRect = overlayRef.overlayElement.getBoundingClientRect();
+        expect(Math.floor(overlayRect.left)).toBe(0);
+    });
+    it('should keep the element inside the viewport as the user is scrolling, ' +
+        'with position locking disabled', () => {
+        const veryLargeElement = document.createElement('div');
+        originElement.style.top = `${-OVERLAY_HEIGHT * 2}px`;
+        originElement.style.left = '200px';
+        veryLargeElement.style.width = '100%';
+        veryLargeElement.style.height = '2000px';
+        document.body.appendChild(veryLargeElement);
+        positionStrategy
+            .withLockedPosition(false)
+            .withViewportMargin(0)
+            .withPositions([{
+                overlayY: 'top',
+                overlayX: 'start',
+                originY: 'top',
+                originX: 'start'
+            }]);
+        attachOverlay({positionStrategy});
+        let overlayRect = overlayRef.overlayElement.getBoundingClientRect();
+        expect(Math.floor(overlayRect.top))
+            .toBe(0, 'Expected overlay to be in the viewport initially.');
+        window.scroll(0, 100);
+        overlayRef.updatePosition();
+        zone.simulateZoneExit();
+        overlayRect = overlayRef.overlayElement.getBoundingClientRect();
+        expect(Math.floor(overlayRect.top))
+            .toBe(0, 'Expected overlay to stay in the viewport after scrolling.');
+        window.scroll(0, 0);
+        document.body.removeChild(veryLargeElement);
+    });
+    it('should not continue pushing the overlay as the user scrolls, if position ' +
+        'locking is enabled', () => {
+        const veryLargeElement = document.createElement('div');
+        originElement.style.top = `${-OVERLAY_HEIGHT * 2}px`;
+        originElement.style.left = '200px';
+        veryLargeElement.style.width = '100%';
+        veryLargeElement.style.height = '2000px';
+        document.body.appendChild(veryLargeElement);
+        positionStrategy
+            .withLockedPosition()
+            .withViewportMargin(0)
+            .withPositions([{
+                overlayY: 'top',
+                overlayX: 'start',
+                originY: 'top',
+                originX: 'start'
+            }]);
+        attachOverlay({positionStrategy});
+        const scrollBy = 100;
+        let initialOverlayTop = Math.floor(overlayRef.overlayElement.getBoundingClientRect().top);
+        expect(initialOverlayTop).toBe(0, 'Expected overlay to be inside the viewport initially.');
+        window.scroll(0, scrollBy);
+        overlayRef.updatePosition();
+        zone.simulateZoneExit();
+        let currentOverlayTop = Math.floor(overlayRef.overlayElement.getBoundingClientRect().top);
+        expect(currentOverlayTop).toBeLessThan(0,
+            'Expected overlay to no longer be completely inside the viewport.');
+        expect(currentOverlayTop).toBe(initialOverlayTop - scrollBy,
+            'Expected overlay to maintain its previous position.');
+        window.scroll(0, 0);
+        document.body.removeChild(veryLargeElement);
+    });
+
   });
 
   describe('with flexible dimensions', () => {
diff --git a/src/cdk/overlay/position/flexible-connected-position-strategy.ts b/src/cdk/overlay/position/flexible-connected-position-strategy.ts
@@ -8,7 +8,7 @@
 
 import {PositionStrategy} from './position-strategy';
 import {ElementRef} from '@angular/core';
-import {ViewportRuler, CdkScrollable} from '@angular/cdk/scrolling';
+import {ViewportRuler, CdkScrollable, ViewportScrollPosition} from '@angular/cdk/scrolling';
 import {
   ConnectedOverlayPositionChange,
   ConnectionPositionPair,
@@ -112,6 +112,9 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
   /** Amount of subscribers to the `positionChanges` stream. */
   private _positionChangeSubscriptions = 0;
 
+  /** Amount by which the overlay was pushed in each axis during the last time it was positioned. */
+  private _previousPushAmount: {x: number, y: number} | null;
+
   /** Observable sequence of position changes. */
   positionChanges: Observable<ConnectedOverlayPositionChange> = Observable.create(observer => {
     const subscription = this._positionChanges.subscribe(observer);
@@ -152,7 +155,13 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
     this._boundingBox = overlayRef.hostElement;
     this._pane = overlayRef.overlayElement;
     this._resizeSubscription.unsubscribe();
-    this._resizeSubscription = this._viewportRuler.change().subscribe(() => this.apply());
+    this._resizeSubscription = this._viewportRuler.change().subscribe(() => {
+        // When the window is resized, we want to trigger the next reposition as if it
+        // was an initial render, in order for the strategy to pick a new optimal position,
+        // otherwise position locking will cause it to stay at the old one.
+        this._isInitialRender = true;
+        this.apply();
+    });
   }
 
   /**
@@ -282,6 +291,8 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
   }
 
   detach() {
+    this._lastPosition = null;
+    this._previousPushAmount = null;
     this._resizeSubscription.unsubscribe();
   }
 
@@ -541,39 +552,54 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
    * the viewport, the top-left corner will be pushed on-screen (with overflow occuring on the
    * right and bottom).
    *
-   * @param start The starting point from which the overlay is pushed.
-   * @param overlay The overlay dimensions.
+   * @param start Starting point from which the overlay is pushed.
+   * @param overlay Dimensions of the overlay.
+   * @param scrollPosition Current viewport scroll position.
    * @returns The point at which to position the overlay after pushing. This is effectively a new
    *     originPoint.
    */
-  private _pushOverlayOnScreen(start: Point, overlay: ClientRect): Point {
+  private _pushOverlayOnScreen(start: Point,
+                               overlay: ClientRect,
+                               scrollPosition: ViewportScrollPosition): Point {
+    // If the position is locked and we've pushed the overlay already, reuse the previous push
+    // amount, rather than pushing it again. If we were to continue pushing, the element would
+    // remain in the viewport, which goes against the expectations when position locking is enabled.
+    if (this._previousPushAmount && this._positionLocked) {
+        return {
+            x: start.x + this._previousPushAmount.x,
+            y: start.y + this._previousPushAmount.y
+        };
+    }
     const viewport = this._viewportRect;
 
-    // Determine how much the overlay goes outside the viewport on each side, which we'll use to
-    // decide which direction to push it.
+    // Determine how much the overlay goes outside the viewport on each
+    // side, which we'll use to decide which direction to push it.
     const overflowRight = Math.max(start.x + overlay.width - viewport.right, 0);
     const overflowBottom = Math.max(start.y + overlay.height - viewport.bottom, 0);
-    const overflowTop = Math.max(viewport.top - start.y, 0);
-    const overflowLeft = Math.max(viewport.left - start.x, 0);
+    const overflowTop = Math.max(viewport.top - scrollPosition.top - start.y, 0);
+    const overflowLeft = Math.max(viewport.left - scrollPosition.left - start.x, 0);
 
-    // Amount by which to push the overlay in each direction such that it remains on-screen.
-    let pushX, pushY = 0;
+    // Amount by which to push the overlay in each axis such that it remains on-screen.
+    let pushX = 0;
+    let pushY = 0;
 
     // If the overlay fits completely within the bounds of the viewport, push it from whichever
     // direction is goes off-screen. Otherwise, push the top-left corner such that its in the
     // viewport and allow for the trailing end of the overlay to go out of bounds.
-    if (overlay.width <= viewport.width) {
+    if (overlay.width < viewport.width) {
       pushX = overflowLeft || -overflowRight;
     } else {
-      pushX = viewport.left - start.x;
+      pushX = start.x < this._viewportMargin ? (viewport.left - scrollPosition.left) - start.x : 0;
     }
 
-    if (overlay.height <= viewport.height) {
+    if (overlay.height < viewport.height) {
       pushY = overflowTop || -overflowBottom;
     } else {
-      pushY = viewport.top - start.y;
+      pushY = start.y < this._viewportMargin ? (viewport.top - scrollPosition.top) - start.y : 0;
     }
 
+    this._previousPushAmount = {x: pushX, y: pushY};
+
     return {
       x: start.x + pushX,
       y: start.y + pushY,
@@ -792,8 +818,9 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
     const styles = {} as CSSStyleDeclaration;
 
     if (this._hasExactPosition()) {
-      extendStyles(styles, this._getExactOverlayY(position, originPoint));
-      extendStyles(styles, this._getExactOverlayX(position, originPoint));
+      const scrollPosition = this._viewportRuler.getViewportScrollPosition();
+      extendStyles(styles, this._getExactOverlayY(position, originPoint, scrollPosition));
+      extendStyles(styles, this._getExactOverlayX(position, originPoint, scrollPosition));
     } else {
       styles.position = 'static';
     }
@@ -832,14 +859,16 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
   }
 
   /** Gets the exact top/bottom for the overlay when not using flexible sizing or when pushing. */
-  private _getExactOverlayY(position: ConnectedPosition, originPoint: Point) {
+  private _getExactOverlayY(position: ConnectedPosition,
+                            originPoint: Point,
+                            scrollPosition: ViewportScrollPosition) {
     // Reset any existing styles. This is necessary in case the
     // preferred position has changed since the last `apply`.
     let styles = {top: null, bottom: null} as CSSStyleDeclaration;
     let overlayPoint = this._getOverlayPoint(originPoint, this._overlayRect, position);
 
     if (this._isPushed) {
-      overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect);
+      overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect, scrollPosition);
     }
 
     // @breaking-change 7.0.0 Currently the `_overlayContainer` is optional in order to avoid a
@@ -869,14 +898,16 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
   }
 
   /** Gets the exact left/right for the overlay when not using flexible sizing or when pushing. */
-  private _getExactOverlayX(position: ConnectedPosition, originPoint: Point) {
+  private _getExactOverlayX(position: ConnectedPosition,
+                            originPoint: Point,
+                            scrollPosition: ViewportScrollPosition) {
     // Reset any existing styles. This is necessary in case the preferred position has
     // changed since the last `apply`.
     let styles = {left: null, right: null} as CSSStyleDeclaration;
     let overlayPoint = this._getOverlayPoint(originPoint, this._overlayRect, position);
 
     if (this._isPushed) {
-      overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect);
+      overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect, scrollPosition);
     }
 
     // We want to set either `left` or `right` based on whether the overlay wants to appear "before"
diff --git a/src/cdk/scrolling/viewport-ruler.ts b/src/cdk/scrolling/viewport-ruler.ts
@@ -14,6 +14,12 @@ import {auditTime} from 'rxjs/operators';
 /** Time in ms to throttle the resize events by default. */
 export const DEFAULT_RESIZE_TIME = 20;
 
+/** Object that holds the scroll position of the viewport in each direction. */
+export interface ViewportScrollPosition {
+    top: number;
+    left: number;
+}
+
 /**
  * Simple utility for getting the bounds of the browser viewport.
  * @docs-private
@@ -82,7 +88,7 @@ export class ViewportRuler implements OnDestroy {
   }
 
   /** Gets the (top, left) scroll position of the viewport. */
-  getViewportScrollPosition() {
+  getViewportScrollPosition(): ViewportScrollPosition {
     // While we can get a reference to the fake document
     // during SSR, it doesn't have getBoundingClientRect.
     if (!this._platform.isBrowser) {
diff --git a/src/lib/menu/menu-trigger.ts b/src/lib/menu/menu-trigger.ts
@@ -360,6 +360,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
     return new OverlayConfig({
       positionStrategy: this._overlay.position()
           .flexibleConnectedTo(this._element)
+          .withLockedPosition()
           .withTransformOriginOn('.mat-menu-panel'),
       hasBackdrop: this.menu.hasBackdrop == null ? !this.triggersSubmenu() : this.menu.hasBackdrop,
       backdropClass: this.menu.backdropClass || 'cdk-overlay-transparent-backdrop',
