From 6c0fe8d0c1755d1fa39b0d653c0f592cda9aac41 Mon Sep 17 00:00:00 2001 From: Leo Rossi Date: Sat, 22 Sep 2018 12:13:24 -0300 Subject: [PATCH] This defect was already fixed in PR #12624 but this one is going to be merge into 6.4.X. * 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. --- ...exible-connected-position-strategy.spec.ts | 127 ++++++++++++++++++ .../flexible-connected-position-strategy.ts | 73 +++++++--- src/cdk/scrolling/viewport-ruler.ts | 8 +- src/lib/menu/menu-trigger.ts | 1 + 4 files changed, 187 insertions(+), 22 deletions(-) diff --git a/src/cdk/overlay/position/flexible-connected-position-strategy.spec.ts b/src/cdk/overlay/position/flexible-connected-position-strategy.spec.ts index 4f30a0b485c2..4c559ed68988 100644 --- 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 index 75791a3c7c16..2796a019a468 100644 --- 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 = 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 index f8e8c6a166d2..c81cc467c3db 100644 --- 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 index 6b65e2255f84..6072479f2589 100644 --- 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',