[CrOS PhoneHub] Fix Phone Hub settings UI issues

* Indents subfeatures.
* Clicking on the row of editable features that don't link to a subpage
  will cause the feature to toggle.

Specs: https://carbon.googleplex.com/cros-ux/pages/phone-hub/settings
Screenshots:
https://screenshot.googleplex.com/3qpNqzYoNvdL8dN
https://screenshot.googleplex.com/5ejywFFaUtfetrF

Fixed: 1141609
Bug: 1106937
Change-Id: I342aa77a067b4301f3d676177bfa920ae86a385b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2518424
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824575}

chrome/browser/resources/settings/chromeos/multidevice_page/multidevice_feature_item.html

@@ -17,6 +17,14 @@
 <dom-module id="settings-multidevice-feature-item">
   <template>
     <style include="settings-shared">
+      :host([is-sub-feature]) iron-icon {
+        display: none;
+      }
+
+      :host([is-sub-feature]) .settings-box .middle {
+        padding-inline-start: 64px;
+      }
+
       #card {
         border-top: var(--cr-separator-line);
         border-top-style: var(--feature-item-border-top-style, solid);
@@ -33,7 +41,7 @@
     </style>
     <div id="card" class="settings-box two-line no-padding">
       <div class="link-wrapper" actionable
-           actionable$="[[hasSubpageClickHandler_(
+           actionable$="[[isRowClickable_(
                feature, pageContentData, subpageRoute)]]"
            on-click="handleItemClick_">
         <slot name="icon">

chrome/browser/resources/settings/chromeos/multidevice_page/multidevice_feature_item.js

