Skip to content

Commit 7f66dd3

Browse files
crisbetojosephperrott
authored andcommitted
fix(overlay): flexible overlay with push not handling scroll offset and position locking
* 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.
1 parent 9062640 commit 7f66dd3

File tree

4 files changed

+213
-21
lines changed

4 files changed

+213
-21
lines changed

src/cdk/overlay/position/flexible-connected-position-strategy.spec.ts

+159
Original file line numberDiff line numberDiff line change
@@ -1101,6 +1101,165 @@ describe('FlexibleConnectedPositionStrategy', () => {
11011101
expect(Math.floor(overlayRect.top)).toBe(15);
11021102
});
11031103

1104+
it('should not mess with the left offset when pushing from the top', () => {
1105+
originElement.style.top = `${-OVERLAY_HEIGHT * 2}px`;
1106+
originElement.style.left = '200px';
1107+
1108+
positionStrategy.withPositions([{
1109+
originX: 'start',
1110+
originY: 'bottom',
1111+
overlayX: 'start',
1112+
overlayY: 'top'
1113+
}]);
1114+
1115+
attachOverlay({positionStrategy});
1116+
1117+
const overlayRect = overlayRef.overlayElement.getBoundingClientRect();
1118+
expect(Math.floor(overlayRect.left)).toBe(200);
1119+
});
1120+
1121+
it('should align to the trigger if the overlay is wider than the viewport, but the trigger ' +
1122+
'is still within the viewport', () => {
1123+
originElement.style.top = '200px';
1124+
originElement.style.left = '200px';
1125+
1126+
positionStrategy.withPositions([
1127+
{
1128+
originX: 'start',
1129+
originY: 'bottom',
1130+
overlayX: 'start',
1131+
overlayY: 'top'
1132+
},
1133+
{
1134+
originX: 'end',
1135+
originY: 'bottom',
1136+
overlayX: 'end',
1137+
overlayY: 'top'
1138+
}
1139+
]);
1140+
1141+
attachOverlay({
1142+
width: viewport.getViewportRect().width + 100,
1143+
positionStrategy
1144+
});
1145+
1146+
const overlayRect = overlayRef.overlayElement.getBoundingClientRect();
1147+
const originRect = originElement.getBoundingClientRect();
1148+
1149+
expect(Math.floor(overlayRect.left)).toBe(Math.floor(originRect.left));
1150+
});
1151+
1152+
it('should push into the viewport if the overlay is wider than the viewport and the trigger' +
1153+
'out of the viewport', () => {
1154+
originElement.style.top = '200px';
1155+
originElement.style.left = `-${DEFAULT_WIDTH / 2}px`;
1156+
1157+
positionStrategy.withPositions([
1158+
{
1159+
originX: 'start',
1160+
originY: 'bottom',
1161+
overlayX: 'start',
1162+
overlayY: 'top'
1163+
},
1164+
{
1165+
originX: 'end',
1166+
originY: 'bottom',
1167+
overlayX: 'end',
1168+
overlayY: 'top'
1169+
}
1170+
]);
1171+
1172+
attachOverlay({
1173+
width: viewport.getViewportRect().width + 100,
1174+
positionStrategy
1175+
});
1176+
1177+
const overlayRect = overlayRef.overlayElement.getBoundingClientRect();
1178+
expect(Math.floor(overlayRect.left)).toBe(0);
1179+
});
1180+
1181+
it('should keep the element inside the viewport as the user is scrolling, ' +
1182+
'with position locking disabled', () => {
1183+
const veryLargeElement = document.createElement('div');
1184+
1185+
originElement.style.top = `${-OVERLAY_HEIGHT * 2}px`;
1186+
originElement.style.left = '200px';
1187+
1188+
veryLargeElement.style.width = '100%';
1189+
veryLargeElement.style.height = '2000px';
1190+
document.body.appendChild(veryLargeElement);
1191+
1192+
positionStrategy
1193+
.withLockedPosition(false)
1194+
.withViewportMargin(0)
1195+
.withPositions([{
1196+
overlayY: 'top',
1197+
overlayX: 'start',
1198+
originY: 'top',
1199+
originX: 'start'
1200+
}]);
1201+
1202+
attachOverlay({positionStrategy});
1203+
1204+
let overlayRect = overlayRef.overlayElement.getBoundingClientRect();
1205+
expect(Math.floor(overlayRect.top))
1206+
.toBe(0, 'Expected overlay to be in the viewport initially.');
1207+
1208+
window.scroll(0, 100);
1209+
overlayRef.updatePosition();
1210+
zone.simulateZoneExit();
1211+
1212+
overlayRect = overlayRef.overlayElement.getBoundingClientRect();
1213+
expect(Math.floor(overlayRect.top))
1214+
.toBe(0, 'Expected overlay to stay in the viewport after scrolling.');
1215+
1216+
window.scroll(0, 0);
1217+
document.body.removeChild(veryLargeElement);
1218+
});
1219+
1220+
it('should not continue pushing the overlay as the user scrolls, if position ' +
1221+
'locking is enabled', () => {
1222+
const veryLargeElement = document.createElement('div');
1223+
1224+
originElement.style.top = `${-OVERLAY_HEIGHT * 2}px`;
1225+
originElement.style.left = '200px';
1226+
1227+
veryLargeElement.style.width = '100%';
1228+
veryLargeElement.style.height = '2000px';
1229+
document.body.appendChild(veryLargeElement);
1230+
1231+
positionStrategy
1232+
.withLockedPosition()
1233+
.withViewportMargin(0)
1234+
.withPositions([{
1235+
overlayY: 'top',
1236+
overlayX: 'start',
1237+
originY: 'top',
1238+
originX: 'start'
1239+
}]);
1240+
1241+
attachOverlay({positionStrategy});
1242+
1243+
const scrollBy = 100;
1244+
let initialOverlayTop = Math.floor(overlayRef.overlayElement.getBoundingClientRect().top);
1245+
1246+
expect(initialOverlayTop).toBe(0, 'Expected overlay to be inside the viewport initially.');
1247+
1248+
window.scroll(0, scrollBy);
1249+
overlayRef.updatePosition();
1250+
zone.simulateZoneExit();
1251+
1252+
let currentOverlayTop = Math.floor(overlayRef.overlayElement.getBoundingClientRect().top);
1253+
1254+
expect(currentOverlayTop).toBeLessThan(0,
1255+
'Expected overlay to no longer be completely inside the viewport.');
1256+
expect(currentOverlayTop).toBe(initialOverlayTop - scrollBy,
1257+
'Expected overlay to maintain its previous position.');
1258+
1259+
window.scroll(0, 0);
1260+
document.body.removeChild(veryLargeElement);
1261+
});
1262+
11041263
});
11051264

