From 79985f94e043bd4de3f3652a19f4b7698291cdd7 Mon Sep 17 00:00:00 2001
From: dpapad <dpapad@chromium.org>
Date: Fri, 4 May 2018 01:09:23 +0000
Subject: [PATCH] WebUI: Simplify cr-toggle case of finishing drag gesture
 outside element.

Previous code was working around crbug.com/689158 and crbug.com/768555.
New code still works around them, but without requiring JS logic to do so,
achieved by wrapping the cr-toggle contents with a <button>.

Bug: 768073
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I435f6a44979a934b8548b13bdbd947d324eab5c5
Reviewed-on: https://chromium-review.googlesource.com/1040683
Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555944}
---
 .../resources/md_extensions/toggle_row.html   |  2 +-
 .../resources/md_extensions/toggle_row.js     | 12 -------
 .../controls/settings_toggle_button.js        |  7 -----
 .../data/webui/cr_elements/cr_toggle_test.js  |  4 +--
 .../extensions/a11y/extensions_a11y_test.js   | 10 +++++-
 .../webui/settings/a11y/basic_a11y_test.js    |  8 +----
 .../a11y/edit_dictionary_a11y_test.js         |  4 ---
 .../settings/a11y/manage_profile_a11y_test.js | 31 ++++++++-----------
 .../settings/a11y/passwords_a11y_test.js      |  9 ++----
 .../a11y/settings_accessibility_test.js       | 10 ++++++
 .../cr_elements/cr_toggle/cr_toggle.html      | 31 +++++++++++++------
 .../cr_elements/cr_toggle/cr_toggle.js        | 23 +-------------
 12 files changed, 61 insertions(+), 90 deletions(-)

diff --git a/chrome/browser/resources/md_extensions/toggle_row.html b/chrome/browser/resources/md_extensions/toggle_row.html
index b326dc8ca0007b..b71803d44ca07f 100644
--- a/chrome/browser/resources/md_extensions/toggle_row.html
+++ b/chrome/browser/resources/md_extensions/toggle_row.html
@@ -37,7 +37,7 @@
         flex: 1;
       }
     </style>
-    <label id="label" on-click="onLabelTap_">
+    <label id="label">
       <input id="native" type="checkbox" checked="[[checked]]"
           on-change="onNativeChange_" on-click="onNativeClick_">
       <slot></slot>
diff --git a/chrome/browser/resources/md_extensions/toggle_row.js b/chrome/browser/resources/md_extensions/toggle_row.js
index 8667db24ce43d0..1b5de89d990ddc 100644
--- a/chrome/browser/resources/md_extensions/toggle_row.js
+++ b/chrome/browser/resources/md_extensions/toggle_row.js
@@ -26,18 +26,6 @@ cr.define('extensions', function() {
       return this.$.label;
     },
 
-    /**
-     * @param {!Event}
-     * @private
-     */
-    onLabelTap_: function(e) {
-      // If the interaction sequence (pointerdown+pointerup) began within the
-      // cr-toggle itself, then prevent this event from changing the state of
-      // the toggle.
-      if (this.$.crToggle.shouldIgnoreHostTap(e))
-        e.preventDefault();
-    },
-
     /**
      * @param {!Event}
      * @private
diff --git a/chrome/browser/resources/settings/controls/settings_toggle_button.js b/chrome/browser/resources/settings/controls/settings_toggle_button.js
index a2a896c0e6a488..320cacdd22a34e 100644
--- a/chrome/browser/resources/settings/controls/settings_toggle_button.js
+++ b/chrome/browser/resources/settings/controls/settings_toggle_button.js
@@ -78,13 +78,6 @@ Polymer({
     if (this.controlDisabled())
       return;
 
-    // Ignore this |tap| event, if the interaction sequence
-    // (pointerdown+pointerup) began within the cr-toggle itself.
-    if (/** @type {!CrToggleElement} */ (this.$.control)
-            .shouldIgnoreHostTap(e)) {
-      return;
-    }
-
     this.checked = !this.checked;
     this.notifyChangedByUserInteraction();
     this.fire('change');
