Changes custom modal to be created dynamically #6799

hoonji · 2024-03-19T14:31:30Z

Motivation for features / changes

The current Custom Modal can't display modals outside of their ancestors' stacking context (e.g. in scalar card tables: #6737 (comment) ), which significantly hinders scalar table context menu functionality.

It also has some minor tricky issues that are difficult to fix like some modals lingering when another one is opened:

Technical description of changes

Before: Custom modals were defined in a consumer component's template. The modals were embedded views of the consumer component (e.g. DataTableComponent), which prevented them from being displayed outside the table's stacking context, e.g. outside of a scalar card table
After: Custom modals are still defined in a consumer component's template, but wrapped in ng-templates. They are dynamically created using a newly defined CustomModal service. When the appropriate CustomModal service method is a called, the modal template is attached as an embedded view in the app's root component (using CDK Overlay), freeing it from all stacking context issues.

CustomModalComponent is completely subsumed by Overlay and is thus deleted. Only the CustomModal service will be required going forward.

Detailed steps to verify changes work correctly (as executed by you)

Manually tested all custom modal features in runs table, filter bar, scalar card table

(Need to set query param enableScalarColumnContextMenus=true to test scalar table context menu behavior)

bmd3k · 2024-03-19T20:15:17Z

tensorboard/webapp/widgets/custom_modal/custom_modal.ts

+ * ```
+ */
+@Injectable({providedIn: 'root'})
+export class CustomModal {


Looks good. The problems I was having with the column selector in data table seem to have been solved!

I was wondering, though, could we have instead used Angular Material Overlay? https://material.angular.io/cdk/overlay/overview
Is CustomModal trying to solve the same problem as Overlay?

Oh wow, didn't know about this! Yes I'd say the Overlay problem scope is basically equivalent to CustomModal's.

I immediately tried this approach out, as it's clearly more preferable to use a mature, featureful library if possible. Unfortunately, the dynamic nature of our clickable affordances doesn't seem to be compatible with how Overlay works.

To elaborate: Overlay usage (ex) seems to require referring to the clickable affordance by template variable (e.g. #trigger), but there is no straightforward way we can implement this for our *ngFor based column headers. Also, it seems that dynamically setting template variables won't be possible for the foreseeable future: angular/angular#33233

Overlay also contains a programmatic interface. Does that make it usable?

Here I migrated the filterbar_component to use overlay:
bmd3k@cc7ebdd

I recognize that filterbar is the easiest of the usages of CustomModal but hopefully this points you in a useful direction. I admit that Overlay might still not be the right fit but I think it's worth exploring further.

Some other things to consider:

custom_modal.ts could still exist as a service that wraps Overlay with some stuff specific to context menus. Maybe it's actually custom_menu.ts or context_menu.ts?

once you've created an overlay for the top level of a menu, is it possible to just reuse that overlay for submenus rather than creating a new overlay for each descendent in the hierarchy?

The programmatic approach is nifty, thanks for the example! I like the idea of using the CustomModal service to encapsulate common functionality like attaching the TemplatePortal and adding the backdropClick handler. I'll experiment with this method and re-request review when it's ready.

The code has been updated to use Overlay! Note that Overlay features make the CustomModalComponent redundant; we can solely use the CustomModal service now.

Some advantages of Overlay include:

Modals can now follow trigger elements when the page is resized

Less boilerplate (no extra <custom-modal> required in templates; no extra change detection management needed)

Easy opportunities for future customization (e.g. configure modal behavior on scroll)

I've added some convenience logic in CustomModal to make Overlays work well with our nested modals requirements, along with some new tests. PTAL! +cc @rileyajones

P.S.

is it possible to just reuse that overlay for submenus rather than creating a new overlay for each descendent in the hierarchy?

I've found that each overlay must specify its own separate position, which means that each uniquely positioned modal needs its own overlay.

Excellent. Thanks for bearing with me. I'm glad it actually went somewhere.

bmd3k · 2024-03-19T20:20:02Z

tensorboard/webapp/runs/views/runs_table/filterbar_component.ts

  openFilterMenu(event: MouseEvent, filterName: string) {
    this.selectedFilterName = filterName;
    const rect = (
      (event.target as HTMLElement).closest('mat-chip') as HTMLElement
    ).getBoundingClientRect();
-    this.filterModal.openAtPosition({
+    this.customModal.createAtPosition(this.filterModalTemplate, {


One relatively minor bug I noticed:

Click on filter chip. Filter opens. (Good.)

Click on filter chip again. Filter closes. (Probably Good.)

Click on filter chip again. Nothing happens. (Probably not Good?)

Oh great catch. There was some residual logic pertaining to the old implementation that was hiding the modal on the second click (*ngIf="selectedFilter"). Removing this makes everything work as expected.

bmd3k

Sorry, I unfortunately didn't have time to review the entire thing. I will hopefully take another pass on Monday. What I did see looks pretty good, though.

bmd3k · 2024-03-22T18:22:22Z

tensorboard/webapp/widgets/custom_modal/custom_modal.ts

+      (ref) => ref === customModalRef
+    );
+    if (index === -1) {
+      console.warn('Could not find customModalRef', customModalRef);


I got this message many times. I think mainly from choosing the "Filter" submenu from the context menu?

This was from a dangling customModalRef reference in DataTableComponent: if CustomModal.closeAll() is called, the modals refs are cleaned up in CustomModal, but not in DataTableComponent, and the next DataTableComponent.closeSubmenus() will request closing a nonexistent modal.

I added an onClose Subject to CustomModalRef that callers can subscribe to and run cleanup code with, like this.filterModalRef = undefined;. (On the other hand, I think onOpen is redundant as open is done synchronously and doesn't require a callback)

bmd3k · 2024-03-22T18:23:46Z

tensorboard/webapp/widgets/data_table/column_selector_component.ts

-  focus() {
-    this.searchField?.nativeElement.focus();
+    this.activate();
+    // Wait until next tick to prevent https://angular.io/errors/NG0100


I still got these errors, though, when adding new columns using the selector.

Resolved by passing the calling component's ViewContainerRef to createNextToElement().

It was caused by the template values in the app root ViewContainer being updated after the DataTableComponent template was checked. Using the calling Component's own ViewContainer ensures proper change detection order without any manual intervention.

bmd3k · 2024-03-22T18:43:06Z

tensorboard/webapp/widgets/custom_modal/custom_modal.ts

+    }
+
+    const appInstance = appComponents[0].instance;
+    let viewContainerRef: ViewContainerRef = appInstance.vcRef;


I guess this just happens to work because ngx-color-picker already forced us to do this:

tensorboard/tensorboard/webapp/app_container.ts

Line 26 in 71952d6

constructor(readonly vcRef: ViewContainerRef) {}

Can you do one of the following?

Add documentation to app_container.ts saying that we're also doing this for customModal.
or

Personally I think it's reasonable to just ask the other components to pass a ViewContainerRef to CustomModal.createNextToElement() (like how we pass it to TemplatePortal in cc7ebdd).

Actually, since CustomModal is an injectable service, can you just inject ViewContainerRef in its constructor? Or is that something that is only available to Components?

can you just inject ViewContainerRef in its constructor

I actually tried this earlier :) Sadly it's indeed undefined in Services.

I think it's reasonable to just ask the other components to pass a ViewContainerRef

I've decided that this is the way to go. For just a little bit extra complexity in the function call, we can eliminate the app root logic, some test boilerplate, and the annoying NG0100 error that was otherwise unavoidable without hacky workarounds.

bmd3k · 2024-03-22T18:45:16Z

tensorboard/webapp/widgets/custom_modal/custom_modal.ts

+  createNextToElement(
+    templateRef: TemplateRef<unknown>,
+    element: Element,
+    connectedPosition: ConnectedPosition = {


I don't see where you ever override connectedPosition. I think you're always relying on the default? Could you remove this as an argument to the function and instead make it a class or file-level constant?

It's not being used at the moment, but it seems very useful to allow future callers to configure the modal position rather than always create it at the bottom right. It also suits the theme of the function: we can create a modal next to an element, but "next to" how?

I don't have a strong opinion on this though - I can clean it up if you think making this constant is cleaner for now.

bmd3k · 2024-03-22T18:46:26Z

tensorboard/webapp/widgets/custom_modal/custom_modal.ts

+ * ```
+ */
+@Injectable({providedIn: 'root'})
+export class CustomModal {


Excellent. Thanks for bearing with me. I'm glad it actually went somewhere.

bmd3k · 2024-03-22T18:49:30Z

tensorboard/webapp/widgets/custom_modal/custom_modal.ts

+import {Subscription} from 'rxjs';
+import {isMouseEventInElement} from '../../util/dom';
+
+/**


bmd3k · 2024-03-22T18:51:06Z

tensorboard/webapp/widgets/custom_modal/custom_modal.ts

+  overlayRef: OverlayRef;
+  subscriptions: Subscription[];
+
+  constructor(overlayRef: OverlayRef, subscriptions: Subscription[] = []) {


As far as I can tell you never override subscriptions with something else. Can you just define its initial [] value on L103?

Oh right, I'm appending the subscriptions one by one after instantiation. Changed as described and added simple tests to check subscribe/unsubscribe logic

bmd3k

Nice work. Thanks! Sorry this took so long.

bmd3k · 2024-03-25T18:51:59Z

tensorboard/webapp/runs/views/runs_table/filterbar_test.ts

+class TestableComponent {
+  constructor(
+    readonly customModal: CustomModal,
+    readonly vcRef: ViewContainerRef


Is vcRef still needed?

Good point, the inner component already has its own ViewContainerRef. Removed (same for below)

bmd3k · 2024-03-25T18:52:31Z

tensorboard/webapp/widgets/data_table/data_table_test.ts

+
+  constructor(
+    readonly customModal: CustomModal,
+    readonly vcRef: ViewContainerRef


Is vcRef still needed?

hoonji · 2024-03-26T04:35:51Z

Nice work. Thanks! Sorry this took so long.

No problem it was a great exercise in using the CDK. Thanks for all the pointers!

Ensure the following properties are passed to the column selector from the data table: ``` [numColumnsLoaded]="numColumnsLoaded" [hasMoreColumnsToLoad]="hasMoreColumnsToLoad" (loadAllColumns)="loadAllColumns.emit()" ``` These were accidentally removed in #6799. Caught by internal tests.

## Motivation for features / changes The current Custom Modal can't display modals outside of their ancestors' stacking context (e.g. in scalar card tables: tensorflow#6737 (comment) ), which significantly hinders scalar table context menu functionality. It also has some minor tricky issues that are difficult to fix like some modals lingering when another one is opened: ![image](https://github.com/tensorflow/tensorboard/assets/736199/934b1d0a-5650-47e2-94f4-e8836c1a6ab4) ## Technical description of changes - Before: Custom modals were defined in a consumer component's template. The modals were embedded views of the consumer component (e.g. DataTableComponent), which prevented them from being displayed outside the table's stacking context, e.g. outside of a scalar card table - After: Custom modals are still defined in a consumer component's template, but wrapped in ng-templates. They are dynamically created using a newly defined CustomModal service. When the appropriate CustomModal service method is a called, the modal template is attached as an embedded view using CDK Overlay, which places it in a top-level overlay container, freeing it from all stacking context issues. CustomModalComponent is completely subsumed by Overlay and is thus deleted. Only the CustomModal service will be required going forward. ## Detailed steps to verify changes work correctly (as executed by you) Manually tested all custom modal features in runs table, filter bar, scalar card table (Need to set query param `enableScalarColumnContextMenus=true` to test scalar table context menu behavior)

Ensure the following properties are passed to the column selector from the data table: ``` [numColumnsLoaded]="numColumnsLoaded" [hasMoreColumnsToLoad]="hasMoreColumnsToLoad" (loadAllColumns)="loadAllColumns.emit()" ``` These were accidentally removed in tensorflow#6799. Caught by internal tests.

hoonji added 6 commits March 19, 2024 16:33

Adds dynamic custom modal creation

1badf93

Fixes tests

8798b76

Formats files

2b50af4

Adds license

9c9815e

Edits CustomModal docstring

88f67a8

Adds docstrings

677ec4b

hoonji requested a review from bmd3k March 19, 2024 14:58

bmd3k requested a review from rileyajones March 19, 2024 20:03

bmd3k reviewed Mar 19, 2024

View reviewed changes

hoonji added 4 commits March 20, 2024 17:16

Fixes modal open on multiple chip click

9ba6a39

Updates custom modal to use CDK Overlay

f351e2b

Merge branch 'master' into revamp_context_menu

55fdeda

Fixes lint

76ca561

hoonji requested a review from bmd3k March 22, 2024 16:39

bmd3k reviewed Mar 22, 2024

View reviewed changes

Adds ViewContainerRef param to createNextToElement

b735b7e

hoonji requested a review from bmd3k March 25, 2024 13:14

bmd3k approved these changes Mar 25, 2024

View reviewed changes

Removes unnecessary ViewContainerRefs from tests

79df70b

hoonji merged commit c01e0bc into tensorflow:master Mar 26, 2024
13 checks passed

bmd3k mentioned this pull request Mar 26, 2024

Hparams: Fix regression in column selector. #6811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes custom modal to be created dynamically #6799

Changes custom modal to be created dynamically #6799

hoonji commented Mar 19, 2024 •

edited

bmd3k Mar 19, 2024

hoonji Mar 20, 2024

bmd3k Mar 20, 2024

hoonji Mar 21, 2024

hoonji Mar 22, 2024

bmd3k Mar 22, 2024

bmd3k Mar 19, 2024

hoonji Mar 20, 2024

bmd3k left a comment

bmd3k Mar 22, 2024

hoonji Mar 25, 2024 •

edited

bmd3k Mar 22, 2024

hoonji Mar 25, 2024 •

edited

bmd3k Mar 22, 2024

bmd3k Mar 22, 2024

hoonji Mar 25, 2024 •

edited

bmd3k Mar 22, 2024

hoonji Mar 25, 2024

bmd3k Mar 22, 2024

bmd3k Mar 22, 2024

bmd3k Mar 22, 2024

hoonji Mar 25, 2024

bmd3k left a comment

bmd3k Mar 25, 2024

hoonji Mar 26, 2024

bmd3k Mar 25, 2024

hoonji commented Mar 26, 2024

Changes custom modal to be created dynamically #6799

Changes custom modal to be created dynamically #6799

Conversation

hoonji commented Mar 19, 2024 • edited

Motivation for features / changes

Technical description of changes

Detailed steps to verify changes work correctly (as executed by you)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoonji Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoonji Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoonji Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoonji commented Mar 26, 2024

hoonji commented Mar 19, 2024 •

edited

hoonji Mar 25, 2024 •

edited

hoonji Mar 25, 2024 •

edited

hoonji Mar 25, 2024 •

edited