From f16ebf54b3638c1ddd035fdc2a0d4b3abf39afa4 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 17 Jul 2018 14:15:31 +0200 Subject: [PATCH 1/2] fix(tabs): selectedIndex being overwritten if tabs are being added / removed Due to a recent change that ensures that the selected tab will be kept selected if a new tab has been added or removed (#9132), updating the `selectedIndex` at the same time will not have any effect because it will be overwritten by the `_tabs.change` (from #9132). In order to guarantee that developers can add new tabs and immediately select them once the change detection runs, we only re-index the `selectedIndex` (purpose of #9132) whenever the `indexToSelect` has not explicitly changed (through developer bindings for example) Fixes #12038 --- src/lib/tabs/tab-group.spec.ts | 34 +++++++++++++++++---------- src/lib/tabs/tab-group.ts | 42 +++++++++++++++++++++------------- 2 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/lib/tabs/tab-group.spec.ts b/src/lib/tabs/tab-group.spec.ts index 4510d2c55323..cb456caa6811 100644 --- a/src/lib/tabs/tab-group.spec.ts +++ b/src/lib/tabs/tab-group.spec.ts @@ -427,25 +427,35 @@ describe('MatTabGroup', () => { it('should maintain the selected tab if a tab is removed', () => { - // Add a couple of tabs so we have more to work with. - fixture.componentInstance.tabs.push( - {label: 'New tab', content: 'with new content'}, - {label: 'Another new tab', content: 'with newer content'} - ); - - // Select the second-to-last tab. - fixture.componentInstance.selectedIndex = 3; + // Select the second tab. + fixture.componentInstance.selectedIndex = 1; fixture.detectChanges(); const component: MatTabGroup = fixture.debugElement.query(By.css('mat-tab-group')).componentInstance; - // Remove a tab right before the selected one. - fixture.componentInstance.tabs.splice(2, 1); + // Remove the first tab that is right before the selected one. + fixture.componentInstance.tabs.splice(0, 1); fixture.detectChanges(); - expect(component.selectedIndex).toBe(1); - expect(component._tabs.toArray()[1].isActive).toBe(true); + // Since the first tab has been removed and the second one was selected before, the selected + // tab moved one position to the right. Meaning that the tab is now the first tab. + expect(component.selectedIndex).toBe(0); + expect(component._tabs.toArray()[0].isActive).toBe(true); + }); + + it('should be able to select a new tab after creation', () => { + fixture.detectChanges(); + const component: MatTabGroup = + fixture.debugElement.query(By.css('mat-tab-group')).componentInstance; + + fixture.componentInstance.tabs.push({label: 'Last tab', content: 'at the end'}); + fixture.componentInstance.selectedIndex = 3; + + fixture.detectChanges(); + + expect(component.selectedIndex).toBe(3); + expect(component._tabs.toArray()[3].isActive).toBe(true); }); it('should not fire `selectedTabChange` when the amount of tabs changes', fakeAsync(() => { diff --git a/src/lib/tabs/tab-group.ts b/src/lib/tabs/tab-group.ts index 198c43330c72..3fb6facf1512 100644 --- a/src/lib/tabs/tab-group.ts +++ b/src/lib/tabs/tab-group.ts @@ -159,12 +159,9 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn * a new selected tab should transition in (from the left or right). */ ngAfterContentChecked() { - // 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). - let indexToSelect = this._indexToSelect = - Math.min(this._tabs.length - 1, Math.max(this._indexToSelect || 0, 0)); + // Clamp the `indexToSelect` not immediately in the setter because it can happen that + // the amount of tabs changes before the actual change detection runs. + const indexToSelect = this._indexToSelect = this._clampIndexToSelect(); // If there is a change in selected index, emit a change event. Should not trigger if // the selected index has not yet been initialized. @@ -200,16 +197,21 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn // Subscribe to changes in the amount of tabs, in order to be // able to re-render the content as new tabs are added or removed. this._tabsSubscription = this._tabs.changes.subscribe(() => { - const tabs = this._tabs.toArray(); - - // Maintain the previously-selected tab if a new tab is added or removed. - for (let i = 0; i < tabs.length; i++) { - if (tabs[i].isActive) { - // Assign both to the `_indexToSelect` and `_selectedIndex` so we don't fire a changed - // event, otherwise the consumer may end up in an infinite loop in some edge cases like - // adding a tab within the `selectedIndexChange` event. - this._indexToSelect = this._selectedIndex = i; - break; + const indexToSelect = this._clampIndexToSelect(); + + // Maintain the previously-selected tab if a new tab is added or removed and there is no + // explicit change that selects a different tab. + if (indexToSelect === this._selectedIndex) { + const tabs = this._tabs.toArray(); + + for (let i = 0; i < tabs.length; i++) { + if (tabs[i].isActive) { + // Assign both to the `_indexToSelect` and `_selectedIndex` so we don't fire a changed + // event, otherwise the consumer may end up in an infinite loop in some edge cases like + // adding a tab within the `selectedIndexChange` event. + this._indexToSelect = this._selectedIndex = i; + break; + } } } @@ -261,6 +263,14 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn }); } + /** Clamps the indexToSelect to the bounds of 0 and the tabs length. */ + private _clampIndexToSelect(): number { + // 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). + return Math.min(this._tabs.length - 1, Math.max(this._indexToSelect || 0, 0)); + } + /** Returns a unique id for each tab label element */ _getTabLabelId(i: number): string { return `mat-tab-label-${this._groupId}-${i}`; From f73f459d33da4230dfcbc10dd3d4b477cbcbd321 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 17 Jul 2018 14:39:27 +0200 Subject: [PATCH 2/2] Address comments --- src/lib/tabs/tab-group.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/lib/tabs/tab-group.ts b/src/lib/tabs/tab-group.ts index 3fb6facf1512..5ef2df2273d1 100644 --- a/src/lib/tabs/tab-group.ts +++ b/src/lib/tabs/tab-group.ts @@ -159,9 +159,9 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn * a new selected tab should transition in (from the left or right). */ ngAfterContentChecked() { - // Clamp the `indexToSelect` not immediately in the setter because it can happen that + // Don't clamp the `indexToSelect` immediately in the setter because it can happen that // the amount of tabs changes before the actual change detection runs. - const indexToSelect = this._indexToSelect = this._clampIndexToSelect(); + const indexToSelect = this._indexToSelect = this._clampTabIndex(this._indexToSelect); // If there is a change in selected index, emit a change event. Should not trigger if // the selected index has not yet been initialized. @@ -197,7 +197,7 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn // Subscribe to changes in the amount of tabs, in order to be // able to re-render the content as new tabs are added or removed. this._tabsSubscription = this._tabs.changes.subscribe(() => { - const indexToSelect = this._clampIndexToSelect(); + const indexToSelect = this._clampTabIndex(this._indexToSelect); // Maintain the previously-selected tab if a new tab is added or removed and there is no // explicit change that selects a different tab. @@ -263,12 +263,12 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn }); } - /** Clamps the indexToSelect to the bounds of 0 and the tabs length. */ - private _clampIndexToSelect(): number { + /** Clamps the given index to the bounds of 0 and the tabs length. */ + private _clampTabIndex(index: number | null): number { // 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). - return Math.min(this._tabs.length - 1, Math.max(this._indexToSelect || 0, 0)); + return Math.min(this._tabs.length - 1, Math.max(index || 0, 0)); } /** Returns a unique id for each tab label element */