diff --git a/chrome/test/data/webui/cr_elements/cr_toggle_test.js b/chrome/test/data/webui/cr_elements/cr_toggle_test.js
index 202c01a4db099f..5258d316003885 100644
--- a/chrome/test/data/webui/cr_elements/cr_toggle_test.js
+++ b/chrome/test/data/webui/cr_elements/cr_toggle_test.js
@@ -20,7 +20,7 @@ suite('cr-toggle', function() {
     assertTrue(toggle.hasAttribute('checked'));
     assertEquals('true', toggle.getAttribute('aria-pressed'));
     // Asserting that the toggle button has actually moved.
-    assertTrue(getComputedStyle(toggle.$.button).transform.includes('matrix'));
+    assertTrue(getComputedStyle(toggle.$.knob).transform.includes('matrix'));
   }
 
   function assertNotChecked() {
@@ -28,7 +28,7 @@ suite('cr-toggle', function() {
     assertEquals(null, toggle.getAttribute('checked'));
     assertEquals('false', toggle.getAttribute('aria-pressed'));
     // Asserting that the toggle button has not moved.
-    assertEquals('none', getComputedStyle(toggle.$.button).transform);
+    assertEquals('none', getComputedStyle(toggle.$.knob).transform);
   }
 
   function assertDisabled() {
diff --git a/chrome/test/data/webui/extensions/a11y/extensions_a11y_test.js b/chrome/test/data/webui/extensions/a11y/extensions_a11y_test.js
index a2763a6b905d06..8eba99c7b0b7bd 100644
--- a/chrome/test/data/webui/extensions/a11y/extensions_a11y_test.js
+++ b/chrome/test/data/webui/extensions/a11y/extensions_a11y_test.js
@@ -60,7 +60,15 @@ var CrExtensionsA11yTest = class extends PolymerTest {
                 node.element, 'iron-iconset-svg');
           });
         });
-      }
+      },
+      'button-name': function(nodeResult) {
+        const parentNode = nodeResult.element.parentNode;
+
+        // Ignore the <button> residing within cr-toggle, which has tabindex -1
+        // anyway.
+        return parentNode && parentNode.host &&
+            parentNode.host.tagName == 'CR-TOGGLE';
+      },
     };
   }
 
diff --git a/chrome/test/data/webui/settings/a11y/basic_a11y_test.js b/chrome/test/data/webui/settings/a11y/basic_a11y_test.js
index d9c860068f1fc9..dc9f421708625c 100644
--- a/chrome/test/data/webui/settings/a11y/basic_a11y_test.js
+++ b/chrome/test/data/webui/settings/a11y/basic_a11y_test.js
@@ -19,11 +19,5 @@ AccessibilityTest.define('SettingsAccessibilityTest', {
   /** @override */
   tests: {'Accessible with No Changes': function() {}},
   /** @override */
-  violationFilter:
-      Object.assign({}, SettingsAccessibilityTest.violationFilter, {
-        'button-name': function(nodeResult) {
-          const node = nodeResult.element;
-          return node.classList.contains('icon-expand-more');
-        },
-      }),
+  violationFilter: SettingsAccessibilityTest.violationFilter,
 });
