Skip to content

fix(tabs): not picking up indirect descendant tabs in ivy #17346

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/material-experimental/mdc-tabs/public-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,5 @@ export {
MatTabHeaderPosition,
MatTabsConfig,
MAT_TABS_CONFIG,
MAT_TAB_GROUP,
} from '@angular/material/tabs';
49 changes: 48 additions & 1 deletion src/material-experimental/mdc-tabs/tab-group.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ describe('MatTabGroup', () => {
TemplateTabs,
TabGroupWithAriaInputs,
TabGroupWithIsActiveBinding,
NestedTabs,
TabGroupWithIndirectDescendantTabs,
],
});

Expand Down Expand Up @@ -589,6 +591,34 @@ describe('MatTabGroup', () => {
}));
});

describe('nested tabs', () => {
it('should not pick up the tabs from descendant tab groups', fakeAsync(() => {
const fixture = TestBed.createComponent(NestedTabs);
fixture.detectChanges();
tick();
fixture.detectChanges();

const groups = fixture.componentInstance.groups.toArray();

expect(groups.length).toBe(2);
expect(groups[0]._tabs.map((tab: MatTab) => tab.textLabel))
.toEqual(['One', 'Two']);
expect(groups[1]._tabs.map((tab: MatTab) => tab.textLabel))
.toEqual(['Inner tab one', 'Inner tab two']);
}));

it('should pick up indirect descendant tabs', fakeAsync(() => {
const fixture = TestBed.createComponent(TabGroupWithIndirectDescendantTabs);
fixture.detectChanges();
tick();
fixture.detectChanges();

const tabs = fixture.componentInstance.tabGroup._tabs;
expect(tabs.map((tab: MatTab) => tab.textLabel)).toEqual(['One', 'Two']);
}));
});


