Skip to content

fix(tabs): changed after checked error when using isActive in view #12206

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
Aug 13, 2018
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
40 changes: 36 additions & 4 deletions src/lib/tabs/tab-group.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@ import {Component, OnInit, QueryList, ViewChild, ViewChildren} from '@angular/co
import {async, ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing';
import {By} from '@angular/platform-browser';
import {BrowserAnimationsModule, NoopAnimationsModule} from '@angular/platform-browser/animations';
import {CommonModule} from '@angular/common';
import {Observable} from 'rxjs';
import {MatTab, MatTabGroup, MatTabHeaderPosition, MatTabsModule} from './index';


describe('MatTabGroup', () => {
beforeEach(fakeAsync(() => {
TestBed.configureTestingModule({
imports: [MatTabsModule, NoopAnimationsModule],
imports: [MatTabsModule, CommonModule, NoopAnimationsModule],
declarations: [
SimpleTabsTestApp,
SimpleDynamicTabsTestApp,
Expand All @@ -20,6 +21,7 @@ describe('MatTabGroup', () => {
TabGroupWithSimpleApi,
TemplateTabs,
TabGroupWithAriaInputs,
TabGroupWithIsActiveBinding,
],
});

Expand Down Expand Up @@ -194,8 +196,9 @@ describe('MatTabGroup', () => {
.toBe(0, 'Expected no ripple to show up on label mousedown.');
});

it('should set the isActive flag on each of the tabs', () => {
it('should set the isActive flag on each of the tabs', fakeAsync(() => {
fixture.detectChanges();
tick();

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

Expand All @@ -205,11 +208,12 @@ describe('MatTabGroup', () => {

fixture.componentInstance.selectedIndex = 2;
fixture.detectChanges();
tick();

expect(tabs[0].isActive).toBe(false);
expect(tabs[1].isActive).toBe(false);
expect(tabs[2].isActive).toBe(true);
});
}));

it('should fire animation done event', fakeAsync(() => {
fixture.detectChanges();
Expand Down Expand Up @@ -512,7 +516,21 @@ describe('MatTabGroup', () => {
const child = fixture.debugElement.query(By.css('.child'));
expect(child.nativeElement).toBeDefined();
}));
});
});

describe('special cases', () => {
it('should not throw an error when binding isActive to the view', fakeAsync(() => {
const fixture = TestBed.createComponent(TabGroupWithIsActiveBinding);

expect(() => {
fixture.detectChanges();
tick();
fixture.detectChanges();
}).not.toThrow();

expect(fixture.nativeElement.textContent).toContain('pizza is active');
}));
});

/**
* Checks that the `selectedIndex` has been updated; checks that the label and body have their
Expand Down Expand Up @@ -773,3 +791,17 @@ class TabGroupWithAriaInputs {
ariaLabel: string;
ariaLabelledby: string;
}


@Component({
template: `
<mat-tab-group>
<mat-tab label="Junk food" #pizza> Pizza, fries </mat-tab>
<mat-tab label="Vegetables"> Broccoli, spinach </mat-tab>
</mat-tab-group>

<div *ngIf="pizza.isActive">pizza is active</div>
`
})
class TabGroupWithIsActiveBinding {
}
30 changes: 18 additions & 12 deletions src/lib/tabs/tab-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,18 +168,27 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn

// If there is a change in selected index, emit a change event. Should not trigger if
// the selected index has not yet been initialized.
if (this._selectedIndex != indexToSelect && this._selectedIndex != null) {
const tabChangeEvent = this._createChangeEvent(indexToSelect);
this.selectedTabChange.emit(tabChangeEvent);
// Emitting this value after change detection has run
// since the checked content may contain this variable'
Promise.resolve().then(() => this.selectedIndexChange.emit(indexToSelect));
if (this._selectedIndex != indexToSelect) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we use !== here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what was being used here before so I kept it. I'm guessing it was there to handle somebody passing the index in as a string.

const isFirstRun = this._selectedIndex == null;

if (!isFirstRun) {
this.selectedTabChange.emit(this._createChangeEvent(indexToSelect));
}

// Changing these values after change detection has run
// since the checked content may contain references to them.
Promise.resolve().then(() => {
this._tabs.forEach((tab, index) => tab.isActive = index === indexToSelect);

if (!isFirstRun) {
this.selectedIndexChange.emit(indexToSelect);
}
});
}

// Setup the position for each tab and optionally setup an origin on the next selected tab.
this._tabs.forEach((tab: MatTab, index: number) => {
tab.position = index - indexToSelect;
tab.isActive = index === indexToSelect;

// If there is already a selected tab, then set up an origin for the next selected tab
// if it doesn't have one already.
Expand Down Expand Up @@ -254,11 +263,8 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn
this._tabLabelSubscription.unsubscribe();
}

this._tabLabelSubscription = merge(
...this._tabs.map(tab => tab._disableChange),
...this._tabs.map(tab => tab._labelChange)).subscribe(() => {
this._changeDetectorRef.markForCheck();
});
this._tabLabelSubscription = merge(...this._tabs.map(tab => tab._stateChanges))
.subscribe(() => this._changeDetectorRef.markForCheck());
}

/** Returns a unique id for each tab label element */
Expand Down
18 changes: 5 additions & 13 deletions src/lib/tabs/tab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,8 @@ export class MatTab extends _MatTabMixinBase implements OnInit, CanDisable, OnCh
return this._contentPortal;
}

/** Emits whenever the label changes. */
readonly _labelChange = new Subject<void>();

/** Emits whenever the disable changes */
readonly _disableChange = new Subject<void>();
/** Emits whenever the internal state of the tab changes. */
readonly _stateChanges = new Subject<void>();

/**
* The relatively indexed position where 0 represents the center, negative is left, and positive
Expand All @@ -101,18 +98,13 @@ export class MatTab extends _MatTabMixinBase implements OnInit, CanDisable, OnCh
}

ngOnChanges(changes: SimpleChanges): void {
if (changes.hasOwnProperty('textLabel')) {
this._labelChange.next();
}

if (changes.hasOwnProperty('disabled')) {
this._disableChange.next();
if (changes.hasOwnProperty('textLabel') || changes.hasOwnProperty('disabled')) {
this._stateChanges.next();
}
}

ngOnDestroy(): void {
this._disableChange.complete();
this._labelChange.complete();
this._stateChanges.complete();
}

ngOnInit(): void {
Expand Down