diff --git a/chrome/test/data/webui/settings/a11y/edit_dictionary_a11y_test.js b/chrome/test/data/webui/settings/a11y/edit_dictionary_a11y_test.js
index 151ea512b683bd..17e5aa68dca8f5 100644
--- a/chrome/test/data/webui/settings/a11y/edit_dictionary_a11y_test.js
+++ b/chrome/test/data/webui/settings/a11y/edit_dictionary_a11y_test.js
@@ -35,10 +35,6 @@ AccessibilityTest.define('SettingsAccessibilityTest', {
               nodeResult.element.getAttribute('aria-describedby');
           return describerId === '' && nodeResult.element.tagName == 'INPUT';
         },
-        'button-name': function(nodeResult) {
-          const node = nodeResult.element;
-          return node.classList.contains('icon-expand-more');
-        },
         'tabindex': function(nodeResult) {
           // TODO(crbug.com/808276): remove this exception when bug is fixed.
           return nodeResult.element.getAttribute('tabindex') == '0';
diff --git a/chrome/test/data/webui/settings/a11y/manage_profile_a11y_test.js b/chrome/test/data/webui/settings/a11y/manage_profile_a11y_test.js
index ea27705254aade..f637d14923e74f 100644
--- a/chrome/test/data/webui/settings/a11y/manage_profile_a11y_test.js
+++ b/chrome/test/data/webui/settings/a11y/manage_profile_a11y_test.js
@@ -27,24 +27,19 @@ AccessibilityTest.define('SettingsAccessibilityTest', {
   /** @override */
   tests: {'Accessible with No Changes': function() {}},
   /** @override */
-  violationFilter: {
-    'aria-valid-attr': function(nodeResult) {
-      return nodeResult.element.hasAttribute('aria-active-attribute');
-    },
-    // Excuse Polymer paper-input elements.
-    'aria-valid-attr-value': function(nodeResult) {
-      const describerId = nodeResult.element.getAttribute('aria-describedby');
-      return describerId === '' && nodeResult.element.tagName == 'INPUT';
-    },
-    'button-name': function(nodeResult) {
-      const node = nodeResult.element;
-      return node.classList.contains('icon-expand-more');
-    },
-    'tabindex': function(nodeResult) {
-      // TODO(crbug.com/808276): remove this exception when bug is fixed.
-      return nodeResult.element.getAttribute('tabindex') == '0';
-    },
-  },
+  violationFilter:
+      Object.assign({}, SettingsAccessibilityTest.violationFilter, {
+        // Excuse Polymer paper-input elements.
+        'aria-valid-attr-value': function(nodeResult) {
+          const describerId =
+              nodeResult.element.getAttribute('aria-describedby');
+          return describerId === '' && nodeResult.element.tagName == 'INPUT';
+        },
+        'tabindex': function(nodeResult) {
+          // TODO(crbug.com/808276): remove this exception when bug is fixed.
+          return nodeResult.element.getAttribute('tabindex') == '0';
+        },
+      }),
 });
 
 GEN('#endif  // !defined(OS_CHROMEOS)');
diff --git a/chrome/test/data/webui/settings/a11y/passwords_a11y_test.js b/chrome/test/data/webui/settings/a11y/passwords_a11y_test.js
index 4d2db5aee5c151..28844f596fa1c3 100644
--- a/chrome/test/data/webui/settings/a11y/passwords_a11y_test.js
+++ b/chrome/test/data/webui/settings/a11y/passwords_a11y_test.js
@@ -101,12 +101,7 @@ AccessibilityTest.define('SettingsA11yManagePasswords', {
       assertEquals(10, this.passwordsSection.savedPasswords.length);
     },
   },
+
   /** @override */
-  violationFilter:
-      Object.assign({}, SettingsAccessibilityTest.violationFilter, {
-        'button-name': function(nodeResult) {
-          const node = nodeResult.element;
-          return node.classList.contains('icon-expand-more');
-        },
-      }),
+  violationFilter: SettingsAccessibilityTest.violationFilter,
 });
diff --git a/chrome/test/data/webui/settings/a11y/settings_accessibility_test.js b/chrome/test/data/webui/settings/a11y/settings_accessibility_test.js
index b09130eb0d1473..e5e9da227cca1f 100644
--- a/chrome/test/data/webui/settings/a11y/settings_accessibility_test.js
+++ b/chrome/test/data/webui/settings/a11y/settings_accessibility_test.js
@@ -37,6 +37,16 @@ SettingsAccessibilityTest.violationFilter = {
   'aria-valid-attr': function(nodeResult) {
     return nodeResult.element.hasAttribute('aria-active-attribute');
   },
+  'button-name': function(nodeResult) {
+    if (nodeResult.element.classList.contains('icon-expand-more'))
+      return true;
+
+    // Ignore the <button> residing within cr-toggle, which has tabindex -1
+    // anyway.
+    const parentNode = nodeResult.element.parentNode;
+    return parentNode && parentNode.host &&
+        parentNode.host.tagName == 'CR-TOGGLE';
+  },
 };
 
 SettingsAccessibilityTest.prototype = {
diff --git a/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.html b/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.html
index c23cf3a10c451e..0ea8fbf12491c7 100644
--- a/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.html
+++ b/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.html
@@ -16,7 +16,13 @@
         pointer-events: none;
       }
 
