From c1b9d37e6f69b66a268063e04893ec13926957cf Mon Sep 17 00:00:00 2001
From: Sophia Lin <sophialin@google.com>
Date: Tue, 28 Jun 2022 10:42:15 +0000
Subject: [PATCH] crostini: Update UI for port forwarding page for
 multi-containers

Modify multi-container UI for port forwarding so it is more consistent
with other parts of Settings UI. Changes include:
* replaced cards with indented table
* replaced triple dot menu for each port with single X icon button
* aligned labels with table column
* removed redundant table title
* added label for container selector

Details and screenshots here: https://docs.google.com/drawings/d/1LWtQ9jeNncbCp-qa6Bpnl8F6jTkKQnscdIwBBvM-Wk0/edit?usp=sharing

containers on DUT

Bug: 1261319, b:228140711
Test: unit and browser tests, manually tested with multi + single
Change-Id: I70c264169c1b626565a669a93b7dfd10fa689306
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3723239
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Commit-Queue: Sophia Lin <sophialin@google.com>
Cr-Commit-Position: refs/heads/main@{#1018610}
---
 .../crostini_container_select.html            |   1 +
 .../crostini_port_forwarding.html             | 106 ++++++++----------
 .../crostini_page/crostini_port_forwarding.js |  32 +-----
 .../settings/chromeos/crostini_page_test.js   |   5 +-
 4 files changed, 52 insertions(+), 92 deletions(-)

diff --git a/chrome/browser/resources/settings/chromeos/crostini_page/crostini_container_select.html b/chrome/browser/resources/settings/chromeos/crostini_page/crostini_container_select.html
index dda4253ace34c4..83ff9d9c84f338 100644
--- a/chrome/browser/resources/settings/chromeos/crostini_page/crostini_container_select.html
+++ b/chrome/browser/resources/settings/chromeos/crostini_page/crostini_container_select.html
@@ -1,4 +1,5 @@
 <style include="settings-shared md-select"></style>
+<label class="cr-form-field-label">Container</label>
 <select id="selectContainer"
     class="md-select"
     value="containerLabel_(containerId)"
diff --git a/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.html b/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.html
index 9bb9456ece8265..84a99ae375878c 100644
--- a/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.html
+++ b/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.html
@@ -14,8 +14,14 @@
   }
 
   .label-column {
+    align-items: center;
+    display: flex;
     flex: 1;
     flex-grow: 3;
+  }
+  
+  .label-text {
+    flex-grow: 3;
     word-break: break-all;
   }
 
@@ -39,31 +45,19 @@
     margin-inline-start:  8px;
   }
 
-  #portForwardingListPortLabel {
-    margin-inline-end: calc(var(--cr-toggle-margin-inline-start) +
-                            var(--cr-toggle-width));
-  }
-
   #portForwardingListContainerId {
-    color: var(--cros-text-color-disabled);
-    margin-inline-start: 8px;
+    border-top: var(--cr-separator-line);
   }
 
-  #portForwardingListCard {
-    background-color: var(--cr-card-background-color);
-    border-radius: var(--cr-card-border-radius);
-    box-shadow: var(--cr-card-shadow);
+  #portForwardingDescription {
+    flex-grow: 3;
   }
 
 </style>
-<div class="settings-box first"
-    id="portForwardingDescription">
-  $i18n{crostiniPortForwardingDescription}
-</div>
 <div id="addPort" class="settings-box first">
-  <div id="portForwardingTableTitle" class="start"
-      aria-hidden="true">
-    $i18n{crostiniPortForwardingTableTitle}
+  <div
+      id="portForwardingDescription">
+    $i18n{crostiniPortForwardingDescription}
   </div>
   <cr-button on-click="onAddPortClick_"
       aria-label="$i18n{crostiniPortForwardingAddPortButtonDescription}"
@@ -90,11 +84,11 @@
       as="containerInfo" index-as="cidx" mutable-data>
     <template is="dom-if" 
         if="[[hasContainerPorts_(allPorts_, containerInfo.id)]]" restamp>
-      <div id="portForwardingListContainerId" 
+      <h2 id="portForwardingListContainerId" 
           hidden="[[!showContainerId_(allPorts_, containerInfo.id)]]"
           class="settings-box first">
           [[containerLabel_(containerInfo.id)]]
-      </div>
+      </h2>
       <div class="list-frame vertical-list" id="portForwardingListCard">
         <div class="list-item">
           <div id="portForwardingListPortNumber"
@@ -124,36 +118,38 @@
                 </template>
               </span>
             </div>
-            <div id="crostiniPortLabel[[cidx]]-[[index]]"
-                class="label-column"
-                aria-hidden="true">
-              [[item.label]]
+            <div class="label-column" aria-hidden="true">
+              <div id="crostiniPortLabel[[cidx]]-[[index]]"
+                  class="label-text"
+                  aria-hidden="true">
+                [[item.label]]
+              </div>
+              <cr-toggle
+                  id="toggleActivationButton[[cidx]]-[[index]]"
+                  checked="[[item.is_active]]"
+                  data-port-number$="[[item.port_number]]"
+                  data-protocol-type$="[[item.protocol_type]]"
+                  data-container-id="[[item.container_id]]"
+                  on-change="onPortActivationChange_"
+                  aria-label="$i18n{crostiniPortForwardingToggleAriaLabel}"
+                  aria-describedby$="crostiniPort[[cidx]]-[[index]] 
+                      crostiniPortLabel[[cidx]]-[[index]]"
+                  disabled="[[!containerInfo.ipv4]]">
+              </cr-toggle>
+              <cr-icon-button 
+                  id="removeSinglePortButton[[cidx]]-[[index]]"
+                  class="icon-clear"
+                  title="$i18n{moreActions}"
+                  on-click="onRemoveSinglePortClick_"
+                  data-port-number$="[[item.port_number]]"
+                  data-protocol-type$="[[item.protocol_type]]"
+                  data-container-id="[[item.container_id]]"
+                  aria-label=
+                      "$i18n{crostiniPortForwardingShowMoreActionsAriaLabel}"
+                  aria-describedby$="crostiniPort[[cidx]]-[[index]] 
+                      crostiniPortLabel[[cidx]]-[[index]]">
+              </cr-icon-button>
             </div>
