From f82d40e122ae271424c45f5e080e9bc5aa6567a8 Mon Sep 17 00:00:00 2001
From: Stephan Lee <stephanwlee@gmail.com>
Date: Wed, 3 Nov 2021 16:56:56 -0700
Subject: [PATCH 1/2] timeseries: fix button toggle state issue

Angular material components are not truly "controlled input" that even
if the parent sets `checked` state to false, if user clicked on the
button from false to true to cause a redux state change that makes
parent put that state, it will have a value `checked`. This is directly
observable by printing out value of `checked` vs. value set by parent.

We work around this issue by simply using button that acts like a
checkbox instead of using an Angular material component. Also, styled
carefully to make it look about the same.
---
 .../main_view/main_view_component.ng.html     | 32 ++++++++++++-------
 .../views/main_view/main_view_component.scss  | 19 ++++++++---
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html b/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html
index 21ea4784495..a74c3d105ce 100644
--- a/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html
+++ b/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html
@@ -17,30 +17,38 @@
 <div class="toolbar">
   <metrics-tag-filter></metrics-tag-filter>
   <mat-button-toggle-group class="filter-view" multiple appearance="standard">
-    <mat-button-toggle
-      [checked]="filteredPluginTypes.size === 0"
+    <button
+      mat-button
+      role="checkbox"
+      [attr.aria-checked]="filteredPluginTypes.size === 0"
       (click)="onPluginTypeAllToggled.emit()"
     >
       All
-    </mat-button-toggle>
-    <mat-button-toggle
-      [checked]="filteredPluginTypes.has(PluginType.SCALARS)"
+    </button>
+    <button
+      mat-button
+      role="checkbox"
+      [attr.aria-checked]="filteredPluginTypes.has(PluginType.SCALARS)"
       (click)="onPluginTypeToggled.emit(PluginType.SCALARS)"
     >
       Scalars
-    </mat-button-toggle>
-    <mat-button-toggle
-      [checked]="filteredPluginTypes.has(PluginType.IMAGES)"
+    </button>
+    <button
+      mat-button
+      role="checkbox"
+      [attr.aria-checked]="filteredPluginTypes.has(PluginType.IMAGES)"
       (click)="onPluginTypeToggled.emit(PluginType.IMAGES)"
     >
       Image
-    </mat-button-toggle>
-    <mat-button-toggle
-      [checked]="filteredPluginTypes.has(PluginType.HISTOGRAMS)"
+    </button>
+    <button
+      mat-button
+      role="checkbox"
+      [attr.aria-checked]="filteredPluginTypes.has(PluginType.HISTOGRAMS)"
       (click)="onPluginTypeToggled.emit(PluginType.HISTOGRAMS)"
     >
       Histogram
-    </mat-button-toggle>
+    </button>
   </mat-button-toggle-group>
   <div class="right-items">
     <button
diff --git a/tensorboard/webapp/metrics/views/main_view/main_view_component.scss b/tensorboard/webapp/metrics/views/main_view/main_view_component.scss
index d1e5f112bed..62644f031a9 100644
--- a/tensorboard/webapp/metrics/views/main_view/main_view_component.scss
+++ b/tensorboard/webapp/metrics/views/main_view/main_view_component.scss
@@ -42,16 +42,27 @@ limitations under the License.
 }
 
 .filter-view {
+  border-radius: 4px;
   flex: none;
   margin-right: 5px;
 
-  mat-button-toggle {
+  button {
     $_height: 25px;
-    height: $_height;
+
+    border-radius: 0;
     font-size: 12px;
+    font-weight: normal;
+    height: $_height;
+    line-height: $_height;
+    min-width: unset;
+    padding: 0 12px;
+
+    & + button {
+      @include tb-theme-foreground-prop(border-left, border, 1px solid);
+    }
 
-    ::ng-deep .mat-button-toggle-label-content {
-      line-height: $_height;
+    &[aria-checked='true'] {
+      @include tb-theme-background-prop(background-color, selected-button);
     }
   }
 }

From 7436aa0f685e83bd20b723f54a61b6ef73c59942 Mon Sep 17 00:00:00 2001
From: Stephan Lee <stephanwlee@gmail.com>
Date: Wed, 3 Nov 2021 17:47:00 -0700
Subject: [PATCH 2/2] fix tests

---
 .../main_view/main_view_component.ng.html     |  4 ++++
 .../metrics/views/main_view/main_view_test.ts | 22 +++++++------------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html b/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html
index a74c3d105ce..912d9c662f6 100644
--- a/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html
+++ b/tensorboard/webapp/metrics/views/main_view/main_view_component.ng.html
@@ -19,6 +19,7 @@
   <mat-button-toggle-group class="filter-view" multiple appearance="standard">
     <button
       mat-button
+      class="filter-view-button"
       role="checkbox"
       [attr.aria-checked]="filteredPluginTypes.size === 0"
       (click)="onPluginTypeAllToggled.emit()"
@@ -27,6 +28,7 @@
     </button>
     <button
       mat-button
+      class="filter-view-button"
       role="checkbox"
       [attr.aria-checked]="filteredPluginTypes.has(PluginType.SCALARS)"
       (click)="onPluginTypeToggled.emit(PluginType.SCALARS)"
@@ -35,6 +37,7 @@
     </button>
     <button
       mat-button
+      class="filter-view-button"
       role="checkbox"
       [attr.aria-checked]="filteredPluginTypes.has(PluginType.IMAGES)"
       (click)="onPluginTypeToggled.emit(PluginType.IMAGES)"
@@ -43,6 +46,7 @@
     </button>
     <button
       mat-button
+      class="filter-view-button"
       role="checkbox"
       [attr.aria-checked]="filteredPluginTypes.has(PluginType.HISTOGRAMS)"
       (click)="onPluginTypeToggled.emit(PluginType.HISTOGRAMS)"
diff --git a/tensorboard/webapp/metrics/views/main_view/main_view_test.ts b/tensorboard/webapp/metrics/views/main_view/main_view_test.ts
index 8a840662ffd..f4726e36270 100644
--- a/tensorboard/webapp/metrics/views/main_view/main_view_test.ts
+++ b/tensorboard/webapp/metrics/views/main_view/main_view_test.ts
@@ -210,14 +210,11 @@ describe('metrics main view', () => {
       fixture.detectChanges();
 
       const buttons = fixture.debugElement.queryAll(
-        By.css('mat-button-toggle')
+        By.css('.filter-view-button')
       );
-      expect(buttons.map((button) => button.properties['checked'])).toEqual([
-        true,
-        false,
-        false,
-        false,
-      ]);
+      expect(
+        buttons.map((button) => button.attributes['aria-checked'])
+      ).toEqual(['true', 'false', 'false', 'false']);
 
       store.overrideSelector(
         selectors.getMetricsFilteredPluginTypes,
@@ -225,12 +222,9 @@ describe('metrics main view', () => {
       );
       store.refreshState();
       fixture.detectChanges();
-      expect(buttons.map((button) => button.properties['checked'])).toEqual([
-        false,
-        false,
-        true,
-        false,
-      ]);
+      expect(
+        buttons.map((button) => button.attributes['aria-checked'])
+      ).toEqual(['false', 'false', 'true', 'false']);
     });
 
     it('dispatches action when clicked on a plugin type', () => {
@@ -238,7 +232,7 @@ describe('metrics main view', () => {
       fixture.detectChanges();
 
       const [, scalars, images] = fixture.debugElement.queryAll(
-        By.css('mat-button-toggle')
+        By.css('.filter-view-button')
       );
 
       scalars.nativeElement.click();