Fix keyboard handling of button-style checkbox/radio button groups (#28834)

- adds more defensive checks to make sure no unnecessary toggling happens on disabled buttons; this also fixes an up-to-now undiscovered bug where a toggle button with `.disabled` class would still have its `aria-pressed` toggled
- adds a set of explicit tests for the above case of disabled buttons and `aria-pressed`
- remove a now irrelevant (or at least very nonsensical) test for `<label>` containing both an actionable and a `hidden` `<input>`
- expand the test for disabled checkbox to also explicitly test starting conditions (used mainly in my debugging)
- ensure that `$btn[0].click()` is used to click checkboxes in tests, rather than the `click()` on the jquery object which is simply a shorthand for `trigger('click')` and does not actually trigger the browser default behavior
- remove the `preventDefault()` from the button handling, which was preventing correct keyboard functionality for checkboxes/radio buttons
- add extra logic to the button.js code to handle checkboxes correctly and avoid double-triggering as a result of mouse interactions (which saw the checkboxes being toggled twice, thus returning them to their original state)
- add logic that prevents the `checked` property from being added incorrectly for any inputs other than radio buttons and checkboxes
- added more tests (including the most basic test for a properly triggered fake checkbox button)
- work around Firefox bug #1540995 (which this code was hitting after removing the `preventDefault()`, due to Firefox's incorrect toggling of disabled checkboxes when programmatically (but not manually) activated with a `click()` event

js/src/button.js

@@ -81,15 +81,16 @@ class Button {
               $(activeElement).removeClass(ClassName.ACTIVE)
             }
           }
+        } else if (input.type === 'checkbox') {
+          if (this._element.tagName === 'LABEL' && input.checked === this._element.classList.contains(ClassName.ACTIVE)) {
+            triggerChangeEvent = false
+          }
+        } else {
+          // if it's not a radio button or checkbox don't add a pointless/invalid checked property to the input
+          triggerChangeEvent = false
         }
 
         if (triggerChangeEvent) {
-          if (input.hasAttribute('disabled') ||
-            rootElement.hasAttribute('disabled') ||
-            input.classList.contains('disabled') ||
-            rootElement.classList.contains('disabled')) {
-            return
-          }
           input.checked = !this._element.classList.contains(ClassName.ACTIVE)
           $(input).trigger('change')
         }
@@ -99,6 +100,7 @@ class Button {
       }
     }
 