-            <cr-toggle
-                id="toggleActivationButton[[cidx]]-[[index]]"
-                checked="[[item.is_active]]"
-                data-port-number$="[[item.port_number]]"
-                data-protocol-type$="[[item.protocol_type]]"
-                data-container-id="[[item.container_id]]"
-                on-change="onPortActivationChange_"
-                aria-label="$i18n{crostiniPortForwardingToggleAriaLabel}"
-                aria-describedby$="crostiniPort[[cidx]]-[[index]] 
-                    crostiniPortLabel[[cidx]]-[[index]]"
-                disabled="[[!containerInfo.ipv4]]">
-            </cr-toggle>
-            <cr-icon-button 
-                id="showRemoveSinglePortMenu[[cidx]]-[[index]]"
-                class="icon-more-vert"
-                title="$i18n{moreActions}"
-                on-click="onShowRemoveSinglePortMenuClick_"
-                data-port-number$="[[item.port_number]]"
-                data-protocol-type$="[[item.protocol_type]]"
-                data-container-id="[[item.container_id]]"
-                aria-label=
-                    "$i18n{crostiniPortForwardingShowMoreActionsAriaLabel}"
-                aria-describedby$="crostiniPort[[cidx]]-[[index]] 
-                    crostiniPortLabel[[cidx]]-[[index]]">
-            </cr-icon-button>
           </div>
         </template>
       </div>
@@ -180,18 +176,6 @@
     </cr-action-menu>
   </template>
 </cr-lazy-render>
-<cr-lazy-render id="removeSinglePortMenu">
-  <template>
-    <cr-action-menu class="complex" role-description="$i18n{menu}">
-      <button id="removeSinglePortButton"
-          class="dropdown-item"
-          role="menuitem"
-          on-click="onRemoveSinglePortClick_">
-        $i18n{crostiniPortForwardingRemovePort}
-      </button>
-    </cr-action-menu>
-  </template>
-</cr-lazy-render>
 <cr-toast id="errorToast" duration="3000">
   <div class="error-message" id="errorMessage">
     <iron-icon id="errorIcon" icon="cr:error-outline"></iron-icon>
diff --git a/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.js b/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.js
index d222cbd8c0f215..e1282bd2e20db8 100644
--- a/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.js
+++ b/chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.js
@@ -208,43 +208,21 @@ class CrostiniPortForwardingElement extends CrostiniPortForwardingBase {
    * @param {!Event} event
    * @private
    */
-  onShowRemoveSinglePortMenuClick_(event) {
+  onRemoveSinglePortClick_(event) {
     const dataSet = /** @type {{portNumber: string, protocolType: string}} */
         (event.currentTarget.dataset);
-    const id = /** @type {GuestId} */
+    const containerId = /** @type {GuestId} */
         (event.currentTarget['dataContainerId']);
-    this.lastMenuOpenedPort_ = {
-      port_number: Number(dataSet.portNumber),
-      protocol_type: /** @type {!CrostiniPortProtocol} */
-          (Number(dataSet.protocolType)),
-      container_id: id,
-    };
-    const menu = /** @type {!CrActionMenuElement} */
-        (this.$.removeSinglePortMenu.get());
-    menu.showAt(/** @type {!HTMLElement} */ (event.target));
-  }
+    const protocolType = /** @type {!CrostiniPortProtocol} */
+        (Number(dataSet.protocolType));
 
-  /**
-   * @param {!Event} event
-   * @private
-   */
-  onRemoveSinglePortClick_(event) {
-    const menu = /** @type {!CrActionMenuElement} */
-        (this.$.removeSinglePortMenu.get());
-    assert(
-        menu.open && this.lastMenuOpenedPort_.port_number != null &&
-        this.lastMenuOpenedPort_.protocol_type != null);
     this.browserProxy_
         .removeCrostiniPortForward(
-            this.lastMenuOpenedPort_.container_id,
-            this.lastMenuOpenedPort_.port_number,
-            this.lastMenuOpenedPort_.protocol_type)
+            containerId, Number(dataSet.portNumber), protocolType)
         .then(result => {
           // TODO(crbug.com/848127): Error handling for result
           recordSettingChange();
-          menu.close();
         });
-    this.lastMenuOpenedPort_ = null;
   }
 
   /**
diff --git a/chrome/test/data/webui/settings/chromeos/crostini_page_test.js b/chrome/test/data/webui/settings/chromeos/crostini_page_test.js
index c29faa33445e68..12d552ca183de6 100644
--- a/chrome/test/data/webui/settings/chromeos/crostini_page_test.js
+++ b/chrome/test/data/webui/settings/chromeos/crostini_page_test.js
@@ -758,11 +758,8 @@ suite('CrostiniPageTests', function() {
         await flushTasks();
         subpage = crostiniPage.shadowRoot.querySelector(
             'settings-crostini-port-forwarding');
-        subpage.shadowRoot.querySelector('#showRemoveSinglePortMenu0-0')
-            .click();
-        await flushTasks();
 
-        subpage.shadowRoot.querySelector('#removeSinglePortButton').click();
+        subpage.shadowRoot.querySelector('#removeSinglePortButton0-0').click();
         assertEquals(
             1, crostiniBrowserProxy.getCallCount('removeCrostiniPortForward'));
         const args =