WebUI cr-toggle/cr-checkbox: Fix case where space toggles twice.

It was possible for the user to focus the inner <button> element via click
(otherwise button has tabindex -1). After that, pressing 'Space' once would
trigger both 'click' and 'keypress' toggling the state twice.

Bug: 841693,840723
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: If1200b6d39ac3eb4d6dc44e5622b75ed7f8beab8
Reviewed-on: https://chromium-review.googlesource.com/1058249
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558811}

chrome/test/data/webui/cr_elements/cr_checkbox_test.js

@@ -187,4 +187,15 @@ suite('cr-checkbox', function() {
     // Initializing with disabled should make tabindex="-1".
     assertEquals(-1, checkbox.tabIndex);
   });
+
+  // Ensure that even if user clicks on the element, the entire element gets
+  // focused, as opposed to focusing the inner <button> only.
+  test('FocusAfterClicking', function() {
+    const innerButton = checkbox.$$('button');
+    innerButton.focus();
+    assertNotChecked();
+    assertEquals(null, checkbox.shadowRoot.activeElement);
+    assertEquals('CR-CHECKBOX', document.activeElement.tagName);
+  });
+
 });

chrome/test/data/webui/cr_elements/cr_elements_browsertest.js

@@ -305,52 +305,6 @@ TEST_F('CrElementsToastTest', 'All', function() {
   mocha.run();
 });
 
-/**
- * @constructor
- * @extends {CrElementsBrowserTest}
- */
-function CrElementsToggleTest() {}
-
-CrElementsToggleTest.prototype = {
-  __proto__: CrElementsBrowserTest.prototype,
-
-  /** @override */
-  browsePreload: 'chrome://resources/cr_elements/cr_toggle/cr_toggle.html',
-
-  /** @override */
-  extraLibraries: CrElementsBrowserTest.prototype.extraLibraries.concat([
-    '../settings/test_util.js',
-    'cr_toggle_test.js',
-  ]),
-};
-
-TEST_F('CrElementsToggleTest', 'All', function() {
-  mocha.run();
-});
-
-/**
- * @constructor
- * @extends {CrElementsBrowserTest}
- */
-function CrElementsCheckboxTest() {}
-
-CrElementsCheckboxTest.prototype = {
-  __proto__: CrElementsBrowserTest.prototype,
-
-  /** @override */
-  browsePreload: 'chrome://resources/cr_elements/cr_checkbox/cr_checkbox.html',
-
-  /** @override */
-  extraLibraries: CrElementsBrowserTest.prototype.extraLibraries.concat([
-    '../settings/test_util.js',
-    'cr_checkbox_test.js',
-  ]),
-};
-
-TEST_F('CrElementsCheckboxTest', 'All', function() {
-  mocha.run();
-});
-
 /**
  * @constructor
  * @extends {CrElementsBrowserTest}

chrome/test/data/webui/cr_elements/cr_elements_focus_test.js

@@ -56,3 +56,50 @@ TEST_F('CrElementsProfileAvatarSelectorFocusTest', 'All', function() {
   cr_profile_avatar_selector.registerTests();
   mocha.grep(cr_profile_avatar_selector.TestNames.Focus).run();
 });
+
+/**
+ * @constructor
+ * @extends {CrElementsFocusTest}
+ */
+function CrElementsToggleTest() {}
+
+CrElementsToggleTest.prototype = {
+  __proto__: CrElementsFocusTest.prototype,
+
+  /** @override */
+  browsePreload: 'chrome://resources/cr_elements/cr_toggle/cr_toggle.html',
+
+  /** @override */
+  extraLibraries: CrElementsFocusTest.prototype.extraLibraries.concat([
+    '../settings/test_util.js',
+    'cr_toggle_test.js',
+  ]),
+};
+
+TEST_F('CrElementsToggleTest', 'All', function() {
+  mocha.run();
+});
+
+
+/**
+ * @constructor
+ * @extends {CrElementsFocusTest}
+ */
+function CrElementsCheckboxTest() {}
+
+CrElementsCheckboxTest.prototype = {
+  __proto__: CrElementsFocusTest.prototype,
+
+  /** @override */
+  browsePreload: 'chrome://resources/cr_elements/cr_checkbox/cr_checkbox.html',
+
+  /** @override */
+  extraLibraries: CrElementsFocusTest.prototype.extraLibraries.concat([
+    '../settings/test_util.js',
+    'cr_checkbox_test.js',
+  ]),
+};
+
+TEST_F('CrElementsCheckboxTest', 'All', function() {
+  mocha.run();
+});

chrome/test/data/webui/cr_elements/cr_toggle_test.js

@@ -180,6 +180,16 @@ suite('cr-toggle', function() {
         });
   });
 
+  // Ensure that even if user clicks on the element, the entire element gets
+  // focused, as opposed to focusing the inner <button> only.
+  test('FocusAfterClicking', function() {
+    const innerButton = toggle.$$('button');
+    innerButton.focus();
+    assertNotChecked();
+    assertEquals(null, toggle.shadowRoot.activeElement);
+    assertEquals('CR-TOGGLE', document.activeElement.tagName);
+  });
+
   // Test that the control is not affected by user interaction when disabled.
   test('ToggleWhenDisabled', function() {
     assertNotDisabled();

ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html

@@ -122,7 +122,7 @@ List of customizable styles:
     <!-- Mousing down then moving cursor out of this element should not trigger
          click on the parent. With <button> this works as expected, while <div>
          does not. -->
-    <button id="checkbox" tabindex="-1">
+    <button id="checkbox" tabindex="-1" on-focus="onButtonFocus_">
       <span id="checkmark"></span>
     </button>
     <div id="label-container"><slot></slot></div>

ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js

@@ -132,6 +132,14 @@ Polymer({
     this.toggleState_(true);
   },
 
+  /** @private */
+  onButtonFocus_: function() {
+    // Forward 'focus' to the enclosing element, so that a subsequent 'Space'
+    // keystroke does not trigger both 'keypress' and 'click' which would toggle
+    // the state twice erroneously.
+    this.focus();
+  },
+
   // customize the element's ripple
   _createRipple: function() {
     this._rippleContainer = this.$.checkbox;

ui/webui/resources/cr_elements/cr_toggle/cr_toggle.html

@@ -97,7 +97,7 @@
          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">
+    <button tabindex="-1" on-focus="onButtonFocus_">
       <span id="bar"></span>
       <span id="knob"></span>
     </button>

ui/webui/resources/cr_elements/cr_toggle/cr_toggle.js

@@ -173,6 +173,14 @@ Polymer({
     }
   },
 
+  /** @private */
+  onButtonFocus_: function() {
+    // Forward 'focus' to the enclosing element, so that a subsequent 'Space'
+    // keystroke does not trigger both 'keypress' and 'click' which would toggle
+    // the state twice erroneously.
+    this.focus();
+  },
+
   // customize the element's ripple
   _createRipple: function() {
     this._rippleContainer = this.$.knob;