Skip to content

Commit

Permalink
fix(material/menu): position classes not update when window is resized (
Browse files Browse the repository at this point in the history
#24385)

Relates to internal bug b/218602349. We were calling `MatMenu.setPositionClasses` in order to set the classes if the position changes, but the call was outside of the `NgZone` so nothing was happening. These changes bring the call back into the `NgZone`.
  • Loading branch information
crisbeto committed Feb 14, 2022
1 parent 599d1b4 commit f868e33
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 5 deletions.
23 changes: 23 additions & 0 deletions src/material-experimental/mdc-menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1331,6 +1331,29 @@ describe('MDC-based MatMenu', () => {
expect(panel.classList).not.toContain('mat-menu-above');
}));

it('should update the position classes if the window is resized', fakeAsync(() => {
trigger.style.position = 'fixed';
trigger.style.top = '300px';
fixture.componentInstance.yPosition = 'above';
fixture.componentInstance.trigger.openMenu();
fixture.detectChanges();
tick(500);

const panel = overlayContainerElement.querySelector('.mat-mdc-menu-panel') as HTMLElement;

expect(panel.classList).toContain('mat-menu-above');
expect(panel.classList).not.toContain('mat-menu-below');

trigger.style.top = '0';
dispatchFakeEvent(window, 'resize');
fixture.detectChanges();
tick(500);
fixture.detectChanges();

expect(panel.classList).not.toContain('mat-menu-above');
expect(panel.classList).toContain('mat-menu-below');
}));

it('should be able to update the position after the first open', fakeAsync(() => {
trigger.style.position = 'fixed';
trigger.style.top = '200px';
Expand Down
14 changes: 13 additions & 1 deletion src/material-experimental/mdc-menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import {Overlay, ScrollStrategy} from '@angular/cdk/overlay';
import {
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
ElementRef,
Inject,
Expand Down Expand Up @@ -56,11 +57,22 @@ export class MatMenu extends _MatMenuBase {
protected override _elevationPrefix = 'mat-mdc-elevation-z';
protected override _baseElevation = 8;

/*
* @deprecated `changeDetectorRef` parameter will become a required parameter.
* @breaking-change 15.0.0
*/
constructor(
elementRef: ElementRef<HTMLElement>,
ngZone: NgZone,
defaultOptions: MatMenuDefaultOptions,
);

constructor(
_elementRef: ElementRef<HTMLElement>,
_ngZone: NgZone,
@Inject(MAT_MENU_DEFAULT_OPTIONS) _defaultOptions: MatMenuDefaultOptions,
changeDetectorRef?: ChangeDetectorRef,
) {
super(_elementRef, _ngZone, _defaultOptions);
super(_elementRef, _ngZone, _defaultOptions, changeDetectorRef);
}
}
26 changes: 25 additions & 1 deletion src/material/menu/menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
Inject,
InjectionToken,
Input,
NgZone,
OnDestroy,
Optional,
Output,
Expand Down Expand Up @@ -200,6 +201,21 @@ export abstract class _MatMenuTriggerBase implements AfterContentInit, OnDestroy
focusMonitor?: FocusMonitor | null,
);

/**
* @deprecated `ngZone` will become a required parameter.
* @breaking-change 15.0.0
*/
constructor(
overlay: Overlay,
element: ElementRef<HTMLElement>,
viewContainerRef: ViewContainerRef,
scrollStrategy: any,
parentMenu: MatMenuPanel,
menuItemInstance: MatMenuItem,
dir: Directionality,
focusMonitor: FocusMonitor,
);

constructor(
private _overlay: Overlay,
private _element: ElementRef<HTMLElement>,
Expand All @@ -211,6 +227,7 @@ export abstract class _MatMenuTriggerBase implements AfterContentInit, OnDestroy
@Optional() @Self() private _menuItemInstance: MatMenuItem,
@Optional() private _dir: Directionality,
private _focusMonitor: FocusMonitor | null,
private _ngZone?: NgZone,
) {
this._scrollStrategy = scrollStrategy;
this._parentMaterialMenu = parentMenu instanceof _MatMenuBase ? parentMenu : undefined;
Expand Down Expand Up @@ -472,7 +489,14 @@ export abstract class _MatMenuTriggerBase implements AfterContentInit, OnDestroy
const posX: MenuPositionX = change.connectionPair.overlayX === 'start' ? 'after' : 'before';
const posY: MenuPositionY = change.connectionPair.overlayY === 'top' ? 'below' : 'above';

this.menu.setPositionClasses!(posX, posY);
// @breaking-change 15.0.0 Remove null check for `ngZone`.
// `positionChanges` fires outside of the `ngZone` and `setPositionClasses` might be
// updating something in the view so we need to bring it back in.
if (this._ngZone) {
this._ngZone.run(() => this.menu.setPositionClasses!(posX, posY));
} else {
this.menu.setPositionClasses!(posX, posY);
}
});
}
}
Expand Down
23 changes: 23 additions & 0 deletions src/material/menu/menu.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1326,6 +1326,29 @@ describe('MatMenu', () => {
expect(panel.classList).not.toContain('mat-menu-above');
}));

it('should update the position classes if the window is resized', fakeAsync(() => {
trigger.style.position = 'fixed';
trigger.style.top = '300px';
fixture.componentInstance.yPosition = 'above';
fixture.componentInstance.trigger.openMenu();
fixture.detectChanges();
tick(500);

const panel = overlayContainerElement.querySelector('.mat-menu-panel') as HTMLElement;

expect(panel.classList).toContain('mat-menu-above');
expect(panel.classList).not.toContain('mat-menu-below');

trigger.style.top = '0';
dispatchFakeEvent(window, 'resize');
fixture.detectChanges();
tick(500);
fixture.detectChanges();

expect(panel.classList).not.toContain('mat-menu-above');
expect(panel.classList).toContain('mat-menu-below');
}));

