diff --git a/tensorboard/webapp/angular/BUILD b/tensorboard/webapp/angular/BUILD index 5b92205186..71db97e5dc 100644 --- a/tensorboard/webapp/angular/BUILD +++ b/tensorboard/webapp/angular/BUILD @@ -364,6 +364,15 @@ tf_ts_library( ], ) +# This is a dummy rule used as a @angular/cdk/portal dependency. +tf_ts_library( + name = "expect_angular_cdk_portal", + srcs = [], + deps = [ + "@npm//@angular/cdk", + ], +) + # This is a dummy rule used as a @angular/cdk/scrolling dependency. tf_ts_library( name = "expect_angular_cdk_scrolling", diff --git a/tensorboard/webapp/runs/views/runs_table/filterbar_component.ng.html b/tensorboard/webapp/runs/views/runs_table/filterbar_component.ng.html index 00c6034e2c..d9a317e7e2 100644 --- a/tensorboard/webapp/runs/views/runs_table/filterbar_component.ng.html +++ b/tensorboard/webapp/runs/views/runs_table/filterbar_component.ng.html @@ -31,12 +31,11 @@ - + - + diff --git a/tensorboard/webapp/runs/views/runs_table/filterbar_component.ts b/tensorboard/webapp/runs/views/runs_table/filterbar_component.ts index 29dd125abd..369cf2e838 100644 --- a/tensorboard/webapp/runs/views/runs_table/filterbar_component.ts +++ b/tensorboard/webapp/runs/views/runs_table/filterbar_component.ts @@ -19,6 +19,8 @@ import { EventEmitter, Component, ViewChild, + TemplateRef, + ViewContainerRef, } from '@angular/core'; import { DiscreteFilter, @@ -27,7 +29,7 @@ import { FilterAddedEvent, } from '../../../widgets/data_table/types'; import {RangeValues} from '../../../widgets/range_input/types'; -import {CustomModalComponent} from '../../../widgets/custom_modal/custom_modal_component'; +import {CustomModal} from '../../../widgets/custom_modal/custom_modal'; @Component({ selector: 'filterbar-component', @@ -41,8 +43,8 @@ export class FilterbarComponent { @Output() removeHparamFilter = new EventEmitter(); @Output() addFilter = new EventEmitter(); - @ViewChild('filterModal', {static: false}) - private readonly filterModal!: CustomModalComponent; + @ViewChild('filterModalTemplate', {read: TemplateRef}) + filterModalTemplate!: TemplateRef; private internalSelectedFilterName = ''; get selectedFilterName(): string { @@ -56,19 +58,18 @@ export class FilterbarComponent { return this.filters.get(this.selectedFilterName); } + constructor( + private readonly customModal: CustomModal, + private readonly viewContainerRef: ViewContainerRef + ) {} + openFilterMenu(event: MouseEvent, filterName: string) { this.selectedFilterName = filterName; - const rect = ( - (event.target as HTMLElement).closest('mat-chip') as HTMLElement - ).getBoundingClientRect(); - this.filterModal.openAtPosition({ - x: rect.x + rect.width, - y: rect.y, - }); - } - - deselectFilter() { - this.selectedFilterName = ''; + this.customModal.createNextToElement( + this.filterModalTemplate, + (event.target as HTMLElement).closest('mat-chip') as HTMLElement, + this.viewContainerRef + ); } emitIntervalFilterChanged(value: RangeValues) { diff --git a/tensorboard/webapp/runs/views/runs_table/filterbar_test.ts b/tensorboard/webapp/runs/views/runs_table/filterbar_test.ts index d3966cf3d2..ff6e547e2d 100644 --- a/tensorboard/webapp/runs/views/runs_table/filterbar_test.ts +++ b/tensorboard/webapp/runs/views/runs_table/filterbar_test.ts @@ -13,7 +13,7 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {ComponentFixture, TestBed} from '@angular/core/testing'; -import {NO_ERRORS_SCHEMA} from '@angular/core'; +import {Component, NO_ERRORS_SCHEMA} from '@angular/core'; import {FilterbarComponent} from './filterbar_component'; import {FilterbarContainer} from './filterbar_container'; import {NoopAnimationsModule} from '@angular/platform-browser/animations'; @@ -22,7 +22,6 @@ import {MockStore} from '@ngrx/store/testing'; import {State} from '../../../app_state'; import {Action, Store} from '@ngrx/store'; import {By} from '@angular/platform-browser'; -import {CustomModalModule} from '../../../widgets/custom_modal/custom_modal_module'; import { actions as hparamsActions, selectors as hparamsSelectors, @@ -36,9 +35,9 @@ import {MatChipHarness} from '@angular/material/chips/testing'; import {TestbedHarnessEnvironment} from '@angular/cdk/testing/testbed'; import {MatChipRemove, MatChipsModule} from '@angular/material/chips'; import {MatIconTestingModule} from '../../../testing/mat_icon_module'; -import {CustomModalComponent} from '../../../widgets/custom_modal/custom_modal_component'; import {FilterDialogModule} from '../../../widgets/data_table/filter_dialog_module'; import {FilterDialog} from '../../../widgets/data_table/filter_dialog_component'; +import {CustomModal} from '../../../widgets/custom_modal/custom_modal'; const discreteFilter: DiscreteFilter = { type: DomainType.DISCRETE, @@ -61,6 +60,14 @@ const fakeFilterMap = new Map([ ['filter2', intervalFilter], ]); +@Component({ + selector: 'testable-component', + template: ` `, +}) +class TestableComponent { + constructor(readonly customModal: CustomModal) {} +} + describe('hparam_filterbar', () => { let actualActions: Action[]; let store: MockStore; @@ -69,13 +76,12 @@ describe('hparam_filterbar', () => { beforeEach(async () => { await TestBed.configureTestingModule({ imports: [ - CustomModalModule, NoopAnimationsModule, MatChipsModule, MatIconTestingModule, FilterDialogModule, ], - declarations: [FilterbarComponent, FilterbarContainer], + declarations: [FilterbarComponent, FilterbarContainer, TestableComponent], providers: [provideMockTbStore()], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); @@ -85,23 +91,26 @@ describe('hparam_filterbar', () => { store?.resetSelectors(); }); - function createComponent(): ComponentFixture { + function createComponent(): ComponentFixture { store = TestBed.inject>(Store) as MockStore; actualActions = []; dispatchSpy = spyOn(store, 'dispatch').and.callFake((action: Action) => { actualActions.push(action); }); - return TestBed.createComponent(FilterbarContainer); + const fixture = TestBed.createComponent(TestableComponent); + return fixture; } it('renders hparam filterbar', () => { const fixture = createComponent(); fixture.detectChanges(); - const dialog = fixture.debugElement.query(By.directive(FilterbarComponent)); + const filterBarComponent = fixture.debugElement.query( + By.directive(FilterbarComponent) + ); - expect(dialog).toBeTruthy(); + expect(filterBarComponent).toBeTruthy(); }); it("doesn't render if no filters are set", async () => { @@ -164,23 +173,23 @@ describe('hparam_filterbar', () => { const component = fixture.debugElement.query( By.directive(FilterbarComponent) ).componentInstance; - const openAtPositionSpy = spyOn( - CustomModalComponent.prototype, - 'openAtPosition' + const createNextToElementSpy = spyOn( + TestBed.inject(CustomModal), + 'createNextToElement' ); const loader = TestbedHarnessEnvironment.loader(fixture); fixture.detectChanges(); const chipHarness = await loader.getHarness(MatChipHarness); const chip = await chipHarness.host(); - const chipDimensions = await chip.getDimensions(); await chip.click(); fixture.detectChanges(); - expect(openAtPositionSpy).toHaveBeenCalledWith({ - x: chipDimensions.left + chipDimensions.width, - y: chipDimensions.top, - }); + expect(createNextToElementSpy).toHaveBeenCalledWith( + component.filterModalTemplate, + fixture.debugElement.query(By.css('mat-chip')).nativeElement, + component.viewContainerRef + ); expect(component.selectedFilterName).toBe('filter1'); }); diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_module.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_module.ts index dac151a753..247a6b1e39 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_module.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_module.ts @@ -43,7 +43,6 @@ import {FilterbarComponent} from './filterbar_component'; import {FilterbarContainer} from './filterbar_container'; import {RunsGroupMenuButtonComponent} from './runs_group_menu_button_component'; import {RunsGroupMenuButtonContainer} from './runs_group_menu_button_container'; -import {CustomModalModule} from '../../../widgets/custom_modal/custom_modal_module'; import {RunsDataTable} from './runs_data_table'; import {RunsTableContainer} from './runs_table_container'; @@ -68,7 +67,6 @@ import {RunsTableContainer} from './runs_table_container'; MatSortModule, MatTableModule, RangeInputModule, - CustomModalModule, AlertModule, ], exports: [RunsTableContainer], diff --git a/tensorboard/webapp/widgets/custom_modal/BUILD b/tensorboard/webapp/widgets/custom_modal/BUILD index d600bb74ef..7da319b9fa 100644 --- a/tensorboard/webapp/widgets/custom_modal/BUILD +++ b/tensorboard/webapp/widgets/custom_modal/BUILD @@ -5,11 +5,12 @@ package(default_visibility = ["//tensorboard:internal"]) tf_ng_module( name = "custom_modal", srcs = [ - "custom_modal_component.ts", - "custom_modal_module.ts", + "custom_modal.ts", ], deps = [ - "@npm//@angular/common", + "//tensorboard/webapp/angular:expect_angular_cdk_overlay", + "//tensorboard/webapp/angular:expect_angular_cdk_portal", + "//tensorboard/webapp/util:dom", "@npm//@angular/core", "@npm//rxjs", ], @@ -23,10 +24,12 @@ tf_ts_library( ], deps = [ ":custom_modal", + "//tensorboard/webapp/angular:expect_angular_cdk_overlay", "//tensorboard/webapp/angular:expect_angular_core_testing", "@npm//@angular/common", "@npm//@angular/core", "@npm//@angular/platform-browser", "@npm//@types/jasmine", + "@npm//rxjs", ], ) diff --git a/tensorboard/webapp/widgets/custom_modal/custom_modal.ts b/tensorboard/webapp/widgets/custom_modal/custom_modal.ts new file mode 100644 index 0000000000..90d3b689c7 --- /dev/null +++ b/tensorboard/webapp/widgets/custom_modal/custom_modal.ts @@ -0,0 +1,170 @@ +/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {Injectable, TemplateRef, ViewContainerRef} from '@angular/core'; +import {ConnectedPosition, Overlay, OverlayRef} from '@angular/cdk/overlay'; +import {TemplatePortal} from '@angular/cdk/portal'; +import {Subject, Subscription} from 'rxjs'; +import {isMouseEventInElement} from '../../util/dom'; + +/** + * Enables dynamic creation of modal components. + * + * # Usage + * Define a modal using an ng-template: + * ``` + * + * + * + * + * ``` + * + * Define a ViewChild to reference the template in the component file: + * ``` + * // my_component.ts + * ... + * @ViewChild('myModalTemplate', {read: TemplateRef}) + * myModalTemplate!: TemplateRef; + * ... + * ``` + * + * Inject CustomModal and ViewContainerRef into the component + * ``` + * // my_component.ts + * ... + * constructor( + * private readonly customModal: CustomModal, + * private readonly viewContainerRef: ViewContainerRef, + * ) {} + * ... + * ``` + * + * To create a modal, call createNextToElement(): + * ``` + * // my_component.ts + * ... + * onSomeButtonClick(mouseEvent: MouseEventj) { + * this.customModal.createNextToElement( + * this.myModalTemplate, + * mouseEvent.target as HTMLElement, + * this.viewContainerRef + * ); + * } + * ... + * ``` + */ + +export class CustomModalRef { + overlayRef: OverlayRef; + subscriptions: Subscription[] = []; + onClose = new Subject(); + + constructor(overlayRef: OverlayRef) { + this.overlayRef = overlayRef; + } +} + +@Injectable({providedIn: 'root'}) +export class CustomModal { + private customModalRefs: CustomModalRef[] = []; + + constructor(private readonly overlay: Overlay) {} + + /** Creates a modal from a template next to an element. */ + createNextToElement( + templateRef: TemplateRef, + element: Element, + viewContainerRef: ViewContainerRef, + connectedPosition: ConnectedPosition = { + originX: 'end', + originY: 'top', + overlayX: 'start', + overlayY: 'top', + } + ): CustomModalRef | undefined { + let positionStrategy = this.overlay.position().flexibleConnectedTo(element); + if (connectedPosition) { + positionStrategy = positionStrategy.withPositions([connectedPosition]); + } + + const overlayRef = this.overlay.create({ + positionStrategy, + hasBackdrop: false, + }); + overlayRef.attach(new TemplatePortal(templateRef, viewContainerRef)); + const customModalRef = new CustomModalRef(overlayRef); + this.customModalRefs.push(customModalRef); + + // setTimeout to prevent closing immediately after modal open. + setTimeout(() => { + const outsidePointerEventsSubscription = overlayRef + .outsidePointerEvents() + .subscribe((event) => { + // Only close when click is outside of every modal + if ( + this.customModalRefs.every( + (ref) => + !isMouseEventInElement(event, ref.overlayRef.overlayElement) + ) + ) { + this.closeAll(); + } + }); + customModalRef.subscriptions.push(outsidePointerEventsSubscription); + }); + + const keydownEventsSubscription = overlayRef + .keydownEvents() + .subscribe((event) => { + if (event.key === 'Escape') { + this.closeAll(); + } + }); + customModalRef.subscriptions.push(keydownEventsSubscription); + + return customModalRef; + } + + /** Destroys given custom modal and related resources. */ + close(customModalRef: CustomModalRef) { + const index = this.customModalRefs.findIndex( + (ref) => ref === customModalRef + ); + if (index === -1) { + console.warn('Could not find customModalRef', customModalRef); + return; + } + + customModalRef.subscriptions.forEach((subscription) => { + subscription.unsubscribe(); + }); + customModalRef.subscriptions = []; + customModalRef.overlayRef?.dispose(); + + this.customModalRefs.splice(index, 1); + + customModalRef.onClose.next(); + customModalRef.onClose.complete(); + } + + /** Destroys all created modals. */ + closeAll() { + while (this.customModalRefs.length) { + this.close(this.customModalRefs[0]); + } + } +} diff --git a/tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts b/tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts deleted file mode 100644 index a2e1a22539..0000000000 --- a/tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts +++ /dev/null @@ -1,133 +0,0 @@ -/* Copyright 2023 The TensorFlow Authors. All Rights Reserved. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -==============================================================================*/ -import { - Component, - EventEmitter, - Output, - ViewChild, - ElementRef, - HostListener, - OnInit, - ViewContainerRef, -} from '@angular/core'; -import {BehaviorSubject} from 'rxjs'; - -export interface ModalContent { - onRender?: () => void; -} - -@Component({ - selector: 'custom-modal', - template: ` -
- - - -
- `, - styles: [ - ` - :host { - position: fixed; - left: 0; - z-index: 9001; - } - - .content { - position: absolute; - } - `, - ], -}) -export class CustomModalComponent implements OnInit { - @Output() onOpen = new EventEmitter(); - @Output() onClose = new EventEmitter(); - - readonly visible$ = new BehaviorSubject(false); - private canClose = true; - - @ViewChild('content', {static: false}) - private readonly content!: ElementRef; - - private clickListener: () => void = this.maybeClose.bind(this); - - constructor(private readonly viewRef: ViewContainerRef) {} - - ngOnInit() { - this.visible$.subscribe((visible) => { - // Wait until after the next browser frame. - window.requestAnimationFrame(() => { - this.canClose = true; - if (visible) { - this.ensureContentIsWithinWindow(); - this.onOpen.emit(); - } else { - this.onClose.emit(); - } - }); - }); - } - - public openAtPosition(position: {x: number; y: number}) { - const root = this.viewRef.element.nativeElement; - // Set left/top to viewport (0,0) if the element has another "containing block" ancestor. - root.style.top = `${root.offsetTop - root.getBoundingClientRect().top}px`; - root.style.left = `${ - root.offsetLeft - root.getBoundingClientRect().left - }px`; - - this.content.nativeElement.style.left = position.x + 'px'; - this.content.nativeElement.style.top = position.y + 'px'; - this.canClose = false; - this.visible$.next(true); - document.addEventListener('click', this.clickListener); - } - - private ensureContentIsWithinWindow() { - if (!this.content) { - return; - } - - const boundingBox = this.content.nativeElement.getBoundingClientRect(); - if (boundingBox.left < 0) { - this.content.nativeElement.style.left = 0; - } - if (boundingBox.left + boundingBox.width > window.innerWidth) { - this.content.nativeElement.style.left = - window.innerWidth - boundingBox.width + 'px'; - } - - if (boundingBox.top < 0) { - this.content.nativeElement.style.top = 0; - } - if (boundingBox.top + boundingBox.height > window.innerHeight) { - this.content.nativeElement.style.top = - window.innerHeight - boundingBox.height + 'px'; - } - } - - @HostListener('document:keydown.escape', ['$event']) - private maybeClose() { - if (!this.canClose) { - return; - } - this.close(); - } - - public close() { - document.removeEventListener('click', this.clickListener); - this.visible$.next(false); - } -} diff --git a/tensorboard/webapp/widgets/custom_modal/custom_modal_module.ts b/tensorboard/webapp/widgets/custom_modal/custom_modal_module.ts deleted file mode 100644 index 85f509ed2e..0000000000 --- a/tensorboard/webapp/widgets/custom_modal/custom_modal_module.ts +++ /dev/null @@ -1,25 +0,0 @@ -/* Copyright 2023 The TensorFlow Authors. All Rights Reserved. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -==============================================================================*/ - -import {CommonModule} from '@angular/common'; -import {NgModule} from '@angular/core'; -import {CustomModalComponent} from './custom_modal_component'; - -@NgModule({ - declarations: [CustomModalComponent], - imports: [CommonModule], - exports: [CustomModalComponent], -}) -export class CustomModalModule {} diff --git a/tensorboard/webapp/widgets/custom_modal/custom_modal_test.ts b/tensorboard/webapp/widgets/custom_modal/custom_modal_test.ts index 406290ee99..3a945450fb 100644 --- a/tensorboard/webapp/widgets/custom_modal/custom_modal_test.ts +++ b/tensorboard/webapp/widgets/custom_modal/custom_modal_test.ts @@ -12,183 +12,309 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {ComponentFixture, TestBed} from '@angular/core/testing'; +import { + ComponentFixture, + TestBed, + fakeAsync, + tick, +} from '@angular/core/testing'; import {By} from '@angular/platform-browser'; -import {CustomModalComponent} from './custom_modal_component'; +import {CustomModal} from './custom_modal'; import {CommonModule} from '@angular/common'; import { Component, - ElementRef, - EventEmitter, - Output, + TemplateRef, ViewChild, + ViewContainerRef, } from '@angular/core'; - -function waitFrame() { - return new Promise((resolve) => window.requestAnimationFrame(resolve)); -} +import {Overlay, OverlayModule, OverlayRef} from '@angular/cdk/overlay'; +import {first} from 'rxjs'; @Component({ - selector: 'testable-modal', - template: ` -
My great content
-
`, -}) -class TestableComponent { - @ViewChild('modal', {static: false}) - modalComponent!: CustomModalComponent; - - @ViewChild('content', {static: false}) - content!: ElementRef; - - isOpen = false; - - @Output() onOpen = new EventEmitter(); - @Output() onClose = new EventEmitter(); - - setOpen() { - this.isOpen = true; - this.onOpen.emit(); - } + selector: 'fake-modal-view-container', + template: ` + + + +
abc123
+
+ +
xyz
+
+ `, + styles: [ + ` + :host { + display: block; + width: 400px; + height: 400px; + } - setClosed() { - this.isOpen = false; - this.onClose.emit(); - } - - close() { - this.modalComponent.close(); - } + .content, + .another-content { + // Make modals small to allow easily testing clicking outside of modals. + width: 10px; + height: 10px; + } + `, + ], +}) +class FakeViewContainerComponent { + @ViewChild('modalTemplate', {read: TemplateRef}) + readonly modalTemplateRef!: TemplateRef; - getContentStyle() { - return (this.modalComponent as any).content.nativeElement.style; - } + @ViewChild('anotherModalTemplate', {read: TemplateRef}) + readonly anotherModalTemplateRef!: TemplateRef; - public openAtPosition(position: {x: number; y: number}) { - this.modalComponent.openAtPosition(position); - } + constructor( + readonly customModal: CustomModal, + readonly vcRef: ViewContainerRef + ) {} } describe('custom modal', () => { + let viewContainerFixture: ComponentFixture; + beforeEach(async () => { await TestBed.configureTestingModule({ - declarations: [TestableComponent, CustomModalComponent], - imports: [CommonModule], + declarations: [FakeViewContainerComponent], + imports: [CommonModule, OverlayModule], }).compileComponents(); + + viewContainerFixture = TestBed.createComponent(FakeViewContainerComponent); + viewContainerFixture.detectChanges(); }); - it('waits a frame before emitting onOpen or onClose', async () => { - const fixture = TestBed.createComponent(TestableComponent); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: 0, y: 0}); - expect(fixture.componentInstance.isOpen).toBeFalse(); - await waitFrame(); - expect(fixture.componentInstance.isOpen).toBeTrue(); - fixture.componentInstance.close(); - fixture.detectChanges(); - await waitFrame(); - expect(fixture.componentInstance.isOpen).toBeFalse(); + it('creates a modal', () => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + const overlay = TestBed.inject(Overlay); + const createSpy = spyOn(overlay, 'create').and.callThrough(); + const attachSpy = spyOn(OverlayRef.prototype, 'attach').and.callThrough(); + + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerFixture.detectChanges(); + + const content = viewContainerFixture.debugElement.query(By.css('.content')); + expect(content.nativeElement.innerHTML).toContain('abc123'); + const createArg = createSpy.calls.mostRecent().args[0]!; + expect(createArg.positionStrategy).toEqual( + jasmine.objectContaining({ + positions: [ + { + originX: 'end', + originY: 'top', + overlayX: 'start', + overlayY: 'top', + }, + ], + }) + ); + const attachArgs = attachSpy.calls.mostRecent().args[0]; + expect(attachArgs.templateRef).toBe( + viewContainerComponent.modalTemplateRef + ); + expect(attachArgs.viewContainerRef).toBe(viewContainerComponent.vcRef); }); - describe('openAtPosition', () => { - it('applies top and left offsets', () => { - const fixture = TestBed.createComponent(TestableComponent); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: 20, y: 10}); - expect(fixture.componentInstance.getContentStyle().top).toEqual('10px'); - expect(fixture.componentInstance.getContentStyle().left).toEqual('20px'); - }); + describe('overlay event subscriptions', () => { + it('subscribes to click and pointer events on create', fakeAsync(() => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + + const customModalRef = + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + )!; + tick(); - it('emits onOpen', async () => { - const fixture = TestBed.createComponent(TestableComponent); - const spy = spyOn(fixture.componentInstance.onOpen, 'emit'); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: 20, y: 10}); - expect(spy).not.toHaveBeenCalled(); - await waitFrame(); - expect(spy).toHaveBeenCalled(); + expect(customModalRef.subscriptions.length).toEqual(2); + })); + + it('cleans up subscriptions on removal', fakeAsync(() => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + const customModalRef = + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + )!; + tick(); + + viewContainerComponent.customModal.close(customModalRef); + + expect(customModalRef.subscriptions.length).toEqual(0); + })); + }); + + describe('closeAll', () => { + it('clears all modals in the modal ViewContainerRef', () => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + const anotherModalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.another-modal-trigger-button') + ).nativeElement; + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.anotherModalTemplateRef, + anotherModalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerFixture.detectChanges(); + + TestBed.inject(CustomModal).closeAll(); + + const content = viewContainerFixture.debugElement.query( + By.css('.content') + ); + const anotherContent = viewContainerFixture.debugElement.query( + By.css('.another-content') + ); + expect(content).toBeNull(); + expect(anotherContent).toBeNull(); + expect(viewContainerComponent.vcRef.length).toBe(0); }); }); describe('closing behavior', () => { - let fixture: ComponentFixture; - beforeEach(async () => { - fixture = TestBed.createComponent(TestableComponent); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: 0, y: 0}); - await waitFrame(); - }); + it('emits onClose event on close', fakeAsync(() => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + const customModalRef = + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + )!; - it('closes when escape key is pressed', async () => { - expect(fixture.componentInstance.isOpen).toBeTrue(); - const event = new KeyboardEvent('keydown', {key: 'escape'}); - document.dispatchEvent(event); - await waitFrame(); + customModalRef.onClose.pipe(first()).subscribe((val) => { + // onClose should emit an empty value. + expect(val).toBeUndefined(); + }); - expect(fixture.componentInstance.isOpen).toBeFalse(); - }); + viewContainerComponent.customModal.close(customModalRef); + tick(); + })); - it('closes when user clicks outside modal', async () => { - expect(fixture.componentInstance.isOpen).toBeTrue(); - document.body.click(); - await waitFrame(); + it('closes when escape key is pressed', fakeAsync(() => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerFixture.detectChanges(); + tick(); + const content = viewContainerFixture.debugElement.query( + By.css('.content') + ); - expect(fixture.componentInstance.isOpen).toBeFalse(); - }); - }); + const event = new KeyboardEvent('keydown', {key: 'Escape'}); + document.body.dispatchEvent(event); + viewContainerFixture.detectChanges(); + tick(); - describe('ensures content is always within the window', () => { - beforeEach(() => { - window.innerHeight = 1000; - window.innerWidth = 1000; - }); + expect(viewContainerComponent.vcRef.length).toBe(0); + expect( + viewContainerFixture.debugElement.query(By.css('.content')) + ).toBeNull(); + })); - it('sets left to 0 if less than 0', async () => { - const fixture = TestBed.createComponent(TestableComponent); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: -10, y: 0}); - expect(fixture.componentInstance.isOpen).toBeFalse(); - await waitFrame(); - fixture.detectChanges(); + it('closes all modals when user clicks an area outside all modals', fakeAsync(() => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + const anotherModalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.another-modal-trigger-button') + ).nativeElement; + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.anotherModalTemplateRef, + anotherModalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerFixture.detectChanges(); + tick(); - const content = fixture.debugElement.query(By.css('.content')); - expect(content.nativeElement.style.left).toEqual('0px'); - }); + const event = new MouseEvent('click', {clientX: 300, clientY: 300}); + viewContainerFixture.nativeElement.dispatchEvent(event); + viewContainerFixture.detectChanges(); - it('sets top to 0 if less than 0', async () => { - const fixture = TestBed.createComponent(TestableComponent); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: 0, y: -10}); - expect(fixture.componentInstance.isOpen).toBeFalse(); - await waitFrame(); - fixture.detectChanges(); + const content = viewContainerFixture.debugElement.query( + By.css('.content') + ); + const anotherContent = viewContainerFixture.debugElement.query( + By.css('.another-content') + ); + expect(content).toBeNull(); + expect(anotherContent).toBeNull(); + expect(viewContainerComponent.vcRef.length).toBe(0); + })); - const content = fixture.debugElement.query(By.css('.content')); - expect(content.nativeElement.style.top).toEqual('0px'); - }); + it('does not close when a click is inside at least one modal', async () => { + const viewContainerComponent = viewContainerFixture.componentInstance; + const modalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.modal-trigger-button') + ).nativeElement; + const anotherModalTriggerButton = viewContainerFixture.debugElement.query( + By.css('.another-modal-trigger-button') + ).nativeElement; + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.modalTemplateRef, + modalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerComponent.customModal.createNextToElement( + viewContainerComponent.anotherModalTemplateRef, + anotherModalTriggerButton, + viewContainerComponent.vcRef + ); + viewContainerFixture.detectChanges(); + const content = viewContainerFixture.debugElement.query( + By.css('.content') + ); + const anotherContent = viewContainerFixture.debugElement.query( + By.css('.another-content') + ); - it('sets left to maximum if content overflows the window', async () => { - const fixture = TestBed.createComponent(TestableComponent); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: 1010, y: 0}); - expect(fixture.componentInstance.isOpen).toBeFalse(); - await waitFrame(); - fixture.detectChanges(); - const content = fixture.debugElement.query(By.css('.content')); - // While rendering in a test the elements width and height will appear to be 0. - expect(content.nativeElement.style.left).toEqual('1000px'); - }); + // Event is in first modal. + const event = new MouseEvent('click', {clientX: 101, clientY: 101}); + content.nativeElement.dispatchEvent(event); + viewContainerFixture.detectChanges(); - it('sets top to maximum if content overflows the window', async () => { - const fixture = TestBed.createComponent(TestableComponent); - fixture.detectChanges(); - fixture.componentInstance.openAtPosition({x: 0, y: 1010}); - expect(fixture.componentInstance.isOpen).toBeFalse(); - await waitFrame(); - fixture.detectChanges(); - const content = fixture.debugElement.query(By.css('.content')); - // While rendering in a test the elements width and height will appear to be 0. - expect(content.nativeElement.style.top).toEqual('1000px'); + expect(content.nativeElement.innerHTML).toContain('abc123'); + expect(anotherContent.nativeElement.innerHTML).toContain('xyz'); }); }); }); diff --git a/tensorboard/webapp/widgets/data_table/column_selector_component.ts b/tensorboard/webapp/widgets/data_table/column_selector_component.ts index 1b55a25e63..8ff421384b 100644 --- a/tensorboard/webapp/widgets/data_table/column_selector_component.ts +++ b/tensorboard/webapp/widgets/data_table/column_selector_component.ts @@ -93,12 +93,12 @@ export class ColumnSelectorComponent implements OnInit, AfterViewInit { ngAfterViewInit() { this.searchInput = ''; - this.searchField.nativeElement.focus(); this.selectedIndex$.next(0); - } - - focus() { - this.searchField?.nativeElement.focus(); + this.activate(); + // Wait until next tick to prevent https://angular.io/errors/NG0100 + setTimeout(() => { + this.searchField?.nativeElement.focus(); + }); } getFilteredColumns() { diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index 7c4a1145bd..656572537e 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -12,7 +12,7 @@ limitations under the License. --> - + - + - + - + - + - +
diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index bd3f708587..53c2b967d2 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -23,11 +23,12 @@ import { OnDestroy, Output, QueryList, + TemplateRef, ViewChild, + ViewContainerRef, } from '@angular/core'; import { ColumnHeader, - ColumnHeaderType, DiscreteFilter, DiscreteFilterValue, FilterAddedEvent, @@ -39,12 +40,11 @@ import { AddColumnEvent, } from './types'; import {HeaderCellComponent} from './header_cell_component'; -import {Subscription} from 'rxjs'; -import {CustomModalComponent} from '../custom_modal/custom_modal_component'; -import {ColumnSelectorComponent} from './column_selector_component'; +import {Subscription, first} from 'rxjs'; import {ContentCellComponent} from './content_cell_component'; import {RangeValues} from '../range_input/types'; import {dataTableUtils} from './utils'; +import {CustomModal, CustomModalRef} from '../custom_modal/custom_modal'; const preventDefault = function (e: MouseEvent) { e.preventDefault(); @@ -85,25 +85,27 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { @Output() addFilter = new EventEmitter(); @Output() loadAllColumns = new EventEmitter(); - @ViewChild('columnSelectorModal', {static: false}) - private readonly columnSelectorModal!: CustomModalComponent; + @ViewChild('contextMenuTemplate', {read: TemplateRef}) + contextMenuTemplate!: TemplateRef; + @ViewChild('filterModalTemplate', {read: TemplateRef}) + filterModalTemplate!: TemplateRef; + @ViewChild('columnSelectorModalTemplate', {read: TemplateRef}) + columnSelectorModalTemplate!: TemplateRef; - @ViewChild(ColumnSelectorComponent, {static: false}) - private readonly columnSelector!: ColumnSelectorComponent; + filterModalRef?: CustomModalRef | undefined; + columnSelectorModalRef?: CustomModalRef | undefined; - @ViewChild('contextMenu', {static: false}) - private readonly contextMenu!: CustomModalComponent; - - @ViewChild('filterModal', {static: false}) - private readonly filterModal!: CustomModalComponent; + draggingHeaderName: string | undefined; + highlightedColumnName: string | undefined; + highlightSide: Side = Side.RIGHT; - readonly ColumnHeaders = ColumnHeaderType; readonly SortingOrder = SortingOrder; readonly Side = Side; - draggingHeaderName: string | undefined; - highlightedColumnName: string | undefined; - highlightSide: Side = Side.RIGHT; + constructor( + private readonly customModal: CustomModal, + private readonly viewContainerRef: ViewContainerRef + ) {} ngOnDestroy() { document.removeEventListener('dragover', preventDefault); @@ -263,50 +265,38 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { }); } - focusColumnSelector() { - this.columnSelector.focus(); - } - openContextMenu(header: ColumnHeader, event: MouseEvent) { event.stopPropagation(); event.preventDefault(); - this.columnSelectorModal?.close(); - this.filterModal?.close(); this.contextMenuHeader = header; - this.contextMenu.openAtPosition({ - x: event.clientX, - y: event.clientY, - }); + // For right clicks, open context menu near button rather than all the way outside of the + // header cell, which looks weird. + const descendantButton = (event.target as HTMLElement).querySelector( + 'button.context-menu-container' + ); + const targetElement = descendantButton ?? (event.target as HTMLElement); + + this.customModal.createNextToElement( + this.contextMenuTemplate, + targetElement, + this.viewContainerRef + ); } - onContextMenuClosed() { - this.contextMenuHeader = undefined; - } + openColumnSelector({event, insertTo}: {event: MouseEvent; insertTo?: Side}) { + event.stopPropagation(); + this.closeSubmenus(); - openColumnSelector({ - event, - insertTo, - isSubMenu, - }: { - event: MouseEvent; - insertTo?: Side; - isSubMenu?: boolean; - }) { - if (isSubMenu) { - event.stopPropagation(); - this.filterModal?.close(); - this.columnSelectorModal?.close(); - } this.insertColumnTo = insertTo; - const rect = ( - (event.target as HTMLElement).closest('button') as HTMLButtonElement - ).getBoundingClientRect(); - this.columnSelectorModal.openAtPosition({ - x: rect.x + rect.width, - y: rect.y, + this.columnSelectorModalRef = this.customModal.createNextToElement( + this.columnSelectorModalTemplate, + (event.target as HTMLElement).closest('button') as HTMLButtonElement, + this.viewContainerRef + ); + this.columnSelectorModalRef?.onClose.pipe(first()).subscribe(() => { + this.columnSelectorModalRef = undefined; }); - this.columnSelector.activate(); } canContextMenuRemoveColumn() { @@ -315,8 +305,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { onRemoveColumn(header: ColumnHeader) { this.removeColumn.emit(header); - this.contextMenu.close(); - this.filterModal?.close(); + this.customModal.closeAll(); } onColumnAdded(header: ColumnHeader) { @@ -327,16 +316,27 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { }); } + closeSubmenus() { + if (this.filterModalRef) { + this.customModal.close(this.filterModalRef); + } + if (this.columnSelectorModalRef) { + this.customModal.close(this.columnSelectorModalRef); + } + } + openFilterMenu(event: MouseEvent) { - this.filterColumn = this.contextMenuHeader; - const rect = ( - (event.target as HTMLElement).closest('button') as HTMLButtonElement - ).getBoundingClientRect(); event.stopPropagation(); - this.columnSelectorModal?.close(); - this.filterModal.openAtPosition({ - x: rect.x + rect.width, - y: rect.y, + this.closeSubmenus(); + + this.filterColumn = this.contextMenuHeader; + this.filterModalRef = this.customModal.createNextToElement( + this.filterModalTemplate, + (event.target as HTMLElement).closest('button') as HTMLButtonElement, + this.viewContainerRef + ); + this.filterModalRef?.onClose.pipe(first()).subscribe(() => { + this.filterModalRef = undefined; }); } @@ -347,10 +347,6 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { return this.columnFilters.get(this.filterColumn.name); } - onFilterClosed() { - this.filterColumn = undefined; - } - intervalFilterChanged(value: RangeValues) { if (!this.filterColumn) { return; diff --git a/tensorboard/webapp/widgets/data_table/data_table_module.ts b/tensorboard/webapp/widgets/data_table/data_table_module.ts index b432d0a75d..cb89395168 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_module.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_module.ts @@ -24,7 +24,6 @@ import {DataTableHeaderModule} from './data_table_header_module'; import {ContentCellComponent} from './content_cell_component'; import {ContentRowComponent} from './content_row_component'; import {ColumnSelectorModule} from './column_selector_module'; -import {CustomModalModule} from '../custom_modal/custom_modal_module'; import {FilterDialogModule} from './filter_dialog_module'; import {ContextMenuModule} from './context_menu_module'; @@ -47,7 +46,6 @@ import {ContextMenuModule} from './context_menu_module'; MatButtonModule, MatProgressSpinnerModule, DataTableHeaderModule, - CustomModalModule, ColumnSelectorModule, FilterDialogModule, ContextMenuModule, diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts index 21979f28ed..eb918b4774 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_test.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts @@ -13,7 +13,14 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {Component, EventEmitter, Input, Output, ViewChild} from '@angular/core'; +import { + ApplicationRef, + Component, + EventEmitter, + Input, + Output, + ViewChild, +} from '@angular/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {MatIconTestingModule} from '../../testing/mat_icon_module'; import {By} from '@angular/platform-browser'; @@ -35,8 +42,8 @@ import {NoopAnimationsModule} from '@angular/platform-browser/animations'; import {ColumnSelectorComponent} from './column_selector_component'; import {ContentCellComponent} from './content_cell_component'; import {ColumnSelectorModule} from './column_selector_module'; -import {CustomModalModule} from '../custom_modal/custom_modal_module'; import {FilterDialog} from './filter_dialog_component'; +import {CustomModal} from '../custom_modal/custom_modal'; @Component({ selector: 'testable-comp', @@ -58,7 +65,6 @@ import {FilterDialog} from './filter_dialog_component'; @@ -97,6 +103,8 @@ class TestableComponent { index?: number; }>(); @Output() removeColumn = new EventEmitter(); + + constructor(readonly customModal: CustomModal) {} } describe('data table', () => { @@ -113,7 +121,6 @@ describe('data table', () => { MatIconTestingModule, DataTableModule, ColumnSelectorModule, - CustomModalModule, NoopAnimationsModule, ], }).compileComponents(); @@ -132,7 +139,6 @@ describe('data table', () => { function createComponent(input: { headers?: ColumnHeader[]; sortingInfo?: SortingInfo; - hparamsEnabled?: boolean; data?: TableData[]; potentialColumns?: ColumnHeader[]; columnFilters?: Map; @@ -167,6 +173,9 @@ describe('data table', () => { fixture.componentInstance.orderColumns = orderColumnsSpy; fixture.detectChanges(); + // Add component to app ref for modal testing. + const appRef = TestBed.inject(ApplicationRef); + appRef.components.push(fixture.componentRef); return fixture; } @@ -924,12 +933,18 @@ describe('data table', () => { }); function openFilterDialog(fixture: ComponentFixture) { - fixture.debugElement - .query(By.directive(DataTableComponent)) - .componentInstance.openContextMenu( - mockHeaders[0], - new MouseEvent('contextmenu') - ); + const dataTableDebugEl = fixture.debugElement.query( + By.directive(DataTableComponent) + ); + const mouseEvent = new MouseEvent('contextmenu'); + const headerEl = fixture.debugElement.query( + By.css('.context-menu-container') + ).nativeElement; + spyOnProperty(mouseEvent, 'target').and.returnValue(headerEl); + dataTableDebugEl.componentInstance.openContextMenu( + mockHeaders[0], + mouseEvent + ); fixture.detectChanges(); const filterBtn = fixture.debugElement .queryAll(By.css('.context-menu button'))