/**
* Checks that the `selectedIndex` has been updated; checks that the label and body have their
* respective `active` classes
Expand Down Expand Up @@ -825,7 +855,9 @@ class TabGroupWithSimpleApi {
</mat-tab-group>
`,
})
class NestedTabs {}
class NestedTabs {
@ViewChildren(MatTabGroup) groups: QueryList<MatTabGroup>;
}

@Component({
selector: 'template-tabs',
Expand Down Expand Up @@ -881,3 +913,18 @@ class TabGroupWithIsActiveBinding {
`,
})
class TabsWithCustomAnimationDuration {}


@Component({
template: `
<mat-tab-group>
<ng-container [ngSwitch]="true">
<mat-tab label="One">Tab one content</mat-tab>
<mat-tab label="Two">Tab two content</mat-tab>
</ng-container>
</mat-tab-group>
`,
})
class TabGroupWithIndirectDescendantTabs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all duplicated from what's in material/. Do we have a reason for choosing this approach?
Was it too much work or is there a blocker to trying to export something from material/ tests that could be re-used in the material-experimental/mdc-x tests?

@ViewChild(MatTabGroup, {static: false}) tabGroup: MatTabGroup;
}
13 changes: 11 additions & 2 deletions src/material-experimental/mdc-tabs/tab-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ import {
Inject,
Optional,
} from '@angular/core';
import {_MatTabGroupBase, MAT_TABS_CONFIG, MatTabsConfig} from '@angular/material/tabs';
import {
_MatTabGroupBase,
MAT_TABS_CONFIG,
MatTabsConfig,
MAT_TAB_GROUP,
} from '@angular/material/tabs';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
import {MatTab} from './tab';
import {MatTabHeader} from './tab-header';
Expand All @@ -37,14 +42,18 @@ import {MatTabHeader} from './tab-header';
encapsulation: ViewEncapsulation.None,
changeDetection: ChangeDetectionStrategy.OnPush,
inputs: ['color', 'disableRipple'],
providers: [{
provide: MAT_TAB_GROUP,
useExisting: MatTabGroup
}],
host: {
'class': 'mat-mdc-tab-group',
'[class.mat-mdc-tab-group-dynamic-height]': 'dynamicHeight',
'[class.mat-mdc-tab-group-inverted-header]': 'headerPosition === "below"',
},
})
export class MatTabGroup extends _MatTabGroupBase {
@ContentChildren(MatTab) _tabs: QueryList<MatTab>;
@ContentChildren(MatTab, {descendants: true}) _allTabs: QueryList<MatTab>;
@ViewChild('tabBodyWrapper', {static: false}) _tabBodyWrapper: ElementRef;
@ViewChild('tabHeader', {static: false}) _tabHeader: MatTabHeader;

Expand Down
2 changes: 1 addition & 1 deletion src/material/tabs/public-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export {
} from './tab-body';
export {MatTabHeader, _MatTabHeaderBase} from './tab-header';
export {MatTabLabelWrapper} from './tab-label-wrapper';
export {MatTab} from './tab';
export {MatTab, MAT_TAB_GROUP} from './tab';
export {MatTabLabel} from './tab-label';
export {MatTabNav, MatTabLink, _MatTabNavBase, _MatTabLinkBase} from './tab-nav-bar/index';
export {MatTabContent} from './tab-content';
Expand Down
48 changes: 47 additions & 1 deletion src/material/tabs/tab-group.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ describe('MatTabGroup', () => {
TemplateTabs,
TabGroupWithAriaInputs,
TabGroupWithIsActiveBinding,
NestedTabs,
TabGroupWithIndirectDescendantTabs,
],
});

Expand Down Expand Up @@ -588,6 +590,33 @@ describe('MatTabGroup', () => {
}));
});

describe('nested tabs', () => {
it('should not pick up the tabs from descendant tab groups', fakeAsync(() => {
const fixture = TestBed.createComponent(NestedTabs);
fixture.detectChanges();
tick();
fixture.detectChanges();

const groups = fixture.componentInstance.groups.toArray();

expect(groups.length).toBe(2);
expect(groups[0]._tabs.map((tab: MatTab) => tab.textLabel))
.toEqual(['One', 'Two']);
expect(groups[1]._tabs.map((tab: MatTab) => tab.textLabel))
.toEqual(['Inner tab one', 'Inner tab two']);
}));

it('should pick up indirect descendant tabs', fakeAsync(() => {
const fixture = TestBed.createComponent(TabGroupWithIndirectDescendantTabs);
fixture.detectChanges();
tick();
fixture.detectChanges();

const tabs = fixture.componentInstance.tabGroup._tabs;
expect(tabs.map((tab: MatTab) => tab.textLabel)).toEqual(['One', 'Two']);
}));
});

/**
* Checks that the `selectedIndex` has been updated; checks that the label and body have their
* respective `active` classes
Expand Down Expand Up @@ -824,7 +853,9 @@ class TabGroupWithSimpleApi {
</mat-tab-group>
`,
})
class NestedTabs {}
class NestedTabs {
@ViewChildren(MatTabGroup) groups: QueryList<MatTabGroup>;
}

@Component({
selector: 'template-tabs',
Expand Down Expand Up @@ -880,3 +911,18 @@ class TabGroupWithIsActiveBinding {
`,
})
class TabsWithCustomAnimationDuration {}


@Component({
template: `
<mat-tab-group>
<ng-container [ngSwitch]="true">
<mat-tab label="One">Tab one content</mat-tab>
<mat-tab label="Two">Tab two content</mat-tab>
</ng-container>
</mat-tab-group>
`,
})
class TabGroupWithIndirectDescendantTabs {
@ViewChild(MatTabGroup, {static: false}) tabGroup: MatTabGroup;
}
40 changes: 35 additions & 5 deletions src/material/tabs/tab-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ import {
mixinDisableRipple,
ThemePalette,
} from '@angular/material/core';
import {merge, Subscription} from 'rxjs';
import {MatTab} from './tab';
import {ANIMATION_MODULE_TYPE} from '@angular/platform-browser/animations';
import {merge, Subscription} from 'rxjs';
import {startWith} from 'rxjs/operators';
import {MatTab, MAT_TAB_GROUP} from './tab';


/** Used to generate unique ID's for each tab component */
Expand Down Expand Up @@ -88,10 +89,18 @@ interface MatTabGroupBaseHeader {
// tslint:disable-next-line:class-name
export abstract class _MatTabGroupBase extends _MatTabGroupMixinBase implements AfterContentInit,
AfterContentChecked, OnDestroy, CanColor, CanDisableRipple {
abstract _tabs: QueryList<MatTab>;

/**
* All tabs inside the tab group. This includes tabs that belong to groups that are nested
* inside the current one. We filter out only the tabs that belong to this group in `_tabs`.
*/
abstract _allTabs: QueryList<MatTab>;
abstract _tabBodyWrapper: ElementRef;
abstract _tabHeader: MatTabGroupBaseHeader;

/** All of the tabs that belong to the group. */
_tabs: QueryList<MatTab> = new QueryList<MatTab>();

/** The tab index that should be selected after the content has been checked. */
private _indexToSelect: number | null = 0;

Expand Down Expand Up @@ -220,6 +229,7 @@ export abstract class _MatTabGroupBase extends _MatTabGroupMixinBase implements
}

ngAfterContentInit() {
this._subscribeToAllTabChanges();
this._subscribeToTabLabels();

// Subscribe to changes in the amount of tabs, in order to be
Expand All @@ -243,11 +253,27 @@ export abstract class _MatTabGroupBase extends _MatTabGroupMixinBase implements
}
}

