From 5b510deda71f58be044946391d62794a246b16bd Mon Sep 17 00:00:00 2001 From: Austin Date: Sun, 27 Aug 2017 13:55:41 -0500 Subject: [PATCH 1/5] bug(tabs): fix inifinite tab loop #4639 --- src/lib/tabs/tab-group.spec.ts | 19 +++++++++++++++++++ src/lib/tabs/tab-group.ts | 15 ++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/lib/tabs/tab-group.spec.ts b/src/lib/tabs/tab-group.spec.ts index d30a5835a8b6..64e5efeeef79 100644 --- a/src/lib/tabs/tab-group.spec.ts +++ b/src/lib/tabs/tab-group.spec.ts @@ -70,6 +70,25 @@ describe('MdTabGroup', () => { }); })); + fit('should support two-way binding for selectedIndex', async(() => { + let component = fixture.componentInstance; + component.selectedIndex = 0; + fixture.detectChanges(); + + setTimeout(() => { + component.selectedIndex = 1; + fixture.detectChanges(); + + setTimeout(() => { + component.selectedIndex = 0; + fixture.detectChanges(); + fixture.whenStable().then(() => { + expect(component.selectedIndex).toBe(0); + }); + }, 1); + }, 1); + })); + it('should change tabs based on selectedIndex', fakeAsync(() => { let component = fixture.componentInstance; let tabComponent = fixture.debugElement.query(By.css('md-tab-group')).componentInstance; diff --git a/src/lib/tabs/tab-group.ts b/src/lib/tabs/tab-group.ts index 8205638fcdee..07c6cb4d52e7 100644 --- a/src/lib/tabs/tab-group.ts +++ b/src/lib/tabs/tab-group.ts @@ -131,9 +131,7 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit private _backgroundColor: ThemePalette; /** Output to enable support for two-way binding on `[(selectedIndex)]` */ - @Output() get selectedIndexChange(): Observable { - return map.call(this.selectChange, event => event.index); - } + @Output() selectedIndexChange = new EventEmitter(); /** Event emitted when focus has changed within a tab group. */ @Output() focusChange: EventEmitter = new EventEmitter(); @@ -157,9 +155,10 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit * a new selected tab should transition in (from the left or right). */ ngAfterContentChecked(): void { - // Clamp the next selected index to the bounds of 0 and the tabs length. Note the `|| 0`, which - // ensures that values like NaN can't get through and which would otherwise throw the - // component into an infinite loop (since Math.max(NaN, 0) === NaN). + // Clamp the next selected index to the boundsof 0 and the tabs length. + // Note the `|| 0`, which ensures that values like NaN can't get through + // and which would otherwise throw the component into an infinite loop + // (since Math.max(NaN, 0) === NaN). let indexToSelect = this._indexToSelect = Math.min(this._tabs.length - 1, Math.max(this._indexToSelect || 0, 0)); @@ -167,9 +166,11 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit // the selected index has not yet been initialized. if (this._selectedIndex != indexToSelect && this._selectedIndex != null) { this.selectChange.emit(this._createChangeEvent(indexToSelect)); + // prevent expression changed error + setTimeout(() => this.selectedIndexChange.emit(indexToSelect)); } - // Setup the position for each tab and optionally setup an origin on the next selected tab. + // Setup the position for each tab and optionally setup an origin on the next selected tab. this._tabs.forEach((tab: MdTab, index: number) => { tab.position = index - indexToSelect; tab.isActive = index === indexToSelect; From bd0adeefa1a468120853ddcbf61a6c5086e825f3 Mon Sep 17 00:00:00 2001 From: Austin Date: Sun, 27 Aug 2017 13:58:05 -0500 Subject: [PATCH 2/5] chore(nit): fix tslint issues --- src/lib/tabs/tab-group.spec.ts | 2 +- src/lib/tabs/tab-group.ts | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/lib/tabs/tab-group.spec.ts b/src/lib/tabs/tab-group.spec.ts index 64e5efeeef79..2f244e882d64 100644 --- a/src/lib/tabs/tab-group.spec.ts +++ b/src/lib/tabs/tab-group.spec.ts @@ -70,7 +70,7 @@ describe('MdTabGroup', () => { }); })); - fit('should support two-way binding for selectedIndex', async(() => { + it('should support two-way binding for selectedIndex', async(() => { let component = fixture.componentInstance; component.selectedIndex = 0; fixture.detectChanges(); diff --git a/src/lib/tabs/tab-group.ts b/src/lib/tabs/tab-group.ts index 07c6cb4d52e7..e8ec9262e120 100644 --- a/src/lib/tabs/tab-group.ts +++ b/src/lib/tabs/tab-group.ts @@ -25,8 +25,6 @@ import { ViewEncapsulation, } from '@angular/core'; import {coerceBooleanProperty} from '@angular/cdk/coercion'; -import {map} from '@angular/cdk/rxjs'; -import {Observable} from 'rxjs/Observable'; import {Subscription} from 'rxjs/Subscription'; import {MdTab} from './tab'; import {merge} from 'rxjs/observable/merge'; From 8c7c909b5811f48368e0b3b8137d3e1e244e21a3 Mon Sep 17 00:00:00 2001 From: Austin Date: Mon, 28 Aug 2017 16:48:21 -0500 Subject: [PATCH 3/5] chore(nit): replace timeout w/ zone run --- src/lib/tabs/tab-group.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib/tabs/tab-group.ts b/src/lib/tabs/tab-group.ts index e8ec9262e120..79d56d51b2b3 100644 --- a/src/lib/tabs/tab-group.ts +++ b/src/lib/tabs/tab-group.ts @@ -23,6 +23,7 @@ import { AfterContentChecked, OnDestroy, ViewEncapsulation, + NgZone, } from '@angular/core'; import {coerceBooleanProperty} from '@angular/cdk/coercion'; import {Subscription} from 'rxjs/Subscription'; @@ -141,6 +142,7 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit constructor(_renderer: Renderer2, elementRef: ElementRef, + private _zone: NgZone, private _changeDetectorRef: ChangeDetectorRef) { super(_renderer, elementRef); this._groupId = nextId++; @@ -165,7 +167,7 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit if (this._selectedIndex != indexToSelect && this._selectedIndex != null) { this.selectChange.emit(this._createChangeEvent(indexToSelect)); // prevent expression changed error - setTimeout(() => this.selectedIndexChange.emit(indexToSelect)); + this._zone.run(() => this.selectedIndexChange.emit(indexToSelect)); } // Setup the position for each tab and optionally setup an origin on the next selected tab. From 68cd5d58139357f59074d3f708b79efa39894edf Mon Sep 17 00:00:00 2001 From: Austin Date: Mon, 28 Aug 2017 17:36:14 -0500 Subject: [PATCH 4/5] chore(nit): fix comments --- src/lib/tabs/tab-group.spec.ts | 2 +- src/lib/tabs/tab-group.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib/tabs/tab-group.spec.ts b/src/lib/tabs/tab-group.spec.ts index 2f244e882d64..439367351a20 100644 --- a/src/lib/tabs/tab-group.spec.ts +++ b/src/lib/tabs/tab-group.spec.ts @@ -70,7 +70,7 @@ describe('MdTabGroup', () => { }); })); - it('should support two-way binding for selectedIndex', async(() => { + it('should set to correct tab on fast change', async(() => { let component = fixture.componentInstance; component.selectedIndex = 0; fixture.detectChanges(); diff --git a/src/lib/tabs/tab-group.ts b/src/lib/tabs/tab-group.ts index 79d56d51b2b3..1f2be74684a4 100644 --- a/src/lib/tabs/tab-group.ts +++ b/src/lib/tabs/tab-group.ts @@ -166,11 +166,12 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit // the selected index has not yet been initialized. if (this._selectedIndex != indexToSelect && this._selectedIndex != null) { this.selectChange.emit(this._createChangeEvent(indexToSelect)); - // prevent expression changed error + // Emitting this value after change detection has run + // since the checked content may contain this variable this._zone.run(() => this.selectedIndexChange.emit(indexToSelect)); } - // Setup the position for each tab and optionally setup an origin on the next selected tab. + // Setup the position for each tab and optionally setup an origin on the next selected tab. this._tabs.forEach((tab: MdTab, index: number) => { tab.position = index - indexToSelect; tab.isActive = index === indexToSelect; From 04153de6fa8323c2627e0bc0c89242fc8a2495a6 Mon Sep 17 00:00:00 2001 From: Austin Date: Wed, 30 Aug 2017 18:19:35 -0500 Subject: [PATCH 5/5] chore(nit): tweaks per feedback --- src/lib/tabs/tab-group.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/lib/tabs/tab-group.ts b/src/lib/tabs/tab-group.ts index 1f2be74684a4..9fb8317c8c36 100644 --- a/src/lib/tabs/tab-group.ts +++ b/src/lib/tabs/tab-group.ts @@ -23,7 +23,6 @@ import { AfterContentChecked, OnDestroy, ViewEncapsulation, - NgZone, } from '@angular/core'; import {coerceBooleanProperty} from '@angular/cdk/coercion'; import {Subscription} from 'rxjs/Subscription'; @@ -130,7 +129,7 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit private _backgroundColor: ThemePalette; /** Output to enable support for two-way binding on `[(selectedIndex)]` */ - @Output() selectedIndexChange = new EventEmitter(); + @Output() selectedIndexChange: EventEmitter = new EventEmitter(); /** Event emitted when focus has changed within a tab group. */ @Output() focusChange: EventEmitter = new EventEmitter(); @@ -142,7 +141,6 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit constructor(_renderer: Renderer2, elementRef: ElementRef, - private _zone: NgZone, private _changeDetectorRef: ChangeDetectorRef) { super(_renderer, elementRef); this._groupId = nextId++; @@ -167,8 +165,8 @@ export class MdTabGroup extends _MdTabGroupMixinBase implements AfterContentInit if (this._selectedIndex != indexToSelect && this._selectedIndex != null) { this.selectChange.emit(this._createChangeEvent(indexToSelect)); // Emitting this value after change detection has run - // since the checked content may contain this variable - this._zone.run(() => this.selectedIndexChange.emit(indexToSelect)); + // since the checked content may contain this variable' + Promise.resolve().then(() => this.selectedIndexChange.emit(indexToSelect)); } // Setup the position for each tab and optionally setup an origin on the next selected tab.