it('should be able to update the position after the first open', fakeAsync(() => {
trigger.style.position = 'fixed';
trigger.style.top = '200px';
Expand Down
19 changes: 18 additions & 1 deletion src/material/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
ViewChild,
ViewEncapsulation,
OnInit,
ChangeDetectorRef,
} from '@angular/core';
import {merge, Observable, Subject, Subscription} from 'rxjs';
import {startWith, switchMap, take} from 'rxjs/operators';
Expand Down Expand Up @@ -272,6 +273,8 @@ export class _MatMenuBase
private _elementRef: ElementRef<HTMLElement>,
private _ngZone: NgZone,
@Inject(MAT_MENU_DEFAULT_OPTIONS) private _defaultOptions: MatMenuDefaultOptions,
// @breaking-change 15.0.0 `_changeDetectorRef` to become a required parameter.
private _changeDetectorRef?: ChangeDetectorRef,
) {}

ngOnInit() {
Expand Down Expand Up @@ -452,6 +455,9 @@ export class _MatMenuBase
classes['mat-menu-after'] = posX === 'after';
classes['mat-menu-above'] = posY === 'above';
classes['mat-menu-below'] = posY === 'below';

// @breaking-change 15.0.0 Remove null check for `_changeDetectorRef`.
this._changeDetectorRef?.markForCheck();
}

/** Starts the enter animation. */
Expand Down Expand Up @@ -522,11 +528,22 @@ export class MatMenu extends _MatMenuBase {
protected override _elevationPrefix = 'mat-elevation-z';
protected override _baseElevation = 4;

/**
* @deprecated `changeDetectorRef` parameter will become a required parameter.
* @breaking-change 15.0.0
*/
constructor(
elementRef: ElementRef<HTMLElement>,
ngZone: NgZone,
defaultOptions: MatMenuDefaultOptions,
);

constructor(
elementRef: ElementRef<HTMLElement>,
ngZone: NgZone,
@Inject(MAT_MENU_DEFAULT_OPTIONS) defaultOptions: MatMenuDefaultOptions,
changeDetectorRef?: ChangeDetectorRef,
) {
super(elementRef, ngZone, defaultOptions);
super(elementRef, ngZone, defaultOptions, changeDetectorRef);
}
}
7 changes: 5 additions & 2 deletions tools/public_api_guard/material/menu.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const MAT_MENU_SCROLL_STRATEGY_FACTORY_PROVIDER: {

// @public
export class MatMenu extends _MatMenuBase {
// @deprecated
constructor(elementRef: ElementRef<HTMLElement>, ngZone: NgZone, defaultOptions: MatMenuDefaultOptions);
// (undocumented)
protected _baseElevation: number;
Expand All @@ -90,7 +91,7 @@ export const matMenuAnimations: {

// @public
export class _MatMenuBase implements AfterContentInit, MatMenuPanel<MatMenuItem>, OnInit, OnDestroy {
constructor(_elementRef: ElementRef<HTMLElement>, _ngZone: NgZone, _defaultOptions: MatMenuDefaultOptions);
constructor(_elementRef: ElementRef<HTMLElement>, _ngZone: NgZone, _defaultOptions: MatMenuDefaultOptions, _changeDetectorRef?: ChangeDetectorRef | undefined);
// (undocumented)
addItem(_item: MatMenuItem): void;
_allItems: QueryList<MatMenuItem>;
Expand Down Expand Up @@ -282,6 +283,8 @@ export class MatMenuTrigger extends _MatMenuTriggerBase {
export abstract class _MatMenuTriggerBase implements AfterContentInit, OnDestroy {
// @deprecated
constructor(overlay: Overlay, element: ElementRef<HTMLElement>, viewContainerRef: ViewContainerRef, scrollStrategy: any, parentMenu: MatMenuPanel, menuItemInstance: MatMenuItem, dir: Directionality, focusMonitor?: FocusMonitor | null);
// @deprecated
constructor(overlay: Overlay, element: ElementRef<HTMLElement>, viewContainerRef: ViewContainerRef, scrollStrategy: any, parentMenu: MatMenuPanel, menuItemInstance: MatMenuItem, dir: Directionality, focusMonitor: FocusMonitor);
closeMenu(): void;
// @deprecated (undocumented)
get _deprecatedMatMenuTriggerFor(): MatMenuPanel;
Expand Down Expand Up @@ -315,7 +318,7 @@ export abstract class _MatMenuTriggerBase implements AfterContentInit, OnDestroy
// (undocumented)
static ɵdir: i0.ɵɵDirectiveDeclaration<_MatMenuTriggerBase, never, never, { "_deprecatedMatMenuTriggerFor": "mat-menu-trigger-for"; "menu": "matMenuTriggerFor"; "menuData": "matMenuTriggerData"; "restoreFocus": "matMenuTriggerRestoreFocus"; }, { "menuOpened": "menuOpened"; "onMenuOpen": "onMenuOpen"; "menuClosed": "menuClosed"; "onMenuClose": "onMenuClose"; }, never>;
// (undocumented)
static ɵfac: i0.ɵɵFactoryDeclaration<_MatMenuTriggerBase, [null, null, null, null, { optional: true; }, { optional: true; self: true; }, { optional: true; }, null]>;
static ɵfac: i0.ɵɵFactoryDeclaration<_MatMenuTriggerBase, [null, null, null, null, { optional: true; }, { optional: true; self: true; }, { optional: true; }, null, null]>;
}

// @public
Expand Down

0 comments on commit f868e33

Please sign in to comment.