11061265
describe('with flexible dimensions', () => {

src/cdk/overlay/position/flexible-connected-position-strategy.ts

+46-20
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {PositionStrategy} from './position-strategy';
1010
import {ElementRef} from '@angular/core';
11-
import {ViewportRuler, CdkScrollable} from '@angular/cdk/scrolling';
11+
import {ViewportRuler, CdkScrollable, ViewportScrollPosition} from '@angular/cdk/scrolling';
1212
import {
1313
ConnectedOverlayPositionChange,
1414
ConnectionPositionPair,
@@ -111,6 +111,9 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
111111
/** Amount of subscribers to the `positionChanges` stream. */
112112
private _positionChangeSubscriptions = 0;
113113

114+
/** Amount by which the overlay was pushed in each axis during the last time it was positioned. */
115+
private _previousPushAmount: {x: number, y: number} | null;
116+
114117
/** Observable sequence of position changes. */
115118
positionChanges: Observable<ConnectedOverlayPositionChange> = Observable.create(observer => {
116119
const subscription = this._positionChanges.subscribe(observer);
@@ -279,6 +282,8 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
279282
}
280283

281284
detach() {
285+
this._lastPosition = null;
286+
this._previousPushAmount = null;
282287
this._resizeSubscription.unsubscribe();
283288
}
284289

@@ -538,39 +543,55 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
538543
* the viewport, the top-left corner will be pushed on-screen (with overflow occuring on the
539544
* right and bottom).
540545
*
541-
* @param start The starting point from which the overlay is pushed.
542-
* @param overlay The overlay dimensions.
546+
* @param start Starting point from which the overlay is pushed.
547+
* @param overlay Dimensions of the overlay.
548+
* @param scrollPosition Current viewport scroll position.
543549
* @returns The point at which to position the overlay after pushing. This is effectively a new
544550
* originPoint.
545551
*/
546-
private _pushOverlayOnScreen(start: Point, overlay: ClientRect): Point {
552+
private _pushOverlayOnScreen(start: Point,
553+
overlay: ClientRect,
554+
scrollPosition: ViewportScrollPosition): Point {
555+
// If the position is locked and we've pushed the overlay already, reuse the previous push
556+
// amount, rather than pushing it again. If we were to continue pushing, the element would
557+
// remain in the viewport, which goes against the expectations when position locking is enabled.
558+
if (this._previousPushAmount && this._positionLocked) {
559+
return {
560+
x: start.x + this._previousPushAmount.x,
561+
y: start.y + this._previousPushAmount.y
562+
};
563+
}
564+
547565
const viewport = this._viewportRect;
548566

549-
// Determine how much the overlay goes outside the viewport on each side, which we'll use to
550-
// decide which direction to push it.
567+
// Determine how much the overlay goes outside the viewport on each
568+
// side, which we'll use to decide which direction to push it.
551569
const overflowRight = Math.max(start.x + overlay.width - viewport.right, 0);
552570
const overflowBottom = Math.max(start.y + overlay.height - viewport.bottom, 0);
553-
const overflowTop = Math.max(viewport.top - start.y, 0);
554-
const overflowLeft = Math.max(viewport.left - start.x, 0);
571+
const overflowTop = Math.max(viewport.top - scrollPosition.top - start.y, 0);
572+
const overflowLeft = Math.max(viewport.left - scrollPosition.left - start.x, 0);
555573

556-
// Amount by which to push the overlay in each direction such that it remains on-screen.
557-
let pushX, pushY = 0;
574+
// Amount by which to push the overlay in each axis such that it remains on-screen.
575+
let pushX = 0;
576+
let pushY = 0;
558577

559578
// If the overlay fits completely within the bounds of the viewport, push it from whichever
560579
// direction is goes off-screen. Otherwise, push the top-left corner such that its in the
561580
// viewport and allow for the trailing end of the overlay to go out of bounds.
562-
if (overlay.width <= viewport.width) {
581+
if (overlay.width < viewport.width) {
563582
pushX = overflowLeft || -overflowRight;
564583
} else {
565-
pushX = viewport.left - start.x;
584+
pushX = start.x < this._viewportMargin ? (viewport.left - scrollPosition.left) - start.x : 0;
566585
}
567586

568-
if (overlay.height <= viewport.height) {
587+
if (overlay.height < viewport.height) {
569588
pushY = overflowTop || -overflowBottom;
570589
} else {
571-
pushY = viewport.top - start.y;
590+
pushY = start.y < this._viewportMargin ? (viewport.top - scrollPosition.top) - start.y : 0;
572591
}
573592

593+
this._previousPushAmount = {x: pushX, y: pushY};
594+
574595
return {
575596
x: start.x + pushX,
576597
y: start.y + pushY,
@@ -789,8 +810,9 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
789810
const styles = {} as CSSStyleDeclaration;
790811

791812
if (this._hasExactPosition()) {
792-
extendStyles(styles, this._getExactOverlayY(position, originPoint));
793-
extendStyles(styles, this._getExactOverlayX(position, originPoint));
813+
const scrollPosition = this._viewportRuler.getViewportScrollPosition();
814+
extendStyles(styles, this._getExactOverlayY(position, originPoint, scrollPosition));
815+
extendStyles(styles, this._getExactOverlayX(position, originPoint, scrollPosition));
794816
} else {
795817
styles.position = 'static';
796818
}
@@ -829,14 +851,16 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
829851
}
830852

831853
/** Gets the exact top/bottom for the overlay when not using flexible sizing or when pushing. */
832-
private _getExactOverlayY(position: ConnectedPosition, originPoint: Point) {
854+
private _getExactOverlayY(position: ConnectedPosition,
855+
originPoint: Point,
856+
scrollPosition: ViewportScrollPosition) {
833857
// Reset any existing styles. This is necessary in case the
834858
// preferred position has changed since the last `apply`.
835859
let styles = {top: null, bottom: null} as CSSStyleDeclaration;
836860
let overlayPoint = this._getOverlayPoint(originPoint, this._overlayRect, position);
837861

838862
if (this._isPushed) {
839-
overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect);
863+
overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect, scrollPosition);
840864
}
841865

842866
// We want to set either `top` or `bottom` based on whether the overlay wants to appear
@@ -854,14 +878,16 @@ export class FlexibleConnectedPositionStrategy implements PositionStrategy {
854878
}
855879

856880
/** Gets the exact left/right for the overlay when not using flexible sizing or when pushing. */
857-
private _getExactOverlayX(position: ConnectedPosition, originPoint: Point) {
881+
private _getExactOverlayX(position: ConnectedPosition,
882+
originPoint: Point,
883+
scrollPosition: ViewportScrollPosition) {
858884
// Reset any existing styles. This is necessary in case the preferred position has
859885
// changed since the last `apply`.
860886
let styles = {left: null, right: null} as CSSStyleDeclaration;
861887
let overlayPoint = this._getOverlayPoint(originPoint, this._overlayRect, position);
862888

863889
if (this._isPushed) {
864-
overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect);
890+
overlayPoint = this._pushOverlayOnScreen(overlayPoint, this._overlayRect, scrollPosition);
865891
}
866892

867893
// We want to set either `left` or `right` based on whether the overlay wants to appear "before"

src/cdk/scrolling/viewport-ruler.ts

+7-1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,12 @@ import {auditTime} from 'rxjs/operators';
1414
/** Time in ms to throttle the resize events by default. */
1515
export const DEFAULT_RESIZE_TIME = 20;
1616

17+
/** Object that holds the scroll position of the viewport in each direction. */
18+
export interface ViewportScrollPosition {
19+
top: number;
20+
left: number;
21+
}
22+
1723
/**
1824
* Simple utility for getting the bounds of the browser viewport.
1925
* @docs-private
@@ -82,7 +88,7 @@ export class ViewportRuler implements OnDestroy {
8288
}
8389

8490
/** Gets the (top, left) scroll position of the viewport. */
85-
getViewportScrollPosition() {
91+
getViewportScrollPosition(): ViewportScrollPosition {
8692
// While we can get a reference to the fake document
8793
// during SSR, it doesn't have getBoundingClientRect.
8894
if (!this._platform.isBrowser) {

src/lib/menu/menu-trigger.ts

+1
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
403403
return this._overlay.position()
404404
.flexibleConnectedTo(this._element)
405405
.withTransformOriginOn('.mat-menu-panel')
406+
.withLockedPosition()
406407
.withPositions([
407408
{originX, originY, overlayX, overlayY, offsetY},
408409
{originX: originFallbackX, originY, overlayX: overlayFallbackX, overlayY, offsetY},

0 commit comments

Comments
 (0)