Skip to content

Commit

Permalink
ng: fix infinite loop when on plugin selector (#3563)
Browse files Browse the repository at this point in the history
cause: mat-tab-group emits the changed event in the Angular's equivalent
to `componentDidUpdate`. The problem is that:

- it emits the change in a promise (next tick)
- it emits the change when the value is changed programmatically.

While lacking complete understanding of the flow, empirically, I found there
to be some kind of event dispatch queue and the emission to not invoke our
callback immediately.

fix: no longer use the mat-tab-group's change event and listen directly
at the click event.

Fixes b/155104143.
  • Loading branch information
stephanwlee authored Apr 30, 2020
1 parent 7fde702 commit e6c4355
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 11 deletions.
2 changes: 1 addition & 1 deletion tensorboard/webapp/header/header_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ describe('header test', () => {
const fixture = TestBed.createComponent(HeaderComponent);
fixture.detectChanges();

const [, barEl] = fixture.debugElement.queryAll(By.css('.mat-tab-label'));
const [, barEl] = fixture.debugElement.queryAll(By.css('.plugin-name'));
barEl.nativeElement.click();
fixture.detectChanges();
await fixture.whenStable();
Expand Down
18 changes: 12 additions & 6 deletions tensorboard/webapp/header/plugin_selector_component.ng.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,20 @@
<mat-tab-group
class="active-plugin-list"
[selectedIndex]="getActivePluginIndex()"
(selectedTabChange)="onActivePluginSelectionChanged($event)"
animationDuration="100ms"
>
<mat-tab
*ngFor="let plugin of activePlugins"
[label]="plugin.tab_name"
[disabled]="!plugin.enabled"
>
<mat-tab *ngFor="let plugin of activePlugins" [disabled]="!plugin.enabled">
<ng-template mat-tab-label>
<!-- Manually subscribe to the click event on the tab content element.
Cannot trust the selectedTabChange event since it is async and can cause
infinte loop. -->
<span
class="plugin-name"
(click)="onActivePluginSelection($event, plugin.id)"
>
{{plugin.tab_name}}
</span>
</ng-template>
</mat-tab>
</mat-tab-group>
<mat-form-field floatLabel="never" *ngIf="disabledPlugins.length > 0">
Expand Down
15 changes: 14 additions & 1 deletion tensorboard/webapp/header/plugin_selector_component.scss
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,15 @@ mat-option {
overflow: hidden;
}

.plugin-name {
align-items: center;
display: inline-flex;
height: 100%;
justify-content: center;
padding: 0 12px;
width: 100%;
}

:host ::ng-deep .active-plugin-list {
// Override mat-tab styling. By default, mat-tab has the right styling but,
// here, we are using it under dark header background. Must invert the color.
Expand Down Expand Up @@ -82,10 +91,14 @@ mat-option {

.mat-tab-label {
min-width: 48px; /* default is 160px which is too big for us */
padding: 0 12px; /* default is 24px */
padding: 0; /* default is 24px */
text-transform: uppercase;
}

.mat-tab-label-content {
height: 100%;
}

mat-tab-header:not(.mat-tab-header-pagination-controls-enabled) {
padding: 0 36px;
}
Expand Down
6 changes: 3 additions & 3 deletions tensorboard/webapp/header/plugin_selector_component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {Component, Input, Output, EventEmitter} from '@angular/core';
import {MatTabChangeEvent} from '@angular/material/tabs';
import {MatSelectChange} from '@angular/material/select';

import {PluginId} from '../types/api';
Expand Down Expand Up @@ -41,8 +40,9 @@ export class PluginSelectorComponent {
return this.activePlugins.findIndex(({id}) => id === this.selectedPlugin);
}

onActivePluginSelectionChanged({index}: MatTabChangeEvent) {
this.onPluginSelectionChanged.emit(this.activePlugins[index].id);
onActivePluginSelection(event: Event, pluginId: PluginId) {
event.stopPropagation();
this.onPluginSelectionChanged.emit(pluginId);
}

onDisabledPluginSelectionChanged(selectChangeEvent: MatSelectChange) {
Expand Down

0 comments on commit e6c4355

Please sign in to comment.