Skip to content

Commit

Permalink
buttons plugin : avoid multiple change event trigger (#31000)
Browse files Browse the repository at this point in the history
- add unit test to count how many events are thrown when widget contains multiple tags inside label
- add a parameter to toggle, if click event is provided onto an input then don't trigger another change event already thrown by the browser
- simplify the case where toggle interface is called click provide from input itself OR it's a button without label. If label is present, then browser propagate click event from childrens through label and then cause multiple calls to toggle
- the test assumes that `.btn` class is always set onto the label if there's one, otherwise need to update this plugin and look for label around the input

Test with keyboard, mouse and js click call

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
  • Loading branch information
Lausselloic and XhmikosR committed Oct 2, 2020
1 parent d9b4426 commit 24572b1
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 6 deletions.
4 changes: 2 additions & 2 deletions .bundlewatch.config.json
Expand Up @@ -26,15 +26,15 @@
},
{
"path": "./dist/js/bootstrap.bundle.js",
"maxSize": "47.50 kB"
"maxSize": "47.5 kB"
},
{
"path": "./dist/js/bootstrap.bundle.min.js",
"maxSize": "21.5 kB"
},
{
"path": "./dist/js/bootstrap.js",
"maxSize": "25 kB"
"maxSize": "25.5 kB"
},
{
"path": "./dist/js/bootstrap.min.js",
Expand Down
13 changes: 9 additions & 4 deletions js/src/button.js
Expand Up @@ -46,6 +46,7 @@ const EVENT_LOAD_DATA_API = `load${EVENT_KEY}${DATA_API_KEY}`
class Button {
constructor(element) {
this._element = element
this.shouldAvoidTriggerChange = false
}

// Getters
Expand Down Expand Up @@ -83,7 +84,9 @@ class Button {
input.checked = !this._element.classList.contains(CLASS_NAME_ACTIVE)
}

$(input).trigger('change')
if (!this.shouldAvoidTriggerChange) {
$(input).trigger('change')
}
}

input.focus()
Expand All @@ -109,7 +112,7 @@ class Button {

// Static

static _jQueryInterface(config) {
static _jQueryInterface(config, avoidTriggerChange) {
return this.each(function () {
const $element = $(this)
let data = $element.data(DATA_KEY)
Expand All @@ -119,6 +122,8 @@ class Button {
$element.data(DATA_KEY, data)
}

data.shouldAvoidTriggerChange = avoidTriggerChange

if (config === 'toggle') {
data[config]()
}
Expand Down Expand Up @@ -151,8 +156,8 @@ $(document)
return
}

if (initialButton.tagName !== 'LABEL' || inputBtn && inputBtn.type !== 'checkbox') {
Button._jQueryInterface.call($(button), 'toggle')
if (initialButton.tagName === 'INPUT' || button.tagName !== 'LABEL') {
Button._jQueryInterface.call($(button), 'toggle', initialButton.tagName === 'INPUT')
}
}
})
Expand Down
26 changes: 26 additions & 0 deletions js/tests/unit/button.js
Expand Up @@ -180,6 +180,32 @@ $(function () {
$group.find('label').trigger('click')
})

QUnit.test('should trigger label change event only once', function (assert) {
assert.expect(1)
var done = assert.async()
var countChangeEvent = 0

var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
'<label class="btn btn-primary">' +
'<input type="checkbox"><span class="check">✓</span> <i class="far fa-clipboard"></i> <span class="d-none d-lg-inline">checkbox</span>' +
'</label>' +
'</div>'
var $group = $(groupHTML).appendTo('#qunit-fixture')

var $btn = $group.children().eq(0)

$group.find('label').on('change', function () {
countChangeEvent++
})

setTimeout(function () {
assert.ok(countChangeEvent === 1, 'onchange event fired only once')
done()
}, 5)

$btn[0].click()
})

QUnit.test('should check for closest matching toggle', function (assert) {
assert.expect(18)
var groupHTML = '<div class="btn-group" data-toggle="buttons">' +
Expand Down

0 comments on commit 24572b1

Please sign in to comment.