bug(tabs): fix inifinite tab loop #4639 #6663
Conversation
googlebot
added
the
cla: yes
src/lib/tabs/tab-group.ts
Outdated
| } | ||
|
|
||
| // 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. |
There was a problem hiding this comment.
Remove line change - accidental indent
src/lib/tabs/tab-group.spec.ts
Outdated
| @@ -70,6 +70,25 @@ describe('MdTabGroup', () => { | |||
| }); | |||
| })); | |||
|
|
|||
| it('should support two-way binding for selectedIndex', async(() => { | |||
There was a problem hiding this comment.
Description matches the previous unit test
src/lib/tabs/tab-group.ts
Outdated
| let indexToSelect = this._indexToSelect = | ||
| Math.min(this._tabs.length - 1, Math.max(this._indexToSelect || 0, 0)); | ||
|
|
||
| // 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) { | ||
| this.selectChange.emit(this._createChangeEvent(indexToSelect)); | ||
| // prevent expression changed error | ||
| this._zone.run(() => this.selectedIndexChange.emit(indexToSelect)); |
There was a problem hiding this comment.
This makes me wonder if we should bother clamping the tab index, which is the reason why wait for checked content to emit this value. Maybe we should be allowing the dev to input a wrong index like how we do with paginator.
@jelbourn Tabs are trying to make sure they do not get into an error state. If the selectedIndex Input is set to 10 but there are only 5 tabs, we clamp it down to the last tab. This means we need to wait for content to be checked so we know how many tabs there are. What do you think about removing this convenience and just letting the dev select tab 10 and let the tab group fail?
There was a problem hiding this comment.
Personally I prefer safety checking but if we do not do that on purpose in other places, we should follow the pattern IMO.
Removing that check, could clean this up though.
src/lib/tabs/tab-group.ts
Outdated
| let indexToSelect = this._indexToSelect = | ||
| Math.min(this._tabs.length - 1, Math.max(this._indexToSelect || 0, 0)); | ||
|
|
||
| // 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) { | ||
| this.selectChange.emit(this._createChangeEvent(indexToSelect)); | ||
| // prevent expression changed error |
There was a problem hiding this comment.
Change comment: "Emitting this value after change detection has run since the checked content may contain this variable"
| // 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. |
There was a problem hiding this comment.
Did the comment change?
There was a problem hiding this comment.
No, I had changed it and had to reformat it to fix length but then backed that out and didn't undo the reformat. Sorry.
| component.selectedIndex = 0; | ||
| fixture.detectChanges(); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
Why is this setTimeout instead of using fixture.whenStable()?
There was a problem hiding this comment.
I used setTimeout because thats what his demo used.
src/lib/tabs/tab-group.ts
Outdated
| @Output() get selectedIndexChange(): Observable<number> { | ||
| return map.call(this.selectChange, event => event.index); | ||
| } | ||
| @Output() selectedIndexChange = new EventEmitter(); |
There was a problem hiding this comment.
Did you mean to omit the emitted type?
src/lib/tabs/tab-group.ts
Outdated
| let indexToSelect = this._indexToSelect = | ||
| Math.min(this._tabs.length - 1, Math.max(this._indexToSelect || 0, 0)); | ||
|
|
||
| // 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) { | ||
| 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)); |
There was a problem hiding this comment.
It's odd to use zone.run like this; does it work if you use Promise.resolve().then(() => ...) ?
There was a problem hiding this comment.
That does work, nifty trick.
|
@jelbourn Ready for another review. |
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
@jelbourn @andrewseguin - The issue is the
selectChangeevent is causing thengAfterContentCheckedto get recursively called because this change event is firing so quickly.The root cause seems to be how mapping of the
selectedIndexChangeevent was hooked up:switching this to its own
EventEmitterresolved the problem. However, that created an issue with a expression changed error, I was able to patch that by adding a timeout around that event emit and solve the issue.