this._subscribeToTabLabels();
Copy link
Member Author

@crisbeto crisbeto Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this call because I noticed that there's another one just like it a few lines above. It seems like an issue when resolving merge conflicts.

this._changeDetectorRef.markForCheck();
});
}

/** Listens to changes in all of the tabs. */
private _subscribeToAllTabChanges() {
// Since we use a query with `descendants: true` to pick up the tabs, we may end up catching
// some that are inside of nested tab groups. We filter them out manually by checking that
// the closest group to the tab is the current one.
this._allTabs.changes
.pipe(startWith(this._allTabs))
.subscribe((tabs: QueryList<MatTab>) => {
this._tabs.reset(tabs.filter(tab => {
// @breaking-change 10.0.0 Remove null check for `_closestTabGroup`
// once it becomes a required parameter in MatTab.
return !tab._closestTabGroup || tab._closestTabGroup === this;
}));
this._tabs.notifyOnChanges();
});
}

ngOnDestroy() {
this._tabsSubscription.unsubscribe();
this._tabLabelSubscription.unsubscribe();
Expand Down Expand Up @@ -363,14 +389,18 @@ export abstract class _MatTabGroupBase extends _MatTabGroupMixinBase implements
// tslint:disable-next-line:validate-decorators
changeDetection: ChangeDetectionStrategy.Default,
inputs: ['color', 'disableRipple'],
providers: [{
provide: MAT_TAB_GROUP,
useExisting: MatTabGroup
}],
host: {
'class': 'mat-tab-group',
'[class.mat-tab-group-dynamic-height]': 'dynamicHeight',
'[class.mat-tab-group-inverted-header]': 'headerPosition === "below"',
},
})
export class MatTabGroup extends _MatTabGroupBase {
@ContentChildren(MatTab) _tabs: QueryList<MatTab>;
@ContentChildren(MatTab, {descendants: true}) _allTabs: QueryList<MatTab>;
@ViewChild('tabBodyWrapper', {static: false}) _tabBodyWrapper: ElementRef;
@ViewChild('tabHeader', {static: false}) _tabHeader: MatTabGroupBaseHeader;

Expand Down
17 changes: 16 additions & 1 deletion src/material/tabs/tab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ import {
ViewChild,
ViewContainerRef,
ViewEncapsulation,
InjectionToken,
Inject,
Optional,
} from '@angular/core';
import {CanDisable, CanDisableCtor, mixinDisabled} from '@angular/material/core';
import {Subject} from 'rxjs';
Expand All @@ -33,6 +36,12 @@ class MatTabBase {}
const _MatTabMixinBase: CanDisableCtor & typeof MatTabBase =
mixinDisabled(MatTabBase);

/**
* Used to provide a tab group to a tab without causing a circular dependency.
* @docs-private
*/
export const MAT_TAB_GROUP = new InjectionToken<any>('MAT_TAB_GROUP');

@Component({
moduleId: module.id,
selector: 'mat-tab',
Expand Down Expand Up @@ -107,7 +116,13 @@ export class MatTab extends _MatTabMixinBase implements OnInit, CanDisable, OnCh
*/
isActive = false;

constructor(private _viewContainerRef: ViewContainerRef) {
constructor(
private _viewContainerRef: ViewContainerRef,
/**
* @deprecated `_closestTabGroup` parameter to become required.
* @breaking-change 10.0.0
*/
@Optional() @Inject(MAT_TAB_GROUP) public _closestTabGroup?: any) {
super();
}

Expand Down
11 changes: 8 additions & 3 deletions tools/public_api_guard/material/tabs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ export declare abstract class _MatTabBodyBase implements OnInit, OnDestroy {
}

export declare abstract class _MatTabGroupBase extends _MatTabGroupMixinBase implements AfterContentInit, AfterContentChecked, OnDestroy, CanColor, CanDisableRipple {
abstract _allTabs: QueryList<MatTab>;
_animationMode?: string | undefined;
abstract _tabBodyWrapper: ElementRef;
abstract _tabHeader: MatTabGroupBaseHeader;
abstract _tabs: QueryList<MatTab>;
_tabs: QueryList<MatTab>;
readonly animationDone: EventEmitter<void>;
animationDuration: string;
backgroundColor: ThemePalette;
Expand Down Expand Up @@ -86,6 +87,8 @@ export declare abstract class _MatTabNavBase extends MatPaginatedTabHeader imple
updateActiveLink(_element?: ElementRef): void;
}

export declare const MAT_TAB_GROUP: InjectionToken<any>;

export declare const MAT_TABS_CONFIG: InjectionToken<MatTabsConfig>;

export declare class MatInkBar {
Expand All @@ -97,6 +100,7 @@ export declare class MatInkBar {
}

export declare class MatTab extends _MatTabMixinBase implements OnInit, CanDisable, OnChanges, OnDestroy {
_closestTabGroup?: any;
_explicitContent: TemplateRef<any>;
_implicitContent: TemplateRef<any>;
readonly _stateChanges: Subject<void>;
Expand All @@ -108,7 +112,8 @@ export declare class MatTab extends _MatTabMixinBase implements OnInit, CanDisab
position: number | null;
templateLabel: MatTabLabel;
textLabel: string;
constructor(_viewContainerRef: ViewContainerRef);
constructor(_viewContainerRef: ViewContainerRef,
_closestTabGroup?: any);
ngOnChanges(changes: SimpleChanges): void;
ngOnDestroy(): void;
ngOnInit(): void;
Expand Down Expand Up @@ -140,9 +145,9 @@ export declare class MatTabContent {
}

export declare class MatTabGroup extends _MatTabGroupBase {
_allTabs: QueryList<MatTab>;
_tabBodyWrapper: ElementRef;
_tabHeader: MatTabGroupBaseHeader;
_tabs: QueryList<MatTab>;
constructor(elementRef: ElementRef, changeDetectorRef: ChangeDetectorRef, defaultConfig?: MatTabsConfig, animationMode?: string);
}

Expand Down