timeseries: improve card observer visibility logic with element ids

tensorboard/webapp/metrics/actions/BUILD

@@ -12,6 +12,7 @@ tf_ts_library(
     deps = [
         "//tensorboard/webapp/metrics:internal_types",
         "//tensorboard/webapp/metrics/data_source",
+        "//tensorboard/webapp/util:dom",
         "@npm//@ngrx/store",
     ],
 )

tensorboard/webapp/metrics/actions/index.ts

@@ -14,6 +14,7 @@ limitations under the License.
 ==============================================================================*/
 import {createAction, props} from '@ngrx/store';
 
+import {ElementId} from '../../util/dom';
 import {
   TagMetadata,
   TimeSeriesRequest,
@@ -113,12 +114,15 @@ export const fetchTimeSeriesLoaded = createAction(
 );
 
 /**
- * An event when some cards enter or exit the viewport. The card sets must be
- * mutually exclusive.
+ * An event when some cards enter or exit the viewport. An element within
+ * `enteredCards` must not be found within `exitedCards`, and vice versa.
  */
 export const cardVisibilityChanged = createAction(
   '[Metrics] Card Visibility Changed',
-  props<{enteredCards: Set<CardId>; exitedCards: Set<CardId>}>()
+  props<{
+    enteredCards: Array<{elementId: ElementId; cardId: CardId}>;
+    exitedCards: Array<{elementId: ElementId; cardId: CardId}>;
+  }>()
 );
 
 export const cardStepSliderChanged = createAction(

tensorboard/webapp/metrics/effects/BUILD

@@ -46,6 +46,7 @@ tf_ts_library(
         "//tensorboard/webapp/metrics/data_source",
         "//tensorboard/webapp/metrics/store",
         "//tensorboard/webapp/types",
+        "//tensorboard/webapp/util:dom",
         "//tensorboard/webapp/webapp_data_source:http_client_testing",
         "@npm//@ngrx/effects",
         "@npm//@ngrx/store",

tensorboard/webapp/metrics/effects/metrics_effects_test.ts

@@ -26,6 +26,7 @@ import {of, Subject} from 'rxjs';
 import {buildNavigatedAction} from '../../app_routing/testing';
 import {State} from '../../app_state';
 import * as selectors from '../../selectors';
+import {nextElementId} from '../../util/dom';
 import * as actions from '../actions';
 import {
   METRICS_PLUGIN_ID,
@@ -450,10 +451,7 @@ describe('metrics effects', () => {
         store.refreshState();
 
         actions$.next(
-          actions.cardVisibilityChanged({
-            enteredCards: new Set(),
-            exitedCards: new Set(),
-          })
+          actions.cardVisibilityChanged({enteredCards: [], exitedCards: []})
         );
 
         expect(fetchTimeSeriesSpy).not.toHaveBeenCalled();
@@ -474,15 +472,16 @@ describe('metrics effects', () => {
           loadState: DataLoadState.NOT_LOADED,
         });
 
+        const card1ElementId = nextElementId();
         store.overrideSelector(
           selectors.getVisibleCardIdSet,
           new Set<string>([])
         );
         store.refreshState();
         actions$.next(
           actions.cardVisibilityChanged({
-            enteredCards: new Set(),
-            exitedCards: new Set(['card1']),
+            enteredCards: [],
+            exitedCards: [{elementId: card1ElementId, cardId: 'card1'}],
           })
         );
 
@@ -496,8 +495,8 @@ describe('metrics effects', () => {
         store.refreshState();
         actions$.next(
           actions.cardVisibilityChanged({
-            enteredCards: new Set(['card1']),
-            exitedCards: new Set(),
+            enteredCards: [{elementId: card1ElementId, cardId: 'card1'}],
+            exitedCards: [],
           })
         );
 
@@ -530,15 +529,16 @@ describe('metrics effects', () => {
         });
 
         // Initial load.
+        const card1ElementId = nextElementId();
         store.overrideSelector(
           selectors.getVisibleCardIdSet,
           new Set(['card1'])
         );
         store.refreshState();
         actions$.next(
           actions.cardVisibilityChanged({
-            enteredCards: new Set(['card1']),
-            exitedCards: new Set(),
+            enteredCards: [{elementId: card1ElementId, cardId: 'card1'}],
+            exitedCards: [],
           })
         );
 
@@ -550,8 +550,8 @@ describe('metrics effects', () => {
         store.refreshState();
         actions$.next(
           actions.cardVisibilityChanged({
-            enteredCards: new Set(),
-            exitedCards: new Set(['card1']),
+            enteredCards: [],
+            exitedCards: [{elementId: card1ElementId, cardId: 'card1'}],
           })
         );
 
@@ -566,8 +566,8 @@ describe('metrics effects', () => {
         store.refreshState();
         actions$.next(
           actions.cardVisibilityChanged({
-            enteredCards: new Set(['card1']),
-            exitedCards: new Set(),
+            enteredCards: [{elementId: card1ElementId, cardId: 'card1'}],
+            exitedCards: [],
           })
         );
 
@@ -628,8 +628,11 @@ describe('metrics effects', () => {
         store.refreshState();
         actions$.next(
           actions.cardVisibilityChanged({
-            enteredCards: new Set(['card1', 'card2']),
-            exitedCards: new Set(),
+            enteredCards: [
+              {elementId: nextElementId(), cardId: 'card1'},
+              {elementId: nextElementId(), cardId: 'card2'},
+            ],
+            exitedCards: [],
           })
         );
 
@@ -673,8 +676,8 @@ describe('metrics effects', () => {
           store.refreshState();
           actions$.next(
             actions.cardVisibilityChanged({
-              enteredCards: new Set(['card1']),
-              exitedCards: new Set(),
+              enteredCards: [{elementId: nextElementId(), cardId: 'card1'}],
+              exitedCards: [],
             })
           );
 

tensorboard/webapp/metrics/store/BUILD

@@ -22,6 +22,7 @@ tf_ts_library(
         "//tensorboard/webapp/metrics/data_source",
         "//tensorboard/webapp/persistent_settings",
         "//tensorboard/webapp/types",
+        "//tensorboard/webapp/util:dom",
         "//tensorboard/webapp/util:lang",
         "//tensorboard/webapp/util:ngrx",
         "//tensorboard/webapp/util:types",
@@ -66,6 +67,7 @@ tf_ts_library(
         "//tensorboard/webapp/metrics:internal_types",
         "//tensorboard/webapp/metrics/data_source",
         "//tensorboard/webapp/types",
+        "//tensorboard/webapp/util:dom",
     ],
 )
 
@@ -91,6 +93,7 @@ tf_ts_library(
         "//tensorboard/webapp/metrics/data_source",
         "//tensorboard/webapp/persistent_settings",
         "//tensorboard/webapp/types",
+        "//tensorboard/webapp/util:dom",
         "@npm//@types/jasmine",
     ],
 )

tensorboard/webapp/metrics/store/metrics_reducers.ts

@@ -20,6 +20,7 @@ import {RouteKind} from '../../app_routing/types';
 import * as coreActions from '../../core/actions';
 import {globalSettingsLoaded} from '../../persistent_settings';
 import {DataLoadState} from '../../types/data';
+import {ElementId} from '../../util/dom';
 import {mapObjectValues} from '../../util/lang';
 import {composeReducers} from '../../util/ngrx';
 import * as actions from '../actions';
@@ -255,7 +256,7 @@ const {initialState, reducers: routeContextReducer} = createRouteContextedState<
     },
     settings: METRICS_SETTINGS_DEFAULT,
     settingOverrides: {},
-    visibleCards: new Set<CardId>(),
+    visibleCardMap: new Map<ElementId, CardId>(),
   },
 
   /** onRouteIdChanged */
@@ -265,7 +266,7 @@ const {initialState, reducers: routeContextReducer} = createRouteContextedState<
       // Reset visible cards in case we resume a route that was left dirty.
       // Since visibility tracking is async, the state may not have received
       // 'exited card' updates when it was cached by the router.
-      visibleCards: new Set<CardId>(),
+      visibleCardMap: new Map<ElementId, CardId>(),
     };
   }
 );
@@ -742,25 +743,24 @@ const reducer = createReducer(
     return {...state, tagGroupExpanded};
   }),
   on(actions.cardVisibilityChanged, (state, {enteredCards, exitedCards}) => {
-    if (enteredCards.size === 0 && exitedCards.size === 0) {
+    if (!enteredCards.length && !exitedCards.length) {
       return state;
     }
 
-    const visibleCards = new Set(state.visibleCards);
-    enteredCards.forEach((cardId) => {
-      visibleCards.add(cardId);
-    });
-    exitedCards.forEach((cardId) => {
-      visibleCards.delete(cardId);
-
-      if (enteredCards.has(cardId)) {
+    const visibleCardMap = new Map(state.visibleCardMap);
+    enteredCards.forEach(({elementId, cardId}) => {
+      const existingCardId = visibleCardMap.get(elementId) ?? null;
+      if (existingCardId !== null && existingCardId !== cardId) {
         throw new Error(
-          `A 'cardVisibilityChanged' with an invalid ` +
-            `payload contains overlapping sets`
+          `A DOM element cannot be reused for more than 1 unique card metadata`
         );
       }
+      visibleCardMap.set(elementId, cardId);
+    });
+    exitedCards.forEach(({elementId}) => {
+      visibleCardMap.delete(elementId);
     });
-    return {...state, visibleCards};
+    return {...state, visibleCardMap};
   }),
   on(actions.cardPinStateToggled, (state, {cardId}) => {
     const isPinnedCopy = state.pinnedCardToOriginal.has(cardId);

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -18,6 +18,7 @@ import {RouteKind} from '../../app_routing/types';
 import * as coreActions from '../../core/actions';
 import {globalSettingsLoaded} from '../../persistent_settings';
 import {DataLoadState} from '../../types/data';
+import {nextElementId} from '../../util/dom';
 import * as actions from '../actions';
 import {
   PluginType,
@@ -501,7 +502,10 @@ describe('metrics reducers', () => {
   describe('route id changes', () => {
     it('resets data when mounting a new route', () => {
       const prevState = buildMetricsState({
-        visibleCards: new Set(['card1', 'card2']),
+        visibleCardMap: new Map([
+          [nextElementId(), 'card1'],
+          [nextElementId(), 'card2'],
+        ]),
       });
 
       const navigateFrom1to2 = routingActions.navigated({
@@ -529,9 +533,9 @@ describe('metrics reducers', () => {
       nextState = reducers(nextState, navigateFrom2to1);
 
       const expectedState = buildMetricsState({
-        visibleCards: new Set(),
+        visibleCardMap: new Map(),
       });
-      expect(nextState.visibleCards).toEqual(expectedState.visibleCards);
+      expect(nextState.visibleCardMap).toEqual(expectedState.visibleCardMap);
     });
   });
 
@@ -1224,26 +1228,29 @@ describe('metrics reducers', () => {
   describe('card visibility', () => {
     it('no-ops when nothing is changed', () => {
       const beforeState = buildMetricsState({
-        visibleCards: new Set(['card1']),
+        visibleCardMap: new Map([[nextElementId(), 'card1']]),
       });
 
       const action = actions.cardVisibilityChanged({
-        enteredCards: new Set(),
-        exitedCards: new Set(),
+        enteredCards: [],
+        exitedCards: [],
       });
       const nextState = reducers(beforeState, action);
-      expect(nextState.visibleCards).toEqual(new Set(['card1']));
+      expect(nextState.visibleCardMap).toEqual(
+        new Map([[jasmine.any(Symbol), 'card1']])
+      );
       expect(nextState).toBe(beforeState);
     });
 
     it('handles bad payloads', () => {
+      const existingElementId = nextElementId();
       const beforeState = buildMetricsState({
-        visibleCards: new Set(['card1']),
+        visibleCardMap: new Map([[existingElementId, 'card1']]),
       });
 
       const action = actions.cardVisibilityChanged({
-        enteredCards: new Set(['duplicateCard']),
-        exitedCards: new Set(['duplicateCard']),
+        enteredCards: [{elementId: existingElementId, cardId: 'card2'}],
+        exitedCards: [],
       });
       let nextState = beforeState;
       expect(() => {
@@ -1253,19 +1260,57 @@ describe('metrics reducers', () => {
     });
 
     it('handles adding and removing cards', () => {
+      const existingElementIds = [nextElementId(), nextElementId()];
       const beforeState = buildMetricsState({
-        visibleCards: new Set(['existingCard1', 'existingCard2']),
+        visibleCardMap: new Map([
+          [existingElementIds[0], 'existingCard1'],
+          [existingElementIds[1], 'existingCard2'],
+        ]),
       });
 
+      const newCard1ElementId = nextElementId();
       const action = actions.cardVisibilityChanged({
-        enteredCards: new Set(['existingCard1', 'newCard1']),
-        exitedCards: new Set(['existingCard2', 'newCard2']),
+        enteredCards: [
+          {elementId: existingElementIds[0], cardId: 'existingCard1'},
+          {elementId: newCard1ElementId, cardId: 'newCard1'},
+        ],
+        exitedCards: [
+          {elementId: existingElementIds[1], cardId: 'existingCard2'},
+          {elementId: nextElementId(), cardId: 'newCard2'},
+        ],
       });
       const nextState = reducers(beforeState, action);
-      expect(nextState.visibleCards).toEqual(
-        new Set(['existingCard1', 'newCard1'])
+      expect(nextState.visibleCardMap).toEqual(
+        new Map([
+          [existingElementIds[0], 'existingCard1'],
+          [newCard1ElementId, 'newCard1'],
+        ])
       );
     });
+
+    it(
+      'marks a card as visible when it enters and exits on different ' +
+        'elements',
+      () => {
+        const existingElementIds = [nextElementId(), nextElementId()];
+        const beforeState = buildMetricsState({
+          visibleCardMap: new Map(),
+        });
+
+        const action = actions.cardVisibilityChanged({
+          enteredCards: [
+            {elementId: existingElementIds[0], cardId: 'duplicateCard'},
+          ],
+          exitedCards: [
+            {elementId: existingElementIds[1], cardId: 'duplicateCard'},
+          ],
+        });
+        const nextState = reducers(beforeState, action);
+        expect(nextState.visibleCardMap).toEqual(
+          new Map([[existingElementIds[0], 'duplicateCard']])
+        );
+      }
+    );
   });
 
   describe('cardPinStateToggled', () => {