@@ -33,6 +33,13 @@ Polymer({
      * @type {URLSearchParams|undefined}
      */
     subpageRouteUrlSearchParams: Object,
+
+    /** Whether if the feature is a sub-feature */
+    isSubFeature: {
+      type: Boolean,
+      value: false,
+      reflectToAttribute: true,
+    },
   },
 
   /** settings.RouteOriginBehavior override */
@@ -52,6 +59,15 @@ Polymer({
     elems[0].focus();
   },
 
+  /**
+   * @return {boolean}
+   * @private
+   */
+  isRowClickable_() {
+    return this.hasSubpageClickHandler_() ||
+        this.isFeatureStateEditable(this.feature);
+  },
+
   /**
    * @return {boolean}
    * @private
@@ -62,16 +78,22 @@ Polymer({
 
   /** @private */
   handleItemClick_(event) {
-    if (!this.hasSubpageClickHandler_()) {
-      return;
-    }
-
     // We do not navigate away if the click was on a link.
     if (event.path[0].tagName === 'A') {
       event.stopPropagation();
       return;
     }
 
+    if (!this.hasSubpageClickHandler_()) {
+      if (this.isFeatureStateEditable(this.feature)) {
+        // Toggle the editable feature if the feature is editable and does not
+        // link to a subpage.
+        this.shadowRoot.querySelector('settings-multidevice-feature-toggle')
+            .toggleFeature();
+      }
+      return;
+    }
+
     // Remove the search term when navigating to avoid potentially having any
     // visible search term reappear at a later time. See
     // https://crbug.com/989119.

chrome/browser/resources/settings/chromeos/multidevice_page/multidevice_feature_toggle.html

@@ -11,7 +11,7 @@
         aria-label$="[[getToggleA11yLabel_(feature)]]"
         checked="{{checked_}}"
         disabled="[[!isFeatureStateEditable(feature, pageContentData)]]"
-        on-change="onChange_">
+        on-change="toggleFeature">
     </cr-toggle>
   </template>
   <script src="multidevice_feature_toggle.js"></script>

chrome/browser/resources/settings/chromeos/multidevice_page/multidevice_feature_toggle.js

@@ -39,6 +39,21 @@ Polymer({
     this.$.toggle.focus();
   },
 
+  /**
+   * Callback for clicking on the toggle itself, or a row containing a toggle
+   * without other links. It attempts to toggle the feature's status if the user
+   * is allowed.
+   */
+  toggleFeature() {
+    this.resetChecked_();
+
+    // Pass the negation of |this.checked_|: this indicates that if the toggle
+    // is checked, the intent is for it to be unchecked, and vice versa.
+    this.fire(
+        'feature-toggle-clicked',
+        {feature: this.feature, enabled: !this.checked_});
+  },
+
   /**
    * Because MultiDevice prefs are only meant to be controlled via the
    * MultiDevice mojo service, we need the cr-toggle to appear not to change
@@ -66,21 +81,6 @@ Polymer({
     event.stopPropagation();
   },
 
-  /**
-   * Callback for clicking on the toggle. It attempts to toggle the feature's
-   * status if the user is allowed.
-   * @private
-   */
-  onChange_() {
-    this.resetChecked_();
-
-    // Pass the negation of |this.checked_|: this indicates that if the toggle
-    // is checked, the intent is for it to be unchecked, and vice versa.
-    this.fire(
-        'feature-toggle-clicked',
-        {feature: this.feature, enabled: !this.checked_});
-  },
-
   /**
    * Returns the A11y label for the toggle.
    * @private

chrome/browser/resources/settings/chromeos/multidevice_page/multidevice_subpage.html

@@ -124,7 +124,7 @@
             restamp>
           <settings-multidevice-feature-item id="phoneHubNotificationsItem"
               feature="[[MultiDeviceFeature.PHONE_HUB_NOTIFICATIONS]]"
-              page-content-data="[[pageContentData]]"
+              page-content-data="[[pageContentData]]" is-sub-feature
               deep-link-focus-id$="[[Setting.kPhoneHubNotificationsOnOff]]">
           </settings-multidevice-feature-item>
         </template>
@@ -134,7 +134,7 @@
             restamp>
           <settings-multidevice-feature-item id="phoneHubTaskContinuationItem"
               feature="[[MultiDeviceFeature.PHONE_HUB_TASK_CONTINUATION]]"
-              page-content-data="[[pageContentData]]"
+              page-content-data="[[pageContentData]]" is-sub-feature
               deep-link-focus-id$="[[Setting.kPhoneHubTaskContinuationOnOff]]">
           </settings-multidevice-feature-item>
         </template>

chrome/test/data/webui/settings/chromeos/multidevice_feature_item_tests.js

@@ -7,6 +7,7 @@
 
 // #import {MultiDeviceFeature, MultiDeviceFeatureState, routes, Router} from 'chrome://os-settings/chromeos/os_settings.js';
 // #import {assertEquals, assertFalse, assertTrue} from '../../chai_assert.js';
+// #import {eventToPromise, flushTasks} from 'chrome://test/test_util.m.js';
 // #import {flush} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 // clang-format on
 
@@ -68,6 +69,18 @@ suite('Multidevice', function() {
     assertEquals(initialRoute, settings.Router.getInstance().getCurrentRoute());
   }
 
+  /**
+   * Clicks an element, asserts whether the click fires a
+   * 'feature-toggle-clicked' event.
+   * @param {HTMLElement} element. Target of click.
+   */
+  async function checkWhetherFeatureToggleClickedFired(element) {
+    const showCellularSetupPromise =
+        test_util.eventToPromise('feature-toggle-clicked', featureToggle);
+    element.click();
+    await Promise.all([showCellularSetupPromise, test_util.flushTasks()]);
+  }
+
   setup(function() {
     PolymerTest.clearBody();
 
@@ -110,23 +123,35 @@ suite('Multidevice', function() {
     checkWhetherClickRoutesAway(link, false);
   });
 
+  test('row is clickable', async () => {
+    featureItem.feature = settings.MultiDeviceFeature.BETTER_TOGETHER_SUITE;
+    featureState = settings.MultiDeviceFeatureState.ENABLED_BY_USER;
+    featureItem.subpageRoute = null;
+
+    await checkWhetherFeatureToggleClickedFired(
+        featureItem.$$('#item-text-container'));
+    await checkWhetherFeatureToggleClickedFired(featureItem.$$('iron-icon'));
+    await checkWhetherFeatureToggleClickedFired(
+        featureItem.$$('#featureSecondary'));
+  });
+
   test('toggle click does not navigate to subpage in any state', function() {
     checkWhetherClickRoutesAway(featureToggle, false);
 
     // Checked and enabled
-    setCrToggle(true, true);
+    setCrToggle(true, false);
     checkWhetherClickRoutesAway(crToggle, false);
 
     // Checked and disabled
-    setCrToggle(true, false);
+    setCrToggle(true, true);
     checkWhetherClickRoutesAway(crToggle, false);
 
     // Unchecked and enabled
-    setCrToggle(false, true);
+    setCrToggle(false, false);
     checkWhetherClickRoutesAway(crToggle, false);
 
     // Unchecked and disabled
-    setCrToggle(false, false);
+    setCrToggle(false, true);
     checkWhetherClickRoutesAway(crToggle, false);
   });
 });

chrome/test/data/webui/settings/chromeos/os_settings_browsertest.js

@@ -1015,6 +1015,7 @@ var OSSettingsMultideviceFeatureItemTest = class extends OSSettingsBrowserTest {
   /** @override */
   get extraLibraries() {
     return super.extraLibraries.concat([
+      BROWSER_SETTINGS_PATH + '../test_util.js',
       'multidevice_feature_item_tests.js',
     ]);
   }