-      #container {
+      button {
+        background: none;
+        border: none;
+        cursor: inherit;
+        display: block;
+        outline: inherit;
+        padding: 0;
         position: relative;
         width: 34px;
         z-index: 0;
@@ -45,11 +51,12 @@
         opacity: 0.12;
       }
 
-      #button {
+      #knob {
         background-color: var(
             --cr-toggle-unchecked-button-color, var(--paper-grey-50));
         border-radius: 50%;
         box-shadow: 0 1px 3px 0 rgba(0, 0, 0, 0.4);
+        display: block;
         height: 16px;
         position: relative;
         transition: transform linear 80ms, background-color linear 80ms;
@@ -57,17 +64,17 @@
         z-index: 1;
       }
 
-      :host([checked]) #button {
+      :host([checked]) #knob {
         background-color: var(
             --cr-toggle-checked-button-color, var(--google-blue-500));
         transform: translate3d(18px, 0, 0);
       }
 
-      :host-context([dir=rtl]):host([checked]) #button {
+      :host-context([dir=rtl]):host([checked]) #knob {
         transform: translate3d(-18px, 0, 0);
       }
 
-      :host([disabled]) #button {
+      :host([disabled]) #knob {
         background-color: #bdbdbd;
         opacity: 1;
       }
@@ -92,10 +99,16 @@
         color: var(--cr-toggle-checked-ink-color, var(--primary-color));
       }
     </style>
-    <div id="container">
-     <div id="bar"></div>
-     <div id="button"></div>
-    </div>
+    <!-- A <button> is used (instead of a plain <div>) to avoid a corner case
+         when pointerdown is initiated in cr-toggle, but pointerup happens
+         outside the bounds of cr-toggle, which would end up firing a 'click'
+         event on the parent and unexpectedly modify the toggle's state (see
+         context at crbug.com/689158 and crbug.com/768555). The 'click'
+         event is not fired when the <button> wrapper is used. -->
+    <button tabindex="-1">
+      <span id="bar"></span>
+      <span id="knob"></span>
+    </button>
   </template>
   <script src="cr_toggle.js"></script>
 </dom-module>
diff --git a/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.js b/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.js
index d4d38a789a0c43..25c6441b81cac2 100644
--- a/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.js
+++ b/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.js
@@ -63,9 +63,6 @@ Polymer({
    */
   handledInPointerMove_: false,
 
-  /** @private {number} */
-  lastPointerUpTime_: 0,
-
   /** @override */
   attached: function() {
     let direction = this.matches(':host-context([dir=rtl]) cr-toggle') ? -1 : 1;
@@ -113,7 +110,6 @@ Polymer({
 
   /** @private */
   onPointerUp_: function(e) {
-    this.lastPointerUpTime_ = e.timeStamp;
     this.removeEventListener('pointermove', this.boundPointerMove_);
   },
 
@@ -151,23 +147,6 @@ Polymer({
     this.toggleState_(false);
   },
 
-  /**
-   * Whether the host of this element should handle a 'click' event it received,
-   * to be used when clicking on the parent is supposed to toggle the cr-toggle.
-   *
-   * This is necessary to avoid a corner case when pointerdown is initiated
-   * in cr-toggle, but pointerup happens outside the bounds of cr-toggle, which
-   * ends up firing a 'click' event on the parent (see context at
-   * crbug.com/689158 and crbug.com/768555).
-   * @param {!Event} e
-   * @return {boolean}
-   */
-  shouldIgnoreHostTap: function(e) {
-    let timeStamp =
-        e.detail.sourceEvent ? e.detail.sourceEvent.timeStamp : e.timeStamp;
-    return timeStamp == this.lastPointerUpTime_;
-  },
-
   /**
    * @param {boolean} fromKeyboard
    * @private
@@ -196,7 +175,7 @@ Polymer({
 
   // customize the element's ripple
   _createRipple: function() {
-    this._rippleContainer = this.$.button;
+    this._rippleContainer = this.$.knob;
     let ripple = Polymer.PaperRippleBehavior._createRipple();
     ripple.id = 'ink';
     ripple.setAttribute('recenters', '');