Fix keyboard handling of button-style checkbox/radio button groups #28834
Conversation
- remove `preventDefault()` which stopped regular keyboard behaviour (as currently using keyboard only changes styling on the `.btn`) - special-case checkbox input check in `toggle()` function, to avoid double-triggering (leaving checkbox back in its original state) when using mouse
… label - while the ideal pattern for authors to use is `<label><input...></label>`, they MAY stray from it. in those cases, the extra check (which avoids the double-triggering, since we don't prevent default anymore) should not happen, as otherwise the checkbox would never get triggered
an admittedly jury-rigged tweak, but as running `trigger('click')` in qunit/headless chrome does not fire the same default behaviour as a real click on a `<label>` (which the changed button.js now
relies on), and the point of the test is not related to that aspect, this should be acceptable.
otherwise, this needs to be tested not with `trigger('click')`, but with a fully fledged async event dispatch of synthetic click event.
|
This PR is specifically targeted at v4, as in v5 this whole approach will be dropped in favor of a CSS-only solution, which will obviate most of the bolted-on functionality of the button.js script |
|
@patrickhlauke: it seems BrowserStack tests fail. |
…t much earlier bail a lot sooner and avoid extra contortions...
interesting. assume the BrowserStack tests don't run locally, right? (as doing worth noting that the qunit tests relating to sending a |
|
ok, i give up...can't figure out why the test blows up in qunit in Firefox. testing the same scenario manually in Firefox shows the correct behaviour, so it seems definitely something to do with the way the test harness does something unexpected in Firefox. this could well be tied to the same problem I saw with the other test, where using ...alternatively, which is what i'll try next, the qunit test itself can be modified to avoid reliance on default browser behaviour that comes from a real mouse click on a real |
…ntly our qunit tests don't do
|
No, the Browserstack tests run on Browserstack. Maybe @Johann-S has an idea. |
|
Instead of using |
See if using `click()` fixes the problem/triggers default browser behaviour without the need for replacing `<label>` with `<div>`
ok, going to give that a try, but i'm fairly sure in the past when i tried that it wasn't sufficient as the tests were still happening earlier than the actual default behaviour taking place (so it would need some async or a timeout to wait for the headless browser to do its thing) |
patrickhlauke
force-pushed
the
patrickhlauke-button-plugin-kbd-fix
branch
from
510607b to
d35e5de
Compare
|
still not able to work out why the test fails in Firefox 67. testing this manually shows me the correct behaviour. no idea why the qunit test itself fails. either there's some bug in Firefox that I'm not aware of, or...don't know, to be honest. i will need help trying to work this out (which is frustrating as this appears to correctly fix the keyboard issue at last). @Johann-S any ideas? |
…click on DOM node instead
the original problem (which the hack to replace `<label>` with `<div>` circumvented) was that `click()` on a jQuery object is simply an alias for `trigger('click')`. adding the `[0]` instead lets
us trigger the proper `click()` method on the actual DOM node, which then does the correct browser behaviour.
It's contrived (you wouldn't have a label around a hidden and a real input - this may have been more relevant back when these custom checkboxes used something other than `<label>`). It also seems to cause a bug in IE10/11 otherwise (even just in vanilla HTML, no BS or jQuery involved) where clicking on the label with the mouse doesn't trigger checkbox. As this is contrived, opting to jury-rig this.
|
sigh...fairy snuff. done. so, want me to squash and force-push? or can this be "Squash and merge"d when it comes time? |
|
@patrickhlauke since you know exactly how you want to have the commits, feel free to clean it up locally and force push, otherwise if you just want one commit, you can do it via the web UI when merging. |
|
I changed the target branch to my |
|
so, good to squash and merge? |
|
Yup |
|
Should I add this to the shiplist, or is that sorted out separately when you do stuff in |
|
We just track the PRs in the v4.3.2 project. |
…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
…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
…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
…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
…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
|
Hi there, BTW: great framework! |
|
No exact date, but we'll try to release 4.x soon. |
After various past attempts, this hopefully addresses any outstanding keyboard-handling issues with the "faked" button-style checkbox/radio button groups https://getbootstrap.com/docs/4.3/components/buttons/#checkbox-and-radio-buttons
Closes #28564
Closes #26855
Closes #28847
(pro-tip to test if keyboard is being handled correctly: for the checkbox and radio button buttons, use devtools, find the nested
input, and disabled theposition:absoluterule in the relevant CSS ... now you can see exactly if a radio or checkbox gets toggled properly or not)