+    if (!(this._element.hasAttribute('disabled') || this._element.classList.contains('disabled'))) {
       if (addAriaPressed) {
         this._element.setAttribute('aria-pressed',
           !this._element.classList.contains(ClassName.ACTIVE))
@@ -108,6 +110,7 @@ class Button {
         $(this._element).toggleClass(ClassName.ACTIVE)
       }
     }
+  }
 
   dispose() {
     $.removeData(this._element, DATA_KEY)
@@ -140,15 +143,24 @@ class Button {
 
 $(document)
   .on(Event.CLICK_DATA_API, Selector.DATA_TOGGLE_CARROT, (event) => {
-    event.preventDefault()
-
     let button = event.target
 
     if (!$(button).hasClass(ClassName.BUTTON)) {
-      button = $(button).closest(Selector.BUTTON)
+      button = $(button).closest(Selector.BUTTON)[0]
+    }
+
+    if (!button || button.hasAttribute('disabled') || button.classList.contains('disabled')) {
+      event.preventDefault() // work around Firefox bug #1540995
+    } else {
+      const inputBtn = button.querySelector(Selector.INPUT)
+
+      if (inputBtn && (inputBtn.hasAttribute('disabled') || inputBtn.classList.contains('disabled'))) {
+        event.preventDefault() // work around Firefox bug #1540995
+        return
       }
 
       Button._jQueryInterface.call($(button), 'toggle')
+    }
   })
   .on(Event.FOCUS_BLUR_DATA_API, Selector.DATA_TOGGLE_CARROT, (event) => {
     const button = $(event.target).closest(Selector.BUTTON)[0]

js/tests/unit/button.js

@@ -61,6 +61,22 @@ $(function () {
     assert.strictEqual($btn.attr('aria-pressed'), 'true', 'btn aria-pressed state is true')
   })
 
+  QUnit.test('should not toggle aria-pressed on buttons with disabled class', function (assert) {
+    assert.expect(2)
+    var $btn = $('<button class="btn disabled" data-toggle="button" aria-pressed="false">redux</button>')
+    assert.strictEqual($btn.attr('aria-pressed'), 'false', 'btn aria-pressed state is false')
+    $btn.bootstrapButton('toggle')
+    assert.strictEqual($btn.attr('aria-pressed'), 'false', 'btn aria-pressed state is still false')
+  })
+
+  QUnit.test('should not toggle aria-pressed on buttons that are disabled', function (assert) {
+    assert.expect(2)
+    var $btn = $('<button class="btn" data-toggle="button" aria-pressed="false" disabled>redux</button>')
+    assert.strictEqual($btn.attr('aria-pressed'), 'false', 'btn aria-pressed state is false')
+    $btn.bootstrapButton('toggle')
+    assert.strictEqual($btn.attr('aria-pressed'), 'false', 'btn aria-pressed state is still false')
+  })
+
   QUnit.test('should toggle aria-pressed on buttons with container', function (assert) {
     assert.expect(1)
     var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
@@ -139,29 +155,6 @@ $(function () {
     assert.ok($btn2.find('input').prop('checked'), 'btn2 is checked')
   })
 
-  QUnit.test('should only toggle selectable inputs', function (assert) {
-    assert.expect(6)
-    var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
-      '<label class="btn btn-primary active">' +
-      '<input type="hidden" name="option1" id="option1-default" value="false">' +
-      '<input type="checkbox" name="option1" id="option1" checked="true"> Option 1' +
-      '</label>' +
-      '</div>'
-    var $group = $(groupHTML).appendTo('#qunit-fixture')
-
-    var $btn = $group.children().eq(0)
-    var $hidden = $btn.find('input#option1-default')
-    var $cb = $btn.find('input#option1')
-
-    assert.ok($btn.hasClass('active'), 'btn has active class')
-    assert.ok($cb.prop('checked'), 'btn is checked')
-    assert.ok(!$hidden.prop('checked'), 'hidden is not checked')
-    $btn.trigger('click')
-    assert.ok(!$btn.hasClass('active'), 'btn does not have active class')
-    assert.ok(!$cb.prop('checked'), 'btn is not checked')
-    assert.ok(!$hidden.prop('checked'), 'hidden is not checked') // should not be changed
-  })
-
   QUnit.test('should not add aria-pressed on labels for radio/checkbox inputs in a data-toggle="buttons" group', function (assert) {
     assert.expect(2)
     var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
@@ -181,10 +174,10 @@ $(function () {
   })
 
   QUnit.test('should handle disabled attribute on non-button elements', function (assert) {
-    assert.expect(2)
+    assert.expect(4)
     var groupHTML = '<div class="btn-group disabled" data-toggle="buttons" aria-disabled="true" disabled>' +
-      '<label class="btn btn-danger disabled" aria-disabled="true" disabled>' +
+      '<label class="btn btn-danger disabled">' +
-      '<input type="checkbox" aria-disabled="true" autocomplete="off" disabled class="disabled"/>' +
+      '<input type="checkbox" aria-disabled="true" autocomplete="off" disabled>' +
       '</label>' +
       '</div>'
     var $group = $(groupHTML).appendTo('#qunit-fixture')
@@ -192,9 +185,121 @@ $(function () {
     var $btn = $group.children().eq(0)
     var $input = $btn.children().eq(0)
 
-    $btn.trigger('click')
+    assert.ok($btn.is(':not(.active)'), 'button is initially not active')
+    assert.ok(!$input.prop('checked'), 'checkbox is initially not checked')
+    $btn[0].click() // fire a real click on the DOM node itself, not a click() on the jQuery object that just aliases to trigger('click')
     assert.ok($btn.is(':not(.active)'), 'button did not become active')
-    assert.ok(!$input.is(':checked'), 'checkbox did not get checked')
+    assert.ok(!$input.prop('checked'), 'checkbox did not get checked')
+  })
+
+  QUnit.test('should not set active class if inner hidden checkbox is disabled but author forgot to set disabled class on outer button', function (assert) {
+    assert.expect(4)
+    var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
+      '<label class="btn btn-danger">' +
+      '<input type="checkbox" autocomplete="off" disabled>' +
+      '</label>' +
+      '</div>'
+    var $group = $(groupHTML).appendTo('#qunit-fixture')
+
+    var $btn = $group.children().eq(0)
+    var $input = $btn.children().eq(0)
+
+    assert.ok($btn.is(':not(.active)'), 'button is initially not active')
+    assert.ok(!$input.prop('checked'), 'checkbox is initially not checked')
+    $btn[0].click() // fire a real click on the DOM node itself, not a click() on the jQuery object that just aliases to trigger('click')
+    assert.ok($btn.is(':not(.active)'), 'button did not become active')
+    assert.ok(!$input.prop('checked'), 'checkbox did not get checked')
+  })
+
+  QUnit.test('should correctly set checked state on input and active class on label when using <label><input></label> structure', function (assert) {
+    assert.expect(4)
+    var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
+      '<label class="btn">' +
+      '<input type="checkbox" autocomplete="off">' +
+      '</label>' +
+      '</div>'
+    var $group = $(groupHTML).appendTo('#qunit-fixture')
+
+    var $label = $group.children().eq(0)
+    var $input = $label.children().eq(0)
+
+    assert.ok($label.is(':not(.active)'), 'label is initially not active')
+    assert.ok(!$input.prop('checked'), 'checkbox is initially not checked')
+    $label[0].click() // fire a real click on the DOM node itself, not a click() on the jQuery object that just aliases to trigger('click')
+    assert.ok($label.is('.active'), 'label is active after click')
+    assert.ok($input.prop('checked'), 'checkbox is checked after click')
+  })
+
+  QUnit.test('should correctly set checked state on input and active class on the faked button when using <div><input></div> structure', function (assert) {
+    assert.expect(4)
+    var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
+      '<div class="btn">' +
+      '<input type="checkbox" autocomplete="off" aria-label="Check">' +
+      '</div>' +
+      '</div>'
+    var $group = $(groupHTML).appendTo('#qunit-fixture')
+
+    var $btn = $group.children().eq(0)
+    var $input = $btn.children().eq(0)
+
+    assert.ok($btn.is(':not(.active)'), '<div> is initially not active')
+    assert.ok(!$input.prop('checked'), 'checkbox is initially not checked')
+    $btn[0].click() // fire a real click on the DOM node itself, not a click() on the jQuery object that just aliases to trigger('click')
+    assert.ok($btn.is('.active'), '<div> is active after click')
+    assert.ok($input.prop('checked'), 'checkbox is checked after click')
+  })
+
+  QUnit.test('should not do anything if the click was just sent to the outer container with data-toggle', function (assert) {
+    assert.expect(4)
+    var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
+      '<label class="btn">' +
+      '<input type="checkbox" autocomplete="off">' +
+      '</label>' +
+      '</div>'
+    var $group = $(groupHTML).appendTo('#qunit-fixture')
+
+    var $label = $group.children().eq(0)
+    var $input = $label.children().eq(0)
+
+    assert.ok($label.is(':not(.active)'), 'label is initially not active')
+    assert.ok(!$input.prop('checked'), 'checkbox is initially not checked')
+    $group[0].click() // fire a real click on the DOM node itself, not a click() on the jQuery object that just aliases to trigger('click')
+    assert.ok($label.is(':not(.active)'), 'label is not active after click')
+    assert.ok(!$input.prop('checked'), 'checkbox is not checked after click')
+  })
+
+  QUnit.test('should not try and set checked property on an input of type="hidden"', function (assert) {
+    assert.expect(2)
+    var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
+      '<label class="btn">' +
+      '<input type="hidden" autocomplete="off">' +
+      '</label>' +
+      '</div>'
+    var $group = $(groupHTML).appendTo('#qunit-fixture')
+
+    var $label = $group.children().eq(0)
+    var $input = $label.children().eq(0)
+
+    assert.ok(!$input.prop('checked'), 'hidden input initially has no checked property')
+    $label[0].click() // fire a real click on the DOM node itself, not a click() on the jQuery object that just aliases to trigger('click')
+    assert.ok(!$input.prop('checked'), 'hidden input does not have a checked property')
+  })
+
+  QUnit.test('should not try and set checked property on an input that is not a radio button or checkbox', function (assert) {
+    assert.expect(2)
+    var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
+      '<label class="btn">' +
+      '<input type="text" autocomplete="off">' +
+      '</label>' +
+      '</div>'
+    var $group = $(groupHTML).appendTo('#qunit-fixture')
+
+    var $label = $group.children().eq(0)
+    var $input = $label.children().eq(0)
+
+    assert.ok(!$input.prop('checked'), 'text input initially has no checked property')
+    $label[0].click() // fire a real click on the DOM node itself, not a click() on the jQuery object that just aliases to trigger('click')
+    assert.ok(!$input.prop('checked'), 'text input does not have a checked property')
   })
 
   QUnit.test('dispose should remove data and the element', function (assert) {