Skip to content

Commit 0036372

Browse files
authored
Bug Fix: Prevent custom modal from being closed until after it is opened (#6523)
## Motivation for features / changes In #6504 I introduced a bug to the column selector modal when it was opened via the + button. The effect was that the modal was being closed in the same event that opened it. Rather than changing the way that specific event was being dispatched I instead am preventing the modal from being closed until at least a single frame has been rendered. ## Screenshots of UI changes (or N/A) It didn't work before ![3f02da9d-148b-44d8-bc1b-7951bb789808](https://github.com/tensorflow/tensorboard/assets/78179109/77ee2fc6-8a1b-4280-a224-20055ce6d488) It works now ![a17b2e80-40c6-4bd4-accf-40ee5cdd4b47](https://github.com/tensorflow/tensorboard/assets/78179109/31aab2fd-ad1b-4d04-ac82-731c737a0627)
1 parent 9c9d1e1 commit 0036372

File tree

2 files changed

+11
-21
lines changed

2 files changed

+11
-21
lines changed

tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,18 +56,20 @@ export class CustomModalComponent implements OnInit {
5656
@Output() onClose = new EventEmitter<void>();
5757

5858
readonly visible$ = new BehaviorSubject(false);
59+
private canClose = true;
5960

6061
@ViewChild('content', {static: false})
6162
private readonly content!: ElementRef;
6263

63-
private clickListener: () => void = this.close.bind(this);
64+
private clickListener: () => void = this.maybeClose.bind(this);
6465

6566
constructor(private readonly viewRef: ViewContainerRef) {}
6667

6768
ngOnInit() {
6869
this.visible$.subscribe((visible) => {
6970
// Wait until after the next browser frame.
7071
window.requestAnimationFrame(() => {
72+
this.canClose = true;
7173
if (visible) {
7274
this.ensureContentIsWithinWindow();
7375
this.onOpen.emit();
@@ -87,6 +89,7 @@ export class CustomModalComponent implements OnInit {
8789

8890
this.content.nativeElement.style.left = position.x + 'px';
8991
this.content.nativeElement.style.top = position.y + 'px';
92+
this.canClose = false;
9093
this.visible$.next(true);
9194
document.addEventListener('click', this.clickListener);
9295
}
@@ -115,6 +118,13 @@ export class CustomModalComponent implements OnInit {
115118
}
116119

117120
@HostListener('document:keydown.escape', ['$event'])
121+
private maybeClose() {
122+
if (!this.canClose) {
123+
return;
124+
}
125+
this.close();
126+
}
127+
118128
public close() {
119129
document.removeEventListener('click', this.clickListener);
120130
this.visible$.next(false);

tensorboard/webapp/widgets/data_table/data_table_test.ts

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -862,25 +862,5 @@ describe('data table', () => {
862862
contextMenu.nativeElement.innerHTML.includes('No Actions Available')
863863
).toBeTrue();
864864
});
865-
866-
it('closes when the + button is clicked', () => {
867-
const fixture = createComponent({
868-
headers: mockHeaders,
869-
data: mockTableData,
870-
potentialColumns: mockPotentialColumns,
871-
});
872-
const fixedHeader = fixture.debugElement.queryAll(
873-
By.directive(HeaderCellComponent)
874-
)[0];
875-
fixedHeader.nativeElement.dispatchEvent(new MouseEvent('contextmenu'));
876-
fixture.detectChanges();
877-
expect(fixture.debugElement.query(By.css('.context-menu'))).toBeTruthy();
878-
879-
const addBtn = fixture.debugElement.query(By.css('.add-column-btn'));
880-
addBtn.nativeElement.click();
881-
fixture.detectChanges();
882-
883-
expect(fixture.debugElement.query(By.css('.context-menu'))).toBeFalsy();
884-
});
885865
});
886866
});

0 commit comments

Comments
 (0)