From 993347977167acac06910c68614712e1da1c690f Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 22 Oct 2019 01:26:13 +0200 Subject: [PATCH] =?UTF-8?q?fix(sidenav):=20container=20not=20picking=20up?= =?UTF-8?q?=20indirect=20descendant=20sid=E2=80=A6=20(#17453)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the sidenav container not picking up indirect descendant sidenavs. --- src/material/sidenav/BUILD.bazel | 1 + src/material/sidenav/drawer.spec.ts | 80 +++++++++++++- src/material/sidenav/drawer.ts | 38 ++++++- src/material/sidenav/public-api.ts | 10 +- src/material/sidenav/sidenav.spec.ts | 103 ++++++++++++++++--- src/material/sidenav/sidenav.ts | 15 ++- tools/public_api_guard/material/sidenav.d.ts | 7 +- 7 files changed, 233 insertions(+), 21 deletions(-) diff --git a/src/material/sidenav/BUILD.bazel b/src/material/sidenav/BUILD.bazel index 976946e85d7c..b39fd678de75 100644 --- a/src/material/sidenav/BUILD.bazel +++ b/src/material/sidenav/BUILD.bazel @@ -65,6 +65,7 @@ ng_test_library( "//src/cdk/platform", "//src/cdk/scrolling", "//src/cdk/testing", + "@npm//@angular/common", "@npm//@angular/platform-browser", ], ) diff --git a/src/material/sidenav/drawer.spec.ts b/src/material/sidenav/drawer.spec.ts index c791c04b7020..0bf4645933a9 100644 --- a/src/material/sidenav/drawer.spec.ts +++ b/src/material/sidenav/drawer.spec.ts @@ -18,12 +18,13 @@ import {PlatformModule, Platform} from '@angular/cdk/platform'; import {ESCAPE} from '@angular/cdk/keycodes'; import {dispatchKeyboardEvent, createKeyboardEvent, dispatchEvent} from '@angular/cdk/testing'; import {CdkScrollable} from '@angular/cdk/scrolling'; +import {CommonModule} from '@angular/common'; describe('MatDrawer', () => { beforeEach(async(() => { TestBed.configureTestingModule({ - imports: [MatSidenavModule, A11yModule, PlatformModule, NoopAnimationsModule], + imports: [MatSidenavModule, A11yModule, PlatformModule, NoopAnimationsModule, CommonModule], declarations: [ BasicTestApp, DrawerContainerNoDrawerTestApp, @@ -33,6 +34,8 @@ describe('MatDrawer', () => { DrawerWithFocusableElements, DrawerOpenBinding, DrawerWithoutFocusableElements, + IndirectDescendantDrawer, + NestedDrawerContainers, ], }); @@ -323,6 +326,46 @@ describe('MatDrawer', () => { expect(document.activeElement) .toBe(closeButton, 'Expected focus not to be restored to the open button on close.'); })); + + it('should pick up drawers that are not direct descendants', fakeAsync(() => { + const fixture = TestBed.createComponent(IndirectDescendantDrawer); + fixture.detectChanges(); + + expect(fixture.componentInstance.drawer.opened).toBe(false); + + fixture.componentInstance.container.open(); + fixture.detectChanges(); + tick(); + fixture.detectChanges(); + + expect(fixture.componentInstance.drawer.opened).toBe(true); + })); + + it('should not pick up drawers from nested containers', fakeAsync(() => { + const fixture = TestBed.createComponent(NestedDrawerContainers); + const instance = fixture.componentInstance; + fixture.detectChanges(); + + expect(instance.outerDrawer.opened).toBe(false); + expect(instance.innerDrawer.opened).toBe(false); + + instance.outerContainer.open(); + fixture.detectChanges(); + tick(); + fixture.detectChanges(); + + expect(instance.outerDrawer.opened).toBe(true); + expect(instance.innerDrawer.opened).toBe(false); + + instance.innerContainer.open(); + fixture.detectChanges(); + tick(); + fixture.detectChanges(); + + expect(instance.outerDrawer.opened).toBe(true); + expect(instance.innerDrawer.opened).toBe(true); + })); + }); describe('attributes', () => { @@ -998,3 +1041,38 @@ class AutosizeDrawer { class DrawerContainerWithContent { @ViewChild(MatDrawerContainer, {static: false}) drawerContainer: MatDrawerContainer; } + + +@Component({ + // Note that we need the `ng-container` with the `ngSwitch` so that + // there's a directive between the container and the drawer. + template: ` + + + Drawer + + `, +}) +class IndirectDescendantDrawer { + @ViewChild('container', {static: false}) container: MatDrawerContainer; + @ViewChild('drawer', {static: false}) drawer: MatDrawer; +} + +@Component({ + template: ` + + Drawer + + + Drawer + + + + `, +}) +class NestedDrawerContainers { + @ViewChild('outerContainer', {static: false}) outerContainer: MatDrawerContainer; + @ViewChild('outerDrawer', {static: false}) outerDrawer: MatDrawer; + @ViewChild('innerContainer', {static: false}) innerContainer: MatDrawerContainer; + @ViewChild('innerDrawer', {static: false}) innerDrawer: MatDrawer; +} diff --git a/src/material/sidenav/drawer.ts b/src/material/sidenav/drawer.ts index 791d332eaebe..66530f926431 100644 --- a/src/material/sidenav/drawer.ts +++ b/src/material/sidenav/drawer.ts @@ -71,6 +71,13 @@ export const MAT_DRAWER_DEFAULT_AUTOSIZE = factory: MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY, }); + +/** + * Used to provide a drawer container to a drawer while avoiding circular references. + * @docs-private + */ +export const MAT_DRAWER_CONTAINER = new InjectionToken('MAT_DRAWER_CONTAINER'); + /** @docs-private */ export function MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY(): boolean { return false; @@ -246,7 +253,12 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr private _focusMonitor: FocusMonitor, private _platform: Platform, private _ngZone: NgZone, - @Optional() @Inject(DOCUMENT) private _doc: any) { + @Optional() @Inject(DOCUMENT) private _doc: any, + /** + * @deprecated `_container` parameter to be made required. + * @breaking-change 10.0.0 + */ + @Optional() @Inject(MAT_DRAWER_CONTAINER) public _container?: MatDrawerContainer) { this.openedChange.subscribe((opened: boolean) => { if (opened) { @@ -459,9 +471,23 @@ export class MatDrawer implements AfterContentInit, AfterContentChecked, OnDestr }, changeDetection: ChangeDetectionStrategy.OnPush, encapsulation: ViewEncapsulation.None, + providers: [{ + provide: MAT_DRAWER_CONTAINER, + useExisting: MatDrawerContainer + }] }) export class MatDrawerContainer implements AfterContentInit, DoCheck, OnDestroy { - @ContentChildren(MatDrawer) _drawers: QueryList; + /** All drawers in the container. Includes drawers from inside nested containers. */ + @ContentChildren(MatDrawer, { + // We need to use `descendants: true`, because Ivy will no longer match + // indirect descendants if it's left as false. + descendants: true + }) + _allDrawers: QueryList; + + /** Drawers that belong to this container. */ + _drawers = new QueryList(); + @ContentChild(MatDrawerContent, {static: false}) _content: MatDrawerContent; @ViewChild(MatDrawerContent, {static: false}) _userContent: MatDrawerContent; @@ -565,6 +591,14 @@ export class MatDrawerContainer implements AfterContentInit, DoCheck, OnDestroy } ngAfterContentInit() { + this._allDrawers.changes + .pipe(startWith(this._allDrawers), takeUntil(this._destroyed)) + .subscribe((drawer: QueryList) => { + // @breaking-change 10.0.0 Remove `_container` check once container parameter is required. + this._drawers.reset(drawer.filter(item => !item._container || item._container === this)); + this._drawers.notifyOnChanges(); + }); + this._drawers.changes.pipe(startWith(null)).subscribe(() => { this._validateDrawers(); diff --git a/src/material/sidenav/public-api.ts b/src/material/sidenav/public-api.ts index 5276be4aa808..e2450973e494 100644 --- a/src/material/sidenav/public-api.ts +++ b/src/material/sidenav/public-api.ts @@ -7,6 +7,14 @@ */ export * from './sidenav-module'; -export * from './drawer'; +export { + throwMatDuplicatedDrawerError, + MatDrawerToggleResult, + MAT_DRAWER_DEFAULT_AUTOSIZE, + MAT_DRAWER_DEFAULT_AUTOSIZE_FACTORY, + MatDrawerContent, + MatDrawer, + MatDrawerContainer, +} from './drawer'; export * from './sidenav'; export * from './drawer-animations'; diff --git a/src/material/sidenav/sidenav.spec.ts b/src/material/sidenav/sidenav.spec.ts index c6284a6a48d3..3c4a53848c23 100644 --- a/src/material/sidenav/sidenav.spec.ts +++ b/src/material/sidenav/sidenav.spec.ts @@ -1,29 +1,26 @@ -import {Component} from '@angular/core'; -import {async, TestBed, ComponentFixture} from '@angular/core/testing'; -import {MatSidenav, MatSidenavModule} from './index'; +import {Component, ViewChild} from '@angular/core'; +import {async, TestBed, fakeAsync, tick} from '@angular/core/testing'; +import {MatSidenav, MatSidenavModule, MatSidenavContainer} from './index'; import {NoopAnimationsModule} from '@angular/platform-browser/animations'; import {By} from '@angular/platform-browser'; +import {CommonModule} from '@angular/common'; describe('MatSidenav', () => { - let fixture: ComponentFixture; - let sidenavEl: HTMLElement; - beforeEach(async(() => { TestBed.configureTestingModule({ - imports: [MatSidenavModule, NoopAnimationsModule], - declarations: [SidenavWithFixedPosition], + imports: [MatSidenavModule, NoopAnimationsModule, CommonModule], + declarations: [SidenavWithFixedPosition, IndirectDescendantSidenav, NestedSidenavContainers], }); TestBed.compileComponents(); - - fixture = TestBed.createComponent(SidenavWithFixedPosition); - fixture.detectChanges(); - - sidenavEl = fixture.debugElement.query(By.directive(MatSidenav))!.nativeElement; })); it('should be fixed position when in fixed mode', () => { + const fixture = TestBed.createComponent(SidenavWithFixedPosition); + fixture.detectChanges(); + const sidenavEl = fixture.debugElement.query(By.directive(MatSidenav))!.nativeElement; + expect(sidenavEl.classList).toContain('mat-sidenav-fixed'); fixture.componentInstance.fixed = false; @@ -33,6 +30,10 @@ describe('MatSidenav', () => { }); it('should set fixed bottom and top when in fixed mode', () => { + const fixture = TestBed.createComponent(SidenavWithFixedPosition); + fixture.detectChanges(); + const sidenavEl = fixture.debugElement.query(By.directive(MatSidenav))!.nativeElement; + expect(sidenavEl.style.top).toBe('20px'); expect(sidenavEl.style.bottom).toBe('30px'); @@ -42,6 +43,46 @@ describe('MatSidenav', () => { expect(sidenavEl.style.top).toBeFalsy(); expect(sidenavEl.style.bottom).toBeFalsy(); }); + + it('should pick up sidenavs that are not direct descendants', fakeAsync(() => { + const fixture = TestBed.createComponent(IndirectDescendantSidenav); + fixture.detectChanges(); + + expect(fixture.componentInstance.sidenav.opened).toBe(false); + + fixture.componentInstance.container.open(); + fixture.detectChanges(); + tick(); + fixture.detectChanges(); + + expect(fixture.componentInstance.sidenav.opened).toBe(true); + })); + + it('should not pick up sidenavs from nested containers', fakeAsync(() => { + const fixture = TestBed.createComponent(NestedSidenavContainers); + const instance = fixture.componentInstance; + fixture.detectChanges(); + + expect(instance.outerSidenav.opened).toBe(false); + expect(instance.innerSidenav.opened).toBe(false); + + instance.outerContainer.open(); + fixture.detectChanges(); + tick(); + fixture.detectChanges(); + + expect(instance.outerSidenav.opened).toBe(true); + expect(instance.innerSidenav.opened).toBe(false); + + instance.innerContainer.open(); + fixture.detectChanges(); + tick(); + fixture.detectChanges(); + + expect(instance.outerSidenav.opened).toBe(true); + expect(instance.innerSidenav.opened).toBe(true); + })); + }); @@ -65,3 +106,39 @@ class SidenavWithFixedPosition { fixedTop = 20; fixedBottom = 30; } + + +@Component({ + // Note that we need the `ng-container` with the `ngSwitch` so that + // there's a directive between the container and the sidenav. + template: ` + + + Sidenav. + + Some content. + `, +}) +class IndirectDescendantSidenav { + @ViewChild('container', {static: false}) container: MatSidenavContainer; + @ViewChild('sidenav', {static: false}) sidenav: MatSidenav; +} + +@Component({ + template: ` + + Sidenav + + + Sidenav + + + + `, +}) +class NestedSidenavContainers { + @ViewChild('outerContainer', {static: false}) outerContainer: MatSidenavContainer; + @ViewChild('outerSidenav', {static: false}) outerSidenav: MatSidenav; + @ViewChild('innerContainer', {static: false}) innerContainer: MatSidenavContainer; + @ViewChild('innerSidenav', {static: false}) innerSidenav: MatSidenav; +} diff --git a/src/material/sidenav/sidenav.ts b/src/material/sidenav/sidenav.ts index da78201c667f..41eeb77f1cc4 100644 --- a/src/material/sidenav/sidenav.ts +++ b/src/material/sidenav/sidenav.ts @@ -20,7 +20,7 @@ import { ElementRef, NgZone, } from '@angular/core'; -import {MatDrawer, MatDrawerContainer, MatDrawerContent} from './drawer'; +import {MatDrawer, MatDrawerContainer, MatDrawerContent, MAT_DRAWER_CONTAINER} from './drawer'; import {matDrawerAnimations} from './drawer-animations'; import {coerceBooleanProperty, coerceNumberProperty} from '@angular/cdk/coercion'; import {ScrollDispatcher} from '@angular/cdk/scrolling'; @@ -112,8 +112,19 @@ export class MatSidenav extends MatDrawer { }, changeDetection: ChangeDetectionStrategy.OnPush, encapsulation: ViewEncapsulation.None, + providers: [{ + provide: MAT_DRAWER_CONTAINER, + useExisting: MatSidenavContainer + }] + }) export class MatSidenavContainer extends MatDrawerContainer { - @ContentChildren(MatSidenav) _drawers: QueryList; + @ContentChildren(MatSidenav, { + // We need to use `descendants: true`, because Ivy will no longer match + // indirect descendants if it's left as false. + descendants: true + }) + _allDrawers: QueryList; + @ContentChild(MatSidenavContent, {static: false}) _content: MatSidenavContent; } diff --git a/tools/public_api_guard/material/sidenav.d.ts b/tools/public_api_guard/material/sidenav.d.ts index 377488cd1d47..92745c9eeb44 100644 --- a/tools/public_api_guard/material/sidenav.d.ts +++ b/tools/public_api_guard/material/sidenav.d.ts @@ -7,6 +7,7 @@ export declare class MatDrawer implements AfterContentInit, AfterContentChecked, _animationStarted: Subject; _animationState: 'open-instant' | 'open' | 'void'; readonly _closedStream: Observable; + _container?: MatDrawerContainer | undefined; readonly _isFocusTrapEnabled: boolean; readonly _modeChanged: Subject; readonly _openedStream: Observable; @@ -20,7 +21,8 @@ export declare class MatDrawer implements AfterContentInit, AfterContentChecked, readonly openedChange: EventEmitter; readonly openedStart: Observable; position: 'start' | 'end'; - constructor(_elementRef: ElementRef, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _doc: any); + constructor(_elementRef: ElementRef, _focusTrapFactory: FocusTrapFactory, _focusMonitor: FocusMonitor, _platform: Platform, _ngZone: NgZone, _doc: any, + _container?: MatDrawerContainer | undefined); _animationDoneListener(event: AnimationEvent): void; _animationStartListener(event: AnimationEvent): void; close(): Promise; @@ -36,6 +38,7 @@ export declare const matDrawerAnimations: { }; export declare class MatDrawerContainer implements AfterContentInit, DoCheck, OnDestroy { + _allDrawers: QueryList; _backdropOverride: boolean | null; _content: MatDrawerContent; readonly _contentMarginChanges: Subject<{ @@ -81,8 +84,8 @@ export declare class MatSidenav extends MatDrawer { } export declare class MatSidenavContainer extends MatDrawerContainer { + _allDrawers: QueryList; _content: MatSidenavContent; - _drawers: QueryList; } export declare class MatSidenavContent extends MatDrawerContent {