Skip to content

Commit

Permalink
fix(material/menu): adjust overlay size when amount of items changes
Browse files Browse the repository at this point in the history
Currently we lock the menu into a position after it is opened so that it doesn't jump
around when the user scrolls, but this means that if the amount of items changes,
it might not be the optimal position anymore.

These changes add some code to re-calculate the position if the amount of items changes.

Fixes angular#21456.
  • Loading branch information
crisbeto committed Feb 7, 2021
1 parent 51f5924 commit c02a6d6
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 14 deletions.
37 changes: 31 additions & 6 deletions src/material-experimental/mdc-menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
MockNgZone,
} from '@angular/cdk/testing/private';
import {Subject} from 'rxjs';
import {ScrollDispatcher} from '@angular/cdk/scrolling';
import {ScrollDispatcher, ViewportRuler} from '@angular/cdk/scrolling';
import {FocusMonitor} from '@angular/cdk/a11y';
import {
MAT_MENU_SCROLL_STRATEGY,
Expand All @@ -58,6 +58,7 @@ describe('MDC-based MatMenu', () => {
let overlayContainer: OverlayContainer;
let overlayContainerElement: HTMLElement;
let focusMonitor: FocusMonitor;
let viewportRuler: ViewportRuler;

function createComponent<T>(component: Type<T>,
providers: Provider[] = [],
Expand All @@ -68,11 +69,13 @@ describe('MDC-based MatMenu', () => {
providers
}).compileComponents();

inject([OverlayContainer, FocusMonitor], (oc: OverlayContainer, fm: FocusMonitor) => {
overlayContainer = oc;
overlayContainerElement = oc.getContainerElement();
focusMonitor = fm;
})();
inject([OverlayContainer, FocusMonitor, ViewportRuler],
(oc: OverlayContainer, fm: FocusMonitor, vr: ViewportRuler) => {
overlayContainer = oc;
overlayContainerElement = oc.getContainerElement();
focusMonitor = fm;
viewportRuler = vr;
})();

return TestBed.createComponent<T>(component);
}
Expand Down Expand Up @@ -1059,6 +1062,28 @@ describe('MDC-based MatMenu', () => {
expect(document.activeElement).toBe(items[3], 'Expected fourth item to be focused');
}));

it('should keep the panel in the viewport when more items are added while open', () => {
const fixture = createComponent(SimpleMenu, [], [FakeIcon]);
fixture.detectChanges();

const triggerEl = fixture.componentInstance.triggerEl.nativeElement;
triggerEl.style.position = 'absolute';
triggerEl.style.left = '200px';
triggerEl.style.bottom = '300px';
triggerEl.click();
fixture.detectChanges();

const panel = overlayContainerElement.querySelector('.mat-mdc-menu-panel')!;
const viewportHeight = viewportRuler.getViewportSize().height;
let panelRect = panel.getBoundingClientRect();
expect(Math.floor(panelRect.bottom)).toBeLessThan(viewportHeight);

fixture.componentInstance.extraItems = new Array(50).fill('Hello there');
fixture.detectChanges();
panelRect = panel.getBoundingClientRect();
expect(Math.floor(panelRect.bottom)).toBe(viewportHeight);
});

describe('lazy rendering', () => {
it('should be able to render the menu content lazily', fakeAsync(() => {
const fixture = createComponent(SimpleLazyMenu);
Expand Down
9 changes: 8 additions & 1 deletion src/material/menu/menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,9 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {

const overlayRef = this._createOverlay();
const overlayConfig = overlayRef.getConfig();
const positionStrategy = overlayConfig.positionStrategy as FlexibleConnectedPositionStrategy;

this._setPosition(overlayConfig.positionStrategy as FlexibleConnectedPositionStrategy);
this._setPosition(positionStrategy);
overlayConfig.hasBackdrop = this.menu.hasBackdrop == null ? !this.triggersSubmenu() :
this.menu.hasBackdrop;
overlayRef.attach(this._getPortal());
Expand All @@ -262,6 +263,12 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {

if (this.menu instanceof _MatMenuBase) {
this.menu._startAnimation();
this.menu._directDescendantItems.changes.pipe(takeUntil(this.menu.close)).subscribe(() => {
// Re-adjust the position without locking when the amount of items
// changes so that the overlay is allowed to pick a new optimal position.
positionStrategy.withLockedPosition(false).reapplyLastPosition();
positionStrategy.withLockedPosition(true);
});
}
}

Expand Down
37 changes: 31 additions & 6 deletions src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
TAB,
} from '@angular/cdk/keycodes';
import {Overlay, OverlayContainer} from '@angular/cdk/overlay';
import {ScrollDispatcher} from '@angular/cdk/scrolling';
import {ScrollDispatcher, ViewportRuler} from '@angular/cdk/scrolling';
import {
createKeyboardEvent,
createMouseEvent,
Expand Down Expand Up @@ -59,6 +59,7 @@ describe('MatMenu', () => {
let overlayContainer: OverlayContainer;
let overlayContainerElement: HTMLElement;
let focusMonitor: FocusMonitor;
let viewportRuler: ViewportRuler;

function createComponent<T>(component: Type<T>,
providers: Provider[] = [],
Expand All @@ -69,11 +70,13 @@ describe('MatMenu', () => {
providers
}).compileComponents();

inject([OverlayContainer, FocusMonitor], (oc: OverlayContainer, fm: FocusMonitor) => {
overlayContainer = oc;
overlayContainerElement = oc.getContainerElement();
focusMonitor = fm;
})();
inject([OverlayContainer, FocusMonitor, ViewportRuler],
(oc: OverlayContainer, fm: FocusMonitor, vr: ViewportRuler) => {
overlayContainer = oc;
overlayContainerElement = oc.getContainerElement();
focusMonitor = fm;
viewportRuler = vr;
})();

return TestBed.createComponent<T>(component);
}
Expand Down Expand Up @@ -1002,6 +1005,28 @@ describe('MatMenu', () => {
expect(document.activeElement).toBe(items[3], 'Expected fourth item to be focused');
}));

it('should keep the panel in the viewport when more items are added while open', () => {
const fixture = createComponent(SimpleMenu, [], [FakeIcon]);
fixture.detectChanges();

const triggerEl = fixture.componentInstance.triggerEl.nativeElement;
triggerEl.style.position = 'absolute';
triggerEl.style.left = '200px';
triggerEl.style.bottom = '300px';
triggerEl.click();
fixture.detectChanges();

const panel = overlayContainerElement.querySelector('.mat-menu-panel')!;
const viewportHeight = viewportRuler.getViewportSize().height;
let panelRect = panel.getBoundingClientRect();
expect(Math.floor(panelRect.bottom)).toBeLessThan(viewportHeight);

fixture.componentInstance.extraItems = new Array(50).fill('Hello there');
fixture.detectChanges();
panelRect = panel.getBoundingClientRect();
expect(Math.floor(panelRect.bottom)).toBe(viewportHeight);
});

describe('lazy rendering', () => {
it('should be able to render the menu content lazily', fakeAsync(() => {
const fixture = createComponent(SimpleLazyMenu);
Expand Down
2 changes: 1 addition & 1 deletion src/material/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatMenuItem>
@ContentChildren(MatMenuItem, {descendants: true}) _allItems: QueryList<MatMenuItem>;

/** Only the direct descendant menu items. */
private _directDescendantItems = new QueryList<MatMenuItem>();
_directDescendantItems = new QueryList<MatMenuItem>();

/** Subscription to tab events on the menu panel */
private _tabSubscription = Subscription.EMPTY;
Expand Down
1 change: 1 addition & 0 deletions tools/public_api_guard/material/menu.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export declare class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatM
_classList: {
[key: string]: boolean;
};
_directDescendantItems: QueryList<MatMenuItem>;
_isAnimating: boolean;
_panelAnimationState: 'void' | 'enter';
ariaDescribedby: string;
Expand Down

0 comments on commit c02a6d6

